Bug 106349 – AX: native popup buttons should not use textUnderElement for their title

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[chris fleizach]

Native pop up buttons (<select>) should not use textUnderElement for their visibleText/title. That's because that would cause the title to be the value of the pop up button. 

We want to communicate that info through stringValue(), not the title

[chris fleizach]

Created attachment 181714 [details]
patch

[Dominic Mazzoni]

Comment on attachment 181714 [details]
patch

Unofficial review.

View in context: https://bugs.webkit.org/attachment.cgi?id=181714&action=review

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1196
> +        if (renderer() && renderer()->isMenuList())

This should work for elements without a renderer (like in canvas fallback content, or in a region of the page that's hidden from sighted users but has aria-hidden=false). Instead of checking renderer()->isMenuList(), check if it's a select element and if it has multiple or not. There's already similar code in this file.

If there are cases where a rendered popup button is different than an unrendered one, you could override it in AccessibilityRenderObject - but I don't think you need to in this case.

[Early Warning System Bot]

Comment on attachment 181714 [details]
patch

Attachment 181714 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15775013

[Early Warning System Bot]

Comment on attachment 181714 [details]
patch

Attachment 181714 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15763182

[chris fleizach]

(In reply to comment #2)
> (From update of attachment 181714 [details])
> Unofficial review.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=181714&action=review
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1196
> > +        if (renderer() && renderer()->isMenuList())
> 
> This should work for elements without a renderer (like in canvas fallback content, or in a region of the page that's hidden from sighted users but has aria-hidden=false). Instead of checking renderer()->isMenuList(), check if it's a select element and if it has multiple or not. There's already similar code in this file.

i think we want this to apply for multiple and non-multiple. what do you think

> 
> If there are cases where a rendered popup button is different than an unrendered one, you could override it in AccessibilityRenderObject - but I don't think you need to in this case.

[Dominic Mazzoni]

(In reply to comment #5)
> > This should work for elements without a renderer (like in canvas fallback content, or in a region of the page that's hidden from sighted users but has aria-hidden=false). Instead of checking renderer()->isMenuList(), check if it's a select element and if it has multiple or not. There's already similar code in this file.
> 
> i think we want this to apply for multiple and non-multiple. what do you think

I think you're right, but then I'd like both to be covered by tests.

[chris fleizach]

(In reply to comment #6)
> (In reply to comment #5)
> > > This should work for elements without a renderer (like in canvas fallback content, or in a region of the page that's hidden from sighted users but has aria-hidden=false). Instead of checking renderer()->isMenuList(), check if it's a select element and if it has multiple or not. There's already similar code in this file.
> > 
> > i think we want this to apply for multiple and non-multiple. what do you think
> 
> I think you're right, but then I'd like both to be covered by tests.

It looks like they are

<select id="combobox1"><option>1<option selected>2</select>
  <select multiple id="listbox1"><option>1<option selected>2</select>

[chris fleizach]

Created attachment 181715 [details]
patch

[Dominic Mazzoni]

Looks fine to me.

[chris fleizach]

http://trac.webkit.org/changeset/139091

[Dimitri Glazkov (Google)]

(In reply to comment #10)
> http://trac.webkit.org/changeset/139091

The test is still failing across Chromium. What gives?

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=accessibility%2Fcanvas-fallback-content-2.html

[Ryosuke Niwa]

(In reply to comment #11)
> (In reply to comment #10)
> > http://trac.webkit.org/changeset/139091
> 
> The test is still failing across Chromium. What gives?
> 
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=accessibility%2Fcanvas-fallback-content-2.html

Odd. Maybe Dominic would know why?

[noel gordon]

The failure is due to changes in ax title rendering (aka this bug and related).

combobox1
PASS document.activeElement == element1 is true1
combobox2
PASS document.activeElement == element2 is true
PASS axElement2.role is axElement1.role
PASS axElement2.roleDescription is axElement1.roleDescription
FAIL axElement2.title should be AXTitle: 2. Was AXTitle: .
PASS axElement2.description is axElement1.description
PASS axElement2.helpText is axElement1.helpText
PASS axElement2.stringValue is axElement1.stringValue
PASS axElement2.isEnabled is axElement1.isEnabled
PASS axElement2.isRequired is axElement1.isRequired
PASS axElement2.isChecked is axElement1.isChecked
PASS axElement2.intValue is axElement1.intValue
PASS axElement2.minValue is axElement1.minValue
PASS axElement2.maxValue is axElement1.maxValue

[noel gordon]

And the green cr-linux bubble was trying to warn you about it.

Regressions: Unexpected text-only failures (1)
  accessibility/canvas-fallback-content-2.html [ Failure ]

[chris fleizach]

Created attachment 181867 [details]
patch for chromium layout test fix

[noel gordon]

lgtm: accessibility/canvas-fallback-content-2.html passes for me on chrome mac with this change.

[chris fleizach]

Chromium fix
http://trac.webkit.org/changeset/139195