Bug 179068 – AX: search predicate returns containing group for plain text instead of text element

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Doug Russell]

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.

[Radar WebKit Bug Importer]

<rdar://problem/35278072>

[Doug Russell]

Created attachment 325472 [details]
patch

[chris fleizach]

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

[Doug Russell]

(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

[chris fleizach]

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

[Doug Russell]

> 
> 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

[Doug Russell]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[chris fleizach]

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?

[Doug Russell]

(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.

[Doug Russell]

Created attachment 326217 [details]
patch

fix indentation

[Darin Adler]

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.

[Doug Russell]

Created attachment 326494 [details]
patch

[WebKit Commit Bot]

Comment on attachment 326494 [details]
patch

Clearing flags on attachment: 326494

Committed r224650: <https://trac.webkit.org/changeset/224650>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.