Bug 183661 - AX: AccessibilityNodeObject::textForLabelElement() doesn't follow AccName calculation rules
Summary: AX: AccessibilityNodeObject::textForLabelElement() doesn't follow AccName cal...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joanmarie Diggs
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-15 08:51 PDT by Joanmarie Diggs
Modified: 2018-03-16 14:01 PDT (History)
9 users (show)

See Also:


Attachments
Patch (6.38 KB, patch)
2018-03-15 08:56 PDT, Joanmarie Diggs
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.07 MB, application/zip)
2018-03-15 10:39 PDT, EWS Watchlist
no flags Details
Patch (16.70 KB, patch)
2018-03-16 07:46 PDT, Joanmarie Diggs
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.11 MB, application/zip)
2018-03-16 09:29 PDT, EWS Watchlist
no flags Details
patch including results for 'win-future' (17.74 KB, patch)
2018-03-16 09:38 PDT, Joanmarie Diggs
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.08 MB, application/zip)
2018-03-16 11:16 PDT, EWS Watchlist
no flags Details
patch with results for 'win' rather than 'win-future' (17.71 KB, patch)
2018-03-16 11:41 PDT, Joanmarie Diggs
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs 2018-03-15 08:51:26 PDT
AccessibilityNodeObject::textForLabelElement() doesn't include pseudo-element text. For platforms where accessible objects get their name from the associated label element, not including pseudo-element text means part of the name is missing.
Comment 1 Radar WebKit Bug Importer 2018-03-15 08:51:41 PDT
<rdar://problem/38502201>
Comment 2 Joanmarie Diggs 2018-03-15 08:56:37 PDT
Created attachment 335851 [details]
Patch
Comment 3 chris fleizach 2018-03-15 08:59:08 PDT
Comment on attachment 335851 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=335851&action=review

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1288
> +    if (result.isEmpty() && labelObject && (label->beforePseudoElement() || label->afterPseudoElement()))

do we need to check the pseudo element status? would we not just want to default to textUnderElement() anyway?

is there any possibility that a self referential label would lead to recursion here?
Comment 4 Joanmarie Diggs 2018-03-15 09:00:07 PDT
Chris: Any suggestions for how to test this on your platform? I was thinking the label text would show up as one of the text alternatives (e.g. AXTitle or AXDescription). But as you'll see in the results I generated in macOS, that's not the case.
Comment 5 chris fleizach 2018-03-15 09:02:04 PDT
Comment on attachment 335851 [details]
Patch

it's possible Mac is not using textForLabelElement since we return the list of elements in titleUIElements. possible there is no Mac test here beyond what you generated
Comment 6 EWS Watchlist 2018-03-15 10:39:45 PDT
Comment on attachment 335851 [details]
Patch

Attachment 335851 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6966047

New failing tests:
accessibility/label-with-pseudo-elements.html
Comment 7 EWS Watchlist 2018-03-15 10:39:56 PDT
Created attachment 335860 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 8 Joanmarie Diggs 2018-03-15 11:47:05 PDT
(In reply to chris fleizach from comment #3)
> Comment on attachment 335851 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335851&action=review
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1288
> > +    if (result.isEmpty() && labelObject && (label->beforePseudoElement() || label->afterPseudoElement()))
> 
> do we need to check the pseudo element status? would we not just want to
> default to textUnderElement() anyway?

I took a look at some other AccName failures. The answer is "no" to both of your questions it turns out. For instance, if the label has children then the AccName algorithm descends the children and builds the name from them. So the problem is bigger than pseudo elements and cannot be solved by textUnderElement(). I'll do a new version in a little bit.

> is there any possibility that a self referential label would lead to
> recursion here?

I don't see how. Did you have a specific case in mind? Regardless I can add a self-referential label to the layout test.

Thanks for the review!
Comment 9 Joanmarie Diggs 2018-03-16 07:46:48 PDT
Created attachment 335935 [details]
Patch
Comment 10 Joanmarie Diggs 2018-03-16 09:11:40 PDT
Thanks for the review Chris!

I had to play "guess the Windows results" and I apparently guessed wrong. I'll do a revised patch with whatever the bot claims are the right results for Windows once it finishes running.
Comment 11 EWS Watchlist 2018-03-16 09:28:51 PDT
Comment on attachment 335935 [details]
Patch

Attachment 335935 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6982133

New failing tests:
accessibility/label-with-pseudo-elements.html
Comment 12 EWS Watchlist 2018-03-16 09:29:02 PDT
Created attachment 335942 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 13 Joanmarie Diggs 2018-03-16 09:38:01 PDT
Created attachment 335943 [details]
patch including results for 'win-future'
Comment 14 EWS Watchlist 2018-03-16 11:15:55 PDT
Comment on attachment 335943 [details]
patch including results for 'win-future'

Attachment 335943 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6983725

New failing tests:
accessibility/label-with-pseudo-elements.html
Comment 15 EWS Watchlist 2018-03-16 11:16:07 PDT
Created attachment 335952 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 16 Joanmarie Diggs 2018-03-16 11:41:57 PDT
Created attachment 335957 [details]
patch with results for 'win' rather than 'win-future'
Comment 17 WebKit Commit Bot 2018-03-16 14:01:30 PDT
Comment on attachment 335957 [details]
patch with results for 'win' rather than 'win-future'

Clearing flags on attachment: 335957

Committed r229682: <https://trac.webkit.org/changeset/229682>
Comment 18 WebKit Commit Bot 2018-03-16 14:01:32 PDT
All reviewed patches have been landed.  Closing bug.