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.
<rdar://problem/35278072>
Created attachment 325472 [details] patch
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
(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 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
> > 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
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 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
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 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
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 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?
(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.
Created attachment 326217 [details] patch fix indentation
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.
Created attachment 326494 [details] patch
Comment on attachment 326494 [details] patch Clearing flags on attachment: 326494 Committed r224650: <https://trac.webkit.org/changeset/224650>
All reviewed patches have been landed. Closing bug.