Bug 106349

Summary: AX: native popup buttons should not use textUnderElement for their title
Product: WebKit Reporter: chris fleizach <cfleizach>
Component: AccessibilityAssignee: chris fleizach <cfleizach>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, dglazkov, dmazzoni, jdiggs, noel.gordon, rniwa, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
webkit-ews: commit-queue-
patch
rniwa: review+
patch for chromium layout test fix rniwa: review+

Description chris fleizach 2013-01-08 10:59:35 PST
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
Comment 1 chris fleizach 2013-01-08 11:17:41 PST
Created attachment 181714 [details]
patch
Comment 2 Dominic Mazzoni 2013-01-08 11:23:35 PST
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.
Comment 3 Early Warning System Bot 2013-01-08 11:24:26 PST
Comment on attachment 181714 [details]
patch

Attachment 181714 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15775013
Comment 4 Early Warning System Bot 2013-01-08 11:26:28 PST
Comment on attachment 181714 [details]
patch

Attachment 181714 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15763182
Comment 5 chris fleizach 2013-01-08 11:26:43 PST
(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.
Comment 6 Dominic Mazzoni 2013-01-08 11:30:35 PST
(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.
Comment 7 chris fleizach 2013-01-08 11:32:32 PST
(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>
Comment 8 chris fleizach 2013-01-08 11:33:47 PST
Created attachment 181715 [details]
patch
Comment 9 Dominic Mazzoni 2013-01-08 11:36:28 PST
Looks fine to me.
Comment 10 chris fleizach 2013-01-08 11:54:25 PST
http://trac.webkit.org/changeset/139091
Comment 12 Ryosuke Niwa 2013-01-08 14:26:37 PST
(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?
Comment 13 noel gordon 2013-01-08 18:44:42 PST
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
Comment 14 noel gordon 2013-01-08 18:45:51 PST
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 ]
Comment 15 chris fleizach 2013-01-09 00:18:53 PST
Created attachment 181867 [details]
patch for chromium layout test fix
Comment 16 noel gordon 2013-01-09 01:07:21 PST
lgtm: accessibility/canvas-fallback-content-2.html passes for me on chrome mac with this change.
Comment 17 chris fleizach 2013-01-09 07:57:36 PST
Chromium fix
http://trac.webkit.org/changeset/139195