Bug 179068 - AX: search predicate returns containing group for plain text instead of text element
Summary: AX: search predicate returns containing group for plain text instead of text ...
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:
Blocks:
 
Reported: 2017-10-31 12:30 PDT by Doug Russell
Modified: 2017-11-09 15:35 PST (History)
12 users (show)

See Also:


Attachments
patch (14.53 KB, patch)
2017-10-31 12:37 PDT, Doug Russell
no flags Details | Formatted Diff | Diff
patch (31.50 KB, patch)
2017-11-02 11:13 PDT, Doug Russell
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.34 MB, application/zip)
2017-11-02 12:16 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (1002.82 KB, application/zip)
2017-11-02 15:38 PDT, Build Bot
no flags Details
patch (31.51 KB, patch)
2017-11-07 09:47 PST, Doug Russell
darin: review+
Details | Formatted Diff | Diff
patch (34.21 KB, patch)
2017-11-09 15:03 PST, Doug Russell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Doug Russell 2017-10-31 12:30:58 PDT
When querying for plain text via search predicate API, the check for hasPlainText only consults style information, not if the element can actually host text and as a result can return the container element instead of the actual text element.
Comment 1 Radar WebKit Bug Importer 2017-10-31 12:31:32 PDT
<rdar://problem/35278072>
Comment 2 Doug Russell 2017-10-31 12:37:02 PDT
Created attachment 325472 [details]
patch
Comment 3 chris fleizach 2017-10-31 12:53:54 PDT
Comment on attachment 325472 [details]
patch

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

> LayoutTests/accessibility/mac/search-predicate-plaintext.html:12
> +<p style="color:blue; font-family:serif; font-style:italic;">serif blue italic text</p>

we need to add more cases that would have been found before but now fail to find

> Source/WebCore/accessibility/AccessibilityLabel.h:48
> +    bool canHaveStringValue() const final;

this should return true inline here

> Source/WebCore/accessibility/AccessibilityListBoxOption.cpp:169
> +    if (is<HTMLOptGroupElement>(*m_optionElement))

do these really count as plain text? they're more like controls

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1906
> +    if (ariaRoleAttribute() == AccessibilityRole::StaticText)

which elements are hitting AccessibilityNodeObject instead of AXRenderObject?

> Source/WebCore/accessibility/AccessibilityObject.cpp:221
> +        if (axObject->hasPlainText())

this seems unnecessary

> Source/WebCore/accessibility/AccessibilityObject.h:-594
> -    

undo

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:810
> +    if (is<RenderMenuList>(cssBox))

are menu lists really considered plain text?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:816
> +    if (isWebArea())

we shouldn't have false cases. that should just be the default

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:819
> +    if (isTextControl())

do you have the content editable and role=textbox case covered
Comment 4 Doug Russell 2017-10-31 13:16:39 PDT
(In reply to chris fleizach from comment #3)
> Comment on attachment 325472 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325472&action=review
> 
> > LayoutTests/accessibility/mac/search-predicate-plaintext.html:12
> > +<p style="color:blue; font-family:serif; font-style:italic;">serif blue italic text</p>
> 
> we need to add more cases that would have been found before but now fail to
> find

I have at least one where it would find the group around the second block quote instead of text in it

> 
> > Source/WebCore/accessibility/AccessibilityLabel.h:48
> > +    bool canHaveStringValue() const final;
> 
> this should return true inline here

on it

> 
> > Source/WebCore/accessibility/AccessibilityListBoxOption.cpp:169
> > +    if (is<HTMLOptGroupElement>(*m_optionElement))
> 
> do these really count as plain text? they're more like controls

this check is if it can yield text, not if it's plain text

> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1906
> > +    if (ariaRoleAttribute() == AccessibilityRole::StaticText)
> 
> which elements are hitting AccessibilityNodeObject instead of AXRenderObject?

not sure, but I wanted the implementation to be consistent, I'll see if I can find concrete examples

> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:221
> > +        if (axObject->hasPlainText())
> 
> this seems unnecessary

left over debug code, I'll remove it

> 
> > Source/WebCore/accessibility/AccessibilityObject.h:-594
> > -    
> 
> undo

on it

> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:810
> > +    if (is<RenderMenuList>(cssBox))
> 
> are menu lists really considered plain text?

this is only a check if it can yield text, not if it's plain text

> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:816
> > +    if (isWebArea())
> 
> we shouldn't have false cases. that should just be the default

good point

> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:819
> > +    if (isTextControl())
> 
> do you have the content editable and role=textbox case covered


the isTextControl check covers both of those as AccessibilityRole::TextArea
Comment 5 chris fleizach 2017-10-31 13:28:13 PDT
Comment on attachment 325472 [details]
patch

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

>>> LayoutTests/accessibility/mac/search-predicate-plaintext.html:12
>>> +<p style="color:blue; font-family:serif; font-style:italic;">serif blue italic text</p>
>> 
>> we need to add more cases that would have been found before but now fail to find
> 
> I have at least one where it would find the group around the second block quote instead of text in it

we should add other cases that are identified as having plainText now
Comment 6 Doug Russell 2017-10-31 13:29:50 PDT
> 
> we should add other cases that are identified as having plainText now

there's a fairly comprehensive search-predicate.html test that seems to be disabled, I'll see if I can fix that
Comment 7 Doug Russell 2017-11-02 11:13:23 PDT
Created attachment 325736 [details]
patch

Narrowed the scope to only effect AccessibilityRenderObject. We can easily expand it if we find concrete examples not covered by render object. Broke the disabled search-predicate test into three tests to allow the bulk of the test run and still have documentation of the two failures that were in the test.
Comment 8 Build Bot 2017-11-02 12:16:53 PDT
Comment on attachment 325736 [details]
patch

Attachment 325736 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5078431

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-canvas-tainting.https.html
Comment 9 Build Bot 2017-11-02 12:16:54 PDT
Created attachment 325743 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 10 Build Bot 2017-11-02 15:38:48 PDT
Comment on attachment 325736 [details]
patch

Attachment 325736 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5080307

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-canvas-tainting.https.html
Comment 11 Build Bot 2017-11-02 15:38:49 PDT
Created attachment 325783 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 12 chris fleizach 2017-11-06 09:59:08 PST
Comment on attachment 325736 [details]
patch

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

> Source/WebCore/ChangeLog:3
> +Bug 179068 - AX: search predicate returns containing group for plain text instead of text element

wrong indentation

> Source/WebCore/ChangeLog:12
> +               accessibility/mac/search-predicate-visited-links.html

are these enabling new tests? or did you split up old tests? 
why is the new visited links tests added to the failure case?
Comment 13 Doug Russell 2017-11-07 09:46:03 PST
(In reply to chris fleizach from comment #12)
> Comment on attachment 325736 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325736&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +Bug 179068 - AX: search predicate returns containing group for plain text instead of text element
> 
> wrong indentation
> 
> > Source/WebCore/ChangeLog:12
> > +               accessibility/mac/search-predicate-visited-links.html
> 
> are these enabling new tests? or did you split up old tests? 

I broke out the two parts of the monolithic search predicate tests that were broken so I could re-enable the bulk of the test.

> why is the new visited links tests added to the failure case?

it replaces the previously disabled monolithic test which now passes.
Comment 14 Doug Russell 2017-11-07 09:47:56 PST
Created attachment 326217 [details]
patch

fix indentation
Comment 15 Darin Adler 2017-11-09 09:37:04 PST
Comment on attachment 326217 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:806
> +    if (ariaRoleAttribute() == AccessibilityRole::StaticText)
> +        return true;
> +
> +    if (is<RenderText>(*m_renderer))
> +        return true;
> +
> +    if (isTextControl())
> +        return true;

It might make the line a little long, but I think a horizontal version of this function that uses || might be clearer than a string of if statements where it’s hard to spot which are "return true" and which are "return false". Something roughly like this:

    return isARIAStaticText() || is<RenderText>(m_renderer) || isTextControl();

Needs a little rearranging and better helper functions, but something to think about when writing future code.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3567
>      if (!m_renderer)
>          return false;
> -    
> +
> +    if (!canHavePlainText())
> +        return false;

No need to check m_renderer for null in both hasPlainText and canHavePlainText.
Comment 16 Doug Russell 2017-11-09 15:03:31 PST
Created attachment 326494 [details]
patch
Comment 17 WebKit Commit Bot 2017-11-09 15:35:38 PST
Comment on attachment 326494 [details]
patch

Clearing flags on attachment: 326494

Committed r224650: <https://trac.webkit.org/changeset/224650>
Comment 18 WebKit Commit Bot 2017-11-09 15:35:40 PST
All reviewed patches have been landed.  Closing bug.