Closed Bug 1323449 Opened 4 years ago Closed 4 years ago

implement is() method for testing boolean characteristics

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
No description provided.
Attachment #8818549 - Flags: review?(bugs)
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.
Attachment #8818549 - Flags: review?(bugs) → review-
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?
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
(hmm, can't AccessibleNode::GetStates crash? What guarantees mStates points to a non-null value when "defunct" is added to it)
(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.
(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.
Attached patch patch2Splinter Review
Assignee: nobody → surkov.alexander
Attachment #8818571 - Flags: review?(bugs)
Attached patch fix GetStatesSplinter Review
Attachment #8818549 - Attachment is obsolete: true
Attachment #8818572 - Flags: review?(bugs)
Attachment #8818572 - Flags: review?(bugs) → review+
Attachment #8818571 - Flags: review?(bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a73ed8c73a01f41b59dc09f2b6e834521dc1064
Bug 1323449 - implement is() method for testing boolean characteristics, r=smaug
https://hg.mozilla.org/mozilla-central/rev/1a73ed8c73a0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Did the right stuff land here? I mean, wasn't only one of the patches landed?
Flags: needinfo?(surkov.alexander)
(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)
Flags: needinfo?(surkov.alexander)
ah, ok, missed the other one.
You need to log in before you can comment on or make changes to this bug.