Bug 160155 – AX: AccessibilityRenderObject is adding duplicated children when CSS first-letter is being used.

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

[Nan Wang]

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>

[Radar WebKit Bug Importer]

<rdar://problem/27519396>

[Nan Wang]

Created attachment 284463 [details]
patch

[chris fleizach]

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?

[Nan Wang]

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

[Nan Wang]

Created attachment 284464 [details]
patch

Moved the check to firstChild()

[chris fleizach]

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

[Nan Wang]

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

[Nan Wang]

Created attachment 284467 [details]
patch

review comments

[Nan Wang]

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

[chris fleizach]

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)

[Nan Wang]

Created attachment 284502 [details]
Patch

Updated the comment and added an anonymous check so that we won't affect other cases.

[chris fleizach]

Comment on attachment 284502 [details]
Patch

Thanks. Much better

[chris fleizach]

(In reply to comment #12)
> Comment on attachment 284502 [details]
> Patch
> 
> Thanks. Much better

R+ assuming all tests pass

[Nan Wang]

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

[WebKit Commit Bot]

Comment on attachment 284502 [details]
Patch

Clearing flags on attachment: 284502

Committed r203694: <http://trac.webkit.org/changeset/203694>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.