Bug 194754 - Web Inspector: AXI: Audit: image label test is throwing spurious errors on elements with existing alt attr, but no value: <img alt>
Summary: Web Inspector: AXI: Audit: image label test is throwing spurious errors on el...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 198773
Blocks:
  Show dependency treegraph
 
Reported: 2019-02-17 00:26 PST by James Craig
Modified: 2019-06-24 08:03 PDT (History)
15 users (show)

See Also:


Attachments
Patch (8.46 KB, patch)
2019-05-30 18:02 PDT, gr3g
no flags Details | Formatted Diff | Diff
Patch (8.03 KB, patch)
2019-06-10 18:11 PDT, gr3g
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews211 for win-future (14.20 MB, application/zip)
2019-06-11 02:28 PDT, Build Bot
no flags Details
Patch (7.68 KB, patch)
2019-06-13 17:43 PDT, gr3g
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (3.28 MB, application/zip)
2019-06-13 18:24 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (3.65 MB, application/zip)
2019-06-13 18:33 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (3.06 MB, application/zip)
2019-06-13 19:28 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews214 for win-future (13.68 MB, application/zip)
2019-06-13 19:39 PDT, Build Bot
no flags Details
Patch (7.71 KB, patch)
2019-06-13 21:31 PDT, gr3g
no flags Details | Formatted Diff | Diff
Patch (6.34 KB, patch)
2019-06-21 09:35 PDT, gr3g
no flags Details | Formatted Diff | Diff
Patch (6.82 KB, patch)
2019-06-23 20:44 PDT, gr3g
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Craig 2019-02-17 00:26:31 PST
Web Inspector: AXI: Audit: image label test is throwing spurious errors on elements with existing alt attr, but no value: <img alt>

<img alt> should be equivalent to <img alt=""> so we shouldn't be throwing an error. This is an explicitly defined decorative image (and valid HTML), so this should not even count as one of the images in the test. 

It's possible this is a WebCore bug (<img alt> should not be returning img role) or perhaps another bug limited to the Audit code.
Comment 1 James Craig 2019-02-17 00:53:00 PST
<rdar://problem/48144534>
Comment 2 gr3g 2019-05-30 18:02:38 PDT
Created attachment 371006 [details]
Patch
Comment 3 chris fleizach 2019-05-31 10:40:57 PDT
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;
Comment 4 Devin Rousso 2019-06-02 15:43:52 PDT
Should this be in a different component?  The only relation it has to Web Inspector is that one of the audits discovered the problem.
Comment 5 gr3g 2019-06-03 07:37:15 PDT
(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.
Comment 6 James Craig 2019-06-03 13:22:09 PDT
I've moved the problem to the Accessibility component.
Comment 7 gr3g 2019-06-10 09:53:50 PDT
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)
Comment 8 chris fleizach 2019-06-10 10:20:48 PDT
(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
Comment 9 gr3g 2019-06-10 18:11:54 PDT
Created attachment 371801 [details]
Patch
Comment 10 Build Bot 2019-06-11 02:28:41 PDT
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
Comment 11 Build Bot 2019-06-11 02:28:43 PDT
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 12 WebKit Commit Bot 2019-06-11 09:42:44 PDT
Comment on attachment 371801 [details]
Patch

Clearing flags on attachment: 371801

Committed r246320: <https://trac.webkit.org/changeset/246320>
Comment 13 WebKit Commit Bot 2019-06-11 09:42:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Shawn Roberts 2019-06-11 10:56:30 PDT
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
Comment 15 WebKit Commit Bot 2019-06-11 16:37:26 PDT
Re-opened since this is blocked by bug 198773
Comment 16 Shawn Roberts 2019-06-11 16:42:14 PDT
Rolled out in https://trac.webkit.org/changeset/246340/webkit
Comment 17 gr3g 2019-06-13 17:43:04 PDT
Created attachment 372088 [details]
Patch
Comment 18 Build Bot 2019-06-13 18:24:05 PDT
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
Comment 19 Build Bot 2019-06-13 18:24:06 PDT
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 20 Build Bot 2019-06-13 18:33:51 PDT
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
Comment 21 Build Bot 2019-06-13 18:33:53 PDT
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 22 Build Bot 2019-06-13 19:28:23 PDT
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
Comment 23 Build Bot 2019-06-13 19:28:25 PDT
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 24 Build Bot 2019-06-13 19:39:23 PDT
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
Comment 25 Build Bot 2019-06-13 19:39:26 PDT
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
Comment 26 gr3g 2019-06-13 21:31:22 PDT
Created attachment 372103 [details]
Patch
Comment 27 WebKit Commit Bot 2019-06-20 15:06:58 PDT
Comment on attachment 372103 [details]
Patch

Clearing flags on attachment: 372103

Committed r246655: <https://trac.webkit.org/changeset/246655>
Comment 28 WebKit Commit Bot 2019-06-20 15:07:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Truitt Savell 2019-06-21 08:00:32 PDT
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
Comment 30 chris fleizach 2019-06-21 09:29:11 PDT
re-open to fix test
Comment 31 gr3g 2019-06-21 09:35:14 PDT
Created attachment 372631 [details]
Patch
Comment 32 Truitt Savell 2019-06-21 10:45:05 PDT
Reverted r246655 for reason:

Introduced a failing test

Committed r246683: <https://trac.webkit.org/changeset/246683>
Comment 33 gr3g 2019-06-23 20:44:56 PDT
Created attachment 372729 [details]
Patch
Comment 34 WebKit Commit Bot 2019-06-24 08:03:52 PDT
Comment on attachment 372729 [details]
Patch

Clearing flags on attachment: 372729

Committed r246736: <https://trac.webkit.org/changeset/246736>
Comment 35 WebKit Commit Bot 2019-06-24 08:03:54 PDT
All reviewed patches have been landed.  Closing bug.