Web Inspector: AXI: Audit: image label test is throwing spurious errors on elements with existing alt attr, but no value: <img alt>

[James Craig]

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.

[James Craig]

<rdar://problem/48144534>

Created attachment 371006 [details]
Patch

[chris fleizach]

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;

[Devin Rousso]

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.

[James Craig]

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)

[chris fleizach]

(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

[EWS Watchlist]

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

[EWS Watchlist]

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

[WebKit Commit Bot]

Comment on attachment 371801 [details]
Patch

Clearing flags on attachment: 371801

Committed r246320: <https://trac.webkit.org/changeset/246320>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Shawn Roberts]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 198773

[Shawn Roberts]

Rolled out in https://trac.webkit.org/changeset/246340/webkit

Created attachment 372088 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[WebKit Commit Bot]

Comment on attachment 372103 [details]
Patch

Clearing flags on attachment: 372103

Committed r246655: <https://trac.webkit.org/changeset/246655>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

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

[chris fleizach]

re-open to fix test

Created attachment 372631 [details]
Patch

[Truitt Savell]

Reverted r246655 for reason:

Introduced a failing test

Committed r246683: <https://trac.webkit.org/changeset/246683>

Created attachment 372729 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 372729 [details]
Patch

Clearing flags on attachment: 372729

Committed r246736: <https://trac.webkit.org/changeset/246736>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.