Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix transition boolean props complete #2627

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sapphi-red
Copy link
Contributor

To complete without ="", the type needed to be "v" instead of "b".

@yoyo930021
Copy link
Member

I'm not sure this is a good change.
I am not sure about the definition of type.

It looks like changing here is also a possible direction.

if (type !== 'v' && value.length) {

@octref What do you think?

@sapphi-red
Copy link
Contributor Author

reversed attribute of ol which is a boolean attribute1 has the type v, and aria-atomic attribute which should take "true" or "false"2 has the type b.

ol: genTag(
'The ol element represents a list of items, where the items have been intentionally ordered, such that changing the order would change the meaning of the document.',
['reversed:v', 'start', 'type:lt']

export function getHTML5TagProvider(): IHTMLTagProvider {
const globalAttributes = [
'aria-activedescendant',
'aria-atomic:b',

Vue's boolean attributes does allow a value contrary to HTML boolean attributes, but it also does not require a value contrary to ARIA's boolean attributes.

Changing that line to (type !== 'v' || type !== 'b') && value.length will effect ARIA's boolean attributes. So I think changing it to v is better.

Footnotes

  1. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/ol#attributes, https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#boolean-attributes

  2. https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Live_Regions#advanced_live_regions

getAttribute(
'css',
'b',
'v',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This option default is true.
So we need ="".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes it should be.

@@ -133,7 +133,7 @@ const vueTags = {
const valueSets = {
transMode: ['out-in', 'in-out'],
transType: ['transition', 'animation'],
b: ['true', 'false']
v: ['true', 'false']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also need to keep b.

@yoyo930021
Copy link
Member

reversed attribute of ol which is a boolean attribute1 has the type v, and aria-atomic attribute which should take "true" or "false"2 has the type b.

ol: genTag(
'The ol element represents a list of items, where the items have been intentionally ordered, such that changing the order would change the meaning of the document.',
['reversed:v', 'start', 'type:lt']

export function getHTML5TagProvider(): IHTMLTagProvider {
const globalAttributes = [
'aria-activedescendant',
'aria-atomic:b',

Vue's boolean attributes does allow a value contrary to HTML boolean attributes, but it also does not require a value contrary to ARIA's boolean attributes.

Changing that line to (type !== 'v' || type !== 'b') && value.length will effect ARIA's boolean attributes. So I think changing it to v is better.

I am not so familiar with this part.
I think some options may still be necessary.
@octref we need your help.

Footnotes

  1. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/ol#attributes, https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#boolean-attributes

  2. https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Live_Regions#advanced_live_regions

@yoyo930021 yoyo930021 requested a review from octref June 1, 2021 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants