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

AriaAttributes cannot be nullable #1058

Closed
annevk opened this issue Sep 25, 2019 · 10 comments · Fixed by #1261
Closed

AriaAttributes cannot be nullable #1058

annevk opened this issue Sep 25, 2019 · 10 comments · Fixed by #1261
Assignees
Milestone

Comments

@annevk
Copy link
Member

annevk commented Sep 25, 2019

At least as currently defined it seems to use HTML's reflect concept and that only works for normal strings, not nullable strings.

@bzbarsky
Copy link

Relatedly, ariaActiveDescendantElement is claimed to reflect aria-activedescendant, but reflection is not defined at all for things that return Element...

@bzbarsky
Copy link

Looks like Chrome's IDL allows [Reflect] in all sorts of cases that are not supported and just silently ignores it sometimes or something...

Anyway, the nullable DOMString case lands in https://cs.chromium.org/chromium/src/third_party/blink/renderer/bindings/core/v8/generated_code_helper.cc?l=145-159&rcl=0f7e56f37186facc1d9c9edfccb279892b2beb59 which I believe ends up doing the bit at https://cs.chromium.org/chromium/src/third_party/blink/renderer/bindings/core/v8/v8_string_resource.h?l=179-183&rcl=c109404f6df33717fecf49aabb495f2f2e4059aa to create a "null string" to the attribute setter, but the setter then ends up treating that just like it would an empty string.

That's on set. On get, I think empty string is returned if the attr is not present, but I'm not 100% sure from reading the code, and I don't know how to enable this bit in Chrome to test.

@domenic
Copy link
Contributor

domenic commented Oct 31, 2019

Relatedly, ariaActiveDescendantElement is claimed to reflect aria-activedescendant, but reflection is not defined at all for things that return Element...

This is in progress in whatwg/html#3917 but there are still open design discussions; see whatwg/html#3515 and whatwg/html#4925. (Those open design questions are why we're holding back on shipping.)

Nullable string reflection is defined for enumerated attributes, but I'm not sure the ARIA attributes in question are enumerated, or even if they are, whether they have properly-defined missing value defaults. We'll look in to that further. /cc @rakina @alice. (If either of you knows Meredith's GitHub handle please add her too.)

@domenic
Copy link
Contributor

domenic commented Oct 31, 2019

At a quick look, it seems like a reasonable fix here would be some glue at the top of the ARIA spec, which says something like "when we have a table enumerating the values for an attribute, then that attribute is an enumerated attribute. When we notate one of the values as (default), then that value is the missing value default and invalid value default for the attribute."

@alice
Copy link

alice commented Oct 31, 2019

(Meredith's GitHub handle is @dot-miniscule)

@alice
Copy link

alice commented Nov 1, 2019

Now that I'm a bit more awake: @domenic's suggestion in #1058 (comment) seems reasonable to me.

Do we need to worry about missing value defaults for string-type attributes like aria-label?

@domenic
Copy link
Contributor

domenic commented Nov 4, 2019

Do we need to worry about missing value defaults for string-type attributes like aria-label?

Nope, only enumerated attributes. Which I guess would correspond to true/false, tristate, true/false/undefined, and token types in http://w3c.github.io/aria/#propcharacteristic_value.

token list is an interesting one. It seems like maybe aria-dropeffect="" and role="" should be exposed as DOMTokenLists? For those you'd want to do

[SameObject, PutForwards=value] readonly attribute DOMTokenList role;

@carmacleod
Copy link
Contributor

I don't know if this is relevant here, but in case it is, note that aria-activedescendant can have a null (empty string) value and it's not an enumerated attribute. Authors do this all the time in the context of a combobox, i.e. when the combobox is closed, code just nulls activedescendant instead of removing it.

The following paragraph was added to the State and Property Attribute Processing section in the ARIA spec in #501 in order to accomodate this:

Sometimes states and properties are present in the DOM but have a zero-length string ("") as their value. Authors MAY specify a zero-length string ("") for any supported (but not required) state or property. User agents SHOULD treat state and property attributes with a value of "" the same as they treat an absent attribute. For supported states and properties, this corresponds to the default value, but if it is a required attribute, it signals an author error, and the implicit value for the role is used.

@sinabahram
Copy link

Ignore my comment on this thread if you got it before I deleted it. Email client issue. My appologies smile.

@annevk
Copy link
Member Author

annevk commented May 13, 2020

@carmacleod the empty string is not a null value. Null in this thread refers to the IDL null type signified by a question mark in IDL blocks. It's distinct from the empty string and in the JavaScript bindings you would get null (and not "").

jnurthen pushed a commit that referenced this issue Jun 12, 2020
* Enumerated values section now replaces superseded state_prop_values section, and includes examples per comment by @asurkov
* remove the nullable flag from most IDL properties; Note: the remaining props are removed in another branch, so not modified here.
Co-authored-by: Carolyn MacLeod <[email protected]>
jnurthen pushed a commit that referenced this issue Jul 10, 2020
* Enumerated values section now replaces superseded state_prop_values section, and includes examples per comment by @asurkov
* remove the nullable flag from most IDL properties; Note: the remaining props are removed in another branch, so not modified here.
Co-authored-by: Carolyn MacLeod <[email protected]>
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 a pull request may close this issue.

8 participants