Bug 160155 - AX: AccessibilityRenderObject is adding duplicated children when CSS first-letter is being used.
Summary: AX: AccessibilityRenderObject is adding duplicated children when CSS first-le...
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: 2016-07-24 22:57 PDT by Nan Wang
Modified: 2016-07-25 12:16 PDT (History)
11 users (show)

See Also:


Attachments
patch (4.78 KB, patch)
2016-07-24 23:06 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (4.69 KB, patch)
2016-07-24 23:56 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (4.67 KB, patch)
2016-07-25 00:24 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
Patch (4.89 KB, patch)
2016-07-25 11:10 PDT, Nan Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nan Wang 2016-07-24 22:57:05 PDT
Inspect the page: http://www.w3schools.com/cssref/tryit.asp?filename=trycss_sel_firstletter
We can see each paragraph has two accessibility children, both are the same text.

<rdar://problem/27220338>
Comment 1 Radar WebKit Bug Importer 2016-07-24 22:57:21 PDT
<rdar://problem/27519396>
Comment 2 Nan Wang 2016-07-24 23:06:17 PDT
Created attachment 284463 [details]
patch
Comment 3 chris fleizach 2016-07-24 23:08:59 PDT
Comment on attachment 284463 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3144
> +        if (obj->renderer() && obj->renderer()->isInlineElementContinuation())

shouldn't nextSibling take care of this case?
Comment 4 Nan Wang 2016-07-24 23:44:19 PDT
(In reply to comment #3)
> Comment on attachment 284463 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284463&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3144
> > +        if (obj->renderer() && obj->renderer()->isInlineElementContinuation())
> 
> shouldn't nextSibling take care of this case?

I think this case is happened in the firstChild() iteration.
Comment 5 Nan Wang 2016-07-24 23:56:36 PDT
Created attachment 284464 [details]
patch

Moved the check to firstChild()
Comment 6 chris fleizach 2016-07-25 00:02:48 PDT
Comment on attachment 284464 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:226
> +    // Sometimes with CSS first-letter selector, we were adding the same text node twice.

sorry to be a jerk, but this seems like it should be in firstChildConsideringContinuation shouldn't it

also if firstChild() returns null, how will nextSibling() be called
Comment 7 Nan Wang 2016-07-25 00:24:01 PDT
(In reply to comment #6)
> Comment on attachment 284464 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284464&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:226
> > +    // Sometimes with CSS first-letter selector, we were adding the same text node twice.
> 
> sorry to be a jerk, but this seems like it should be in
> firstChildConsideringContinuation shouldn't it
> 
> also if firstChild() returns null, how will nextSibling() be called

I think in this case we don't want to add children for the inline element continuation. The real node will be handled in its parent level.
Comment 8 Nan Wang 2016-07-25 00:24:32 PDT
Created attachment 284467 [details]
patch

review comments
Comment 9 Nan Wang 2016-07-25 00:29:02 PDT
(In reply to comment #8)
> Created attachment 284467 [details]
> patch
> 
> review comments

Actually I'm not sure if putting this in firstChild() would affect other stuff or not, it passed all the test on my machine. But if we put the check in addChildren() we know for sure that this is the case we don't want to add it as a child.
Comment 10 chris fleizach 2016-07-25 09:10:42 PDT
Comment on attachment 284467 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:189
> +    // Sometimes with CSS first-letter selector, we were adding the same text node twice.

comment is worded a bit strangely

Can you change it to say why we don't want to process an inlineElementCont() as the firstChild (the CSS first letter would be noted as an example of that case)
Comment 11 Nan Wang 2016-07-25 11:10:07 PDT
Created attachment 284502 [details]
Patch

Updated the comment and added an anonymous check so that we won't affect other cases.
Comment 12 chris fleizach 2016-07-25 11:15:35 PDT
Comment on attachment 284502 [details]
Patch

Thanks. Much better
Comment 13 chris fleizach 2016-07-25 11:16:03 PDT
(In reply to comment #12)
> Comment on attachment 284502 [details]
> Patch
> 
> Thanks. Much better

R+ assuming all tests pass
Comment 14 Nan Wang 2016-07-25 11:17:52 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > Comment on attachment 284502 [details]
> > Patch
> > 
> > Thanks. Much better
> 
> R+ assuming all tests pass

Thanks will commit when all tests have passed.
Comment 15 WebKit Commit Bot 2016-07-25 12:16:10 PDT
Comment on attachment 284502 [details]
Patch

Clearing flags on attachment: 284502

Committed r203694: <http://trac.webkit.org/changeset/203694>
Comment 16 WebKit Commit Bot 2016-07-25 12:16:17 PDT
All reviewed patches have been landed.  Closing bug.