Summary: | AX: native popup buttons should not use textUnderElement for their title | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||||||
Component: | Accessibility | Assignee: | 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
chris fleizach
2013-01-08 10:59:35 PST
Created attachment 181714 [details]
patch
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 on attachment 181714 [details] patch Attachment 181714 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15775013 Comment on attachment 181714 [details] patch Attachment 181714 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15763182 (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. (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. (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> Created attachment 181715 [details]
patch
Looks fine to me. (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 (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? 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 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 ] Created attachment 181867 [details]
patch for chromium layout test fix
lgtm: accessibility/canvas-fallback-content-2.html passes for me on chrome mac with this change. Chromium fix http://trac.webkit.org/changeset/139195 |