Summary: | Web Inspector: AXI: Audit: image label test is throwing spurious errors on elements with existing alt attr, but no value: <img alt> | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Craig <jcraig> | ||||||||||||||||||||||||
Component: | Accessibility | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, ews-watchlist, gr3g, hi, inspector-bugzilla-changes, jdiggs, rniwa, samuel_white, sroberts, tsavell, webkit-bug-importer | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||
Bug Depends on: | 198773 | ||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||
Attachments: |
|
Description
James Craig
2019-02-17 00:26:31 PST
Created attachment 371006 [details]
Patch
Comment on attachment 371006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371006&action=review > Source/WebCore/accessibility/AccessibilityObject.cpp:2527 > + if (alt.isEmpty()) in this case the element is ignored completely and shouldn't even be in the hierarchy here's the code that does this bool isRenderImage = is<RenderImage>(renderer()); if (isRenderImage) altTextInclusion = objectInclusionFromAltText(downcast<RenderImage>(*m_renderer).altText()); else altTextInclusion = objectInclusionFromAltText(getAttribute(altAttr).string()); if (altTextInclusion == AccessibilityObjectInclusion::IgnoreObject) return true; if (altTextInclusion == AccessibilityObjectInclusion::IncludeObject) return false; Should this be in a different component? The only relation it has to Web Inspector is that one of the audits discovered the problem. (In reply to Devin Rousso from comment #4) > Should this be in a different component? The only relation it has to Web > Inspector is that one of the audits discovered the problem. In Radar, it's already been moved to WebKit Accessibility. I've moved the problem to the Accessibility component. Comment on attachment 371006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371006&action=review >> Source/WebCore/accessibility/AccessibilityObject.cpp:2527 >> + if (alt.isEmpty()) > > in this case the element is ignored completely and shouldn't even be in the hierarchy > > here's the code that does this > > bool isRenderImage = is<RenderImage>(renderer()); > if (isRenderImage) > altTextInclusion = objectInclusionFromAltText(downcast<RenderImage>(*m_renderer).altText()); > else > altTextInclusion = objectInclusionFromAltText(getAttribute(altAttr).string()); > > if (altTextInclusion == AccessibilityObjectInclusion::IgnoreObject) > return true; > if (altTextInclusion == AccessibilityObjectInclusion::IncludeObject) > return false; the accessibility audit isn't using the ignore/include logic you've referenced (from `AccessibilityRenderObject::defaultObjectInclusion`) for its roles, and instead it uses its own function to determine the role (`AccessibilityObject::computedRoleString`). my change fixes the computed role for these scenarios: - `<img alt src="...">` (unassigned alt -> role is presentational) - `<img src="...">` (missing alt -> role is image) (In reply to gr3g from comment #7) > Comment on attachment 371006 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371006&action=review > > >> Source/WebCore/accessibility/AccessibilityObject.cpp:2527 > >> + if (alt.isEmpty()) > > > > in this case the element is ignored completely and shouldn't even be in the hierarchy > > > > here's the code that does this > > > > bool isRenderImage = is<RenderImage>(renderer()); > > if (isRenderImage) > > altTextInclusion = objectInclusionFromAltText(downcast<RenderImage>(*m_renderer).altText()); > > else > > altTextInclusion = objectInclusionFromAltText(getAttribute(altAttr).string()); > > > > if (altTextInclusion == AccessibilityObjectInclusion::IgnoreObject) > > return true; > > if (altTextInclusion == AccessibilityObjectInclusion::IncludeObject) > > return false; > > the accessibility audit isn't using the ignore/include logic you've > referenced (from `AccessibilityRenderObject::defaultObjectInclusion`) for > its roles, and instead it uses its own function to determine the role > (`AccessibilityObject::computedRoleString`). > > my change fixes the computed role for these scenarios: > - `<img alt src="...">` (unassigned alt -> role is presentational) > - `<img src="...">` (missing alt -> role is image) That seems like its giving bad info to the developer then, because they may think the element is accessible, when in reality it is hidden from users Created attachment 371801 [details]
Patch
Comment on attachment 371801 [details] Patch Attachment 371801 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12441591 New failing tests: fast/shadow-dom/svg-text-path-href-change-in-shadow-tree.html fast/shadow-dom/svg-use-href-change-in-shadow-tree.html Created attachment 371834 [details]
Archive of layout-test-results from ews211 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 371801 [details] Patch Clearing flags on attachment: 371801 Committed r246320: <https://trac.webkit.org/changeset/246320> All reviewed patches have been landed. Closing bug. It appears that after changes in r246320 The new layout test added : accessibility/img-alt-attribute-no-value.html Also this layout test : inspector/dom/getAccessibilityPropertiesForNode.html Are failing on Mojave Release, and High Sierra Release WK1 Verified locally with : run-webkit-tests accessibility/img-alt-attribute-no-value.html --iter 25 -f Locally I could only get it to fail flakily, but the inspector test fails 100% locally with r246320 and passes in prior revisions. Diffs: --- /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/accessibility/img-alt-attribute-no-value-expected.txt +++ /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/accessibility/img-alt-attribute-no-value-actual.txt @@ -7,9 +7,9 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". -PASS imagesGroup.childrenCount is 2 +FAIL imagesGroup.childrenCount should be 2. Was 3. PASS platformValueForW3CName(imagesGroup.childAtIndex(0)) is "cake0" -PASS platformValueForW3CName(imagesGroup.childAtIndex(1)) is "cake3" +FAIL platformValueForW3CName(imagesGroup.childAtIndex(1)) should be cake3. Was . PASS successfullyParsed is true TEST COMPLETE --- /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/inspector/dom/getAccessibilityPropertiesForNode-expected.txt +++ /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/inspector/dom/getAccessibilityPropertiesForNode-actual.txt @@ -326,7 +326,7 @@ <img src="data:image/gif..." alt="x" aria-hidden="true"> exists: true label: x - role: img + role: presentation ignored: true ignoredByDefault: true hidden: true @@ -339,18 +339,18 @@ <img src="data:image/gif..." alt=""> exists: true label: + role: presentation + ignored: true + +<img src="data:image/gif..."> + exists: true + label: role: img - ignored: true - -<img src="data:image/gif..."> - exists: true - label: - role: img <img src="./404.gif"> exists: true label: - role: img + role: presentation ignored: true <input style="display:none;"> @@ -361,7 +361,7 @@ <input aria-hidden="true"> exists: true label: - role: + role: presentation focused: false ignored: true ignoredByDefault: true @@ -906,7 +906,7 @@ <span aria-hidden="true"></span> exists: true label: - role: + role: presentation ignored: true ignoredByDefault: true hidden: true @@ -915,7 +915,7 @@ <span></span> exists: true label: - role: + role: presentation ignored: true ignoredByDefault: true parentNodeId: exists @@ -923,7 +923,7 @@ <div aria-hidden="true"></div> exists: true label: - role: + role: presentation ignored: true ignoredByDefault: true hidden: true @@ -932,7 +932,7 @@ <div></div> exists: true label: - role: + role: presentation ignored: true parentNodeId: exists @@ -961,14 +961,14 @@ <div><div></div></div> exists: true label: - role: + role: presentation ignored: true parentNodeId: exists <script style="display:block;"></script> exists: true label: - role: + role: presentation ignored: true parentNodeId: exists Re-opened since this is blocked by bug 198773 Rolled out in https://trac.webkit.org/changeset/246340/webkit Created attachment 372088 [details]
Patch
Comment on attachment 372088 [details] Patch Attachment 372088 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12469465 New failing tests: accessibility/img-alt-attribute-unassigned-value.html Created attachment 372090 [details]
Archive of layout-test-results from ews102 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 372088 [details] Patch Attachment 372088 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12469481 New failing tests: accessibility/img-alt-attribute-unassigned-value.html Created attachment 372091 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 372088 [details] Patch Attachment 372088 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12469675 New failing tests: accessibility/img-alt-attribute-unassigned-value.html Created attachment 372098 [details]
Archive of layout-test-results from ews114 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 372088 [details] Patch Attachment 372088 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12469883 New failing tests: accessibility/img-alt-attribute-unassigned-value.html Created attachment 372100 [details]
Archive of layout-test-results from ews214 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 372103 [details]
Patch
Comment on attachment 372103 [details] Patch Clearing flags on attachment: 372103 Committed r246655: <https://trac.webkit.org/changeset/246655> All reviewed patches have been landed. Closing bug. It looks like the changes in https://trac.webkit.org/changeset/246655/webkit has broken inspector/dom/getAccessibilityPropertiesForNode.html again I was able to reproduce this locally History: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fdom%2FgetAccessibilityPropertiesForNode.html Diff: --- /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/inspector/dom/getAccessibilityPropertiesForNode-expected.txt +++ /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/inspector/dom/getAccessibilityPropertiesForNode-actual.txt @@ -361,7 +361,7 @@ <input aria-hidden="true"> exists: true label: - role: presentation + role: focused: false ignored: true ignoredByDefault: true @@ -906,7 +906,7 @@ <span aria-hidden="true"></span> exists: true label: - role: presentation + role: ignored: true ignoredByDefault: true hidden: true @@ -915,7 +915,7 @@ <span></span> exists: true label: - role: presentation + role: ignored: true ignoredByDefault: true parentNodeId: exists @@ -923,7 +923,7 @@ <div aria-hidden="true"></div> exists: true label: - role: presentation + role: ignored: true ignoredByDefault: true hidden: true @@ -932,7 +932,7 @@ <div></div> exists: true label: - role: presentation + role: ignored: true parentNodeId: exists @@ -961,14 +961,14 @@ <div><div></div></div> exists: true label: - role: presentation + role: ignored: true parentNodeId: exists <script style="display:block;"></script> exists: true label: - role: presentation + role: ignored: true parentNodeId: exists re-open to fix test Created attachment 372631 [details]
Patch
Reverted r246655 for reason: Introduced a failing test Committed r246683: <https://trac.webkit.org/changeset/246683> Created attachment 372729 [details]
Patch
Comment on attachment 372729 [details] Patch Clearing flags on attachment: 372729 Committed r246736: <https://trac.webkit.org/changeset/246736> All reviewed patches have been landed. Closing bug. |