1323449 - implement is() method for testing boolean characteristics

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8818549 [details] [diff] [review]
patch

Isn't the API a bit odd. It checks role and/or states, but "defunct" isn't necessarily part of states here. Seems like an odd inconsistency. And "unknown" role isn't possible either. Yet .states may contain "defunct" or .role be "unknown"

I think I need a spec to read to understand what the API should be here.
Couldn't find the spec. Bug 1313691 links to http://a11y-api.github.io/a11y-api/spec/ but that isn't available.

Feel free to ask review again, if you can provide a link to the spec explaining the expected behavior, and explain why "defunct" or "unknown" aren't accepted.

Comment 2

[alexander :surkov (:asurkov)]

the method comes from https://wicg.github.io/a11yapi/

"unknown" is not really valid role, and it shouldn't happen in real life, "defunct" state will be matched if the states contains it. I guess it makes sense to consider an accessible object as defunct too if it has null mIntl member. Was that your point?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Your patch doesn't match "defunct" since "defunct" is explicitly added by AccessibleNode::GetStates, but the patch uses lower level GetOrCreateAccService()->GetStringStates.
Or, "defunct" is matched if someone has called .states before, since then it is cached in mStates. But that is just wrong if the behavior of is() depends on whether .states has been accessed.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(hmm, can't AccessibleNode::GetStates crash? What guarantees mStates points to a non-null value when "defunct" is added to it)

Comment 5

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #2)
> the method comes from https://wicg.github.io/a11yapi/
> 
> "unknown" is not really valid role

oh, I see, you concerned about 'unknown' value used in AccessibleNode::GetRole() as a fallback, I think I should add those parts for consistency.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #3)
> Your patch doesn't match "defunct" since "defunct" is explicitly added by
> AccessibleNode::GetStates, but the patch uses lower level
> GetOrCreateAccService()->GetStringStates.
> Or, "defunct" is matched if someone has called .states before, since then it
> is cached in mStates. But that is just wrong if the behavior of is() depends
> on whether .states has been accessed.

(In reply to Olli Pettay [:smaug] from comment #4)
> (hmm, can't AccessibleNode::GetStates crash? What guarantees mStates points
> to a non-null value when "defunct" is added to it)

I was supposed to use aStates instead mStates I think. I'll file a patch for this bit.

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 8

[alexander :surkov (:asurkov)]

Attachment: fix GetStates

Comment 9

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a73ed8c73a01f41b59dc09f2b6e834521dc1064
Bug 1323449 - implement is() method for testing boolean characteristics, r=smaug

Comment 10

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c94ca0caa6f2f07505fb9ed3889ce0de6ed6fd23
Bug 1323449 - AOM .states may crash if not attached to internal object, r=smaug

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1a73ed8c73a0

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c94ca0caa6f2

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Did the right stuff land here? I mean, wasn't only one of the patches landed?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #13)
> Did the right stuff land here? I mean, wasn't only one of the patches landed?

they two went in separate commits (comment 11 and comment 12)

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

ah, ok, missed the other one.