Bug 166748 – :first-child, :last-child, :nth-child, and :nth-of-type don't work on shadow root's children

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Ryosuke Niwa]

Pseudo classes such as :first-child, :last-child, :nth-child, and :nth-of-type don’t work on the immediate children of a shadow root.

[Ryosuke Niwa]

Created attachment 298151 [details]
Reduction

[Ryosuke Niwa]

<rdar://problem/29649177>

[Ryosuke Niwa]

*** Bug 184273 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

Created attachment 349116 [details]
WIP

[EWS Watchlist]

Comment on attachment 349116 [details]
WIP

Attachment 349116 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9124393

New failing tests:
fast/selectors/nth-last-child-cannot-match-during-parsing-1.html
fast/selectors/nth-last-child-of-cannot-match-during-parsing-1.html

[EWS Watchlist]

Created attachment 349124 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 349116 [details]
WIP

Attachment 349116 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9124403

New failing tests:
fast/selectors/nth-last-child-cannot-match-during-parsing-1.html
fast/selectors/nth-last-child-of-cannot-match-during-parsing-1.html

[EWS Watchlist]

Created attachment 349125 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 349116 [details]
WIP

Attachment 349116 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9124456

New failing tests:
fast/selectors/nth-last-child-cannot-match-during-parsing-1.html
fast/selectors/nth-last-child-of-cannot-match-during-parsing-1.html

[EWS Watchlist]

Created attachment 349127 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 349116 [details]
WIP

Attachment 349116 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9124513

New failing tests:
fast/selectors/nth-last-child-cannot-match-during-parsing-1.html
fast/selectors/nth-last-child-of-cannot-match-during-parsing-1.html

[EWS Watchlist]

Created attachment 349128 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Ryosuke Niwa]

Created attachment 349252 [details]
Fixes the bug

[Yusuke Suzuki]

Comment on attachment 349252 [details]
Fixes the bug

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

The approach looks good to me. I have some suggestions to clean up CSS JIT codes.

> Source/WebCore/ChangeLog:3
> +        :first-child, :last-child, :nth-child, and :nth-of-type don't work on shadow rootâs children

Let's use ascii `'` for `root's`.

> Source/WebCore/css/SelectorChecker.cpp:747
> +            if (!isFirstChild)

Why not performing isFirstChild here?

if (!isFirstChildElement(element))
    break;


Ah, I see. The implementation is aligned to CSS JIT one. Make sense.

> Source/WebCore/css/SelectorChecker.cpp:768
> +                if (!parentElement.isFinishedParsingChildren())

It is not checked in the case of shadow root, but it is OK since shadow root children are not populated by parsing, is my understanding correct?

> Source/WebCore/cssjit/SelectorCompiler.cpp:3200
> +    Assembler::RegisterID parentNode = m_registerAllocator.allocateRegister();

Let's use `LocalRegister parentNode(m_registerAllocator)`.

> Source/WebCore/cssjit/SelectorCompiler.cpp:3225
> +    m_assembler.jump().linkTo(checkWithoutRelation, &m_assembler);

Remove this jump, fallthough to checkWithoutRelation.

> Source/WebCore/cssjit/SelectorCompiler.cpp:3227
> +    m_registerAllocator.deallocateRegister(parentNode);

Remove this by using LocalRegister.

> Source/WebCore/cssjit/SelectorCompiler.cpp:3228
> +

How about putting checkWithoutRelation here?

Assembler::Label checkWithoutRelation(m_assembler.label());
notResolvingStyle.link(&m_assembler);
failureCases.append(m_assembler.branchTest32(Assembler::NonZero, isFirstChildRegister));
// fallthough to successCases.

> Source/WebCore/cssjit/SelectorCompiler.cpp:3294
> +    Assembler::RegisterID parentNode = m_registerAllocator.allocateRegister();

Let's use `LocalRegister parentNode(m_registerAllocator)`.

> Source/WebCore/cssjit/SelectorCompiler.cpp:3335
> +    m_assembler.jump().linkTo(checkWithoutRelation, &m_assembler);

Remove this jump, fallthough to checkWithoutRelation.

> Source/WebCore/cssjit/SelectorCompiler.cpp:3337
> +    m_registerAllocator.deallocateRegister(parentNode);

Remove this by using LocalRegister.

> Source/WebCore/cssjit/SelectorCompiler.cpp:3338
> +

How about putting checkWithoutRelation here?

Assembler::Label checkWithoutRelation(m_assembler.label());
notResolvingStyle.link(&m_assembler);
failureCases.append(m_assembler.branchTest32(Assembler::NonZero, isLastChildRegister));
// fallthough to successCases.

> Source/WebCore/cssjit/SelectorCompiler.cpp:3362
> +    Assembler::RegisterID parentNode = m_registerAllocator.allocateRegister();

Ditto.

> Source/WebCore/cssjit/SelectorCompiler.cpp:3405
> +    Assembler::Label checkWithoutRelation(m_assembler.label());
>      notResolvingStyle.link(&m_assembler);
>      failureCases.append(m_assembler.branchTest32(Assembler::NonZero, isOnlyChildRegister));
> +    successCases.append(m_assembler.jump());

Ditto.

> Source/WebCore/cssjit/SelectorCompiler.cpp:3412
> +    m_registerAllocator.deallocateRegister(parentNode);

Ditto.

> Source/WebCore/cssjit/SelectorCompiler.cpp:3557
> +void SelectorCodeGenerator::generateNthChildParentCheckAndRelationUpdate(Assembler::JumpList& failureCases, const SelectorFragment& fragment)

Nice cleanup.

> Source/WebCore/cssjit/SelectorCompiler.cpp:3679
> +void SelectorCodeGenerator::generateNthLastChildParentCheckAndRelationUpdate(Assembler::JumpList& failureCases, const SelectorFragment& fragment)

Nice cleanup.

> Source/WebCore/domjit/DOMJITHelpers.h:192
> +inline CCallHelpers::Jump branchTestIsShadowRootFlagOnNode(MacroAssembler& jit, CCallHelpers::ResultCondition condition, GPRReg nodeAddress)
> +{
> +    return jit.branchTest32(condition, CCallHelpers::Address(nodeAddress, Node::nodeFlagsMemoryOffset()), CCallHelpers::TrustedImm32(Node::flagIsShadowRoot()));
> +}
> +
> +inline CCallHelpers::Jump branchTestIsElementOrShadowRootFlagOnNode(MacroAssembler& jit, CCallHelpers::ResultCondition condition, GPRReg nodeAddress)
> +{
> +    return jit.branchTest32(condition, CCallHelpers::Address(nodeAddress, Node::nodeFlagsMemoryOffset()),
> +        CCallHelpers::TrustedImm32(Node::flagIsShadowRoot() | Node::flagIsElement()));
> +}
> +

Nice.

[Ryosuke Niwa]

Comment on attachment 349252 [details]
Fixes the bug

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

> Source/WebCore/css/SelectorChecker.cpp:748
> +                break;

Yeah, and also with isLastChild case where we'd have to set the flag to false when parentElement.isFinishedParsingChildren() is false.

>> Source/WebCore/css/SelectorChecker.cpp:768
>> +                if (!parentElement.isFinishedParsingChildren())
> 
> It is not checked in the case of shadow root, but it is OK since shadow root children are not populated by parsing, is my understanding correct?

Yes, the flag is only available on an element. The HTML parser never appends a child to a ShadowRoot.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:3200
>> +    Assembler::RegisterID parentNode = m_registerAllocator.allocateRegister();
> 
> Let's use `LocalRegister parentNode(m_registerAllocator)`.

Oh yeah, I was wondering about that. Fixed.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:3225
>> +    m_assembler.jump().linkTo(checkWithoutRelation, &m_assembler);
> 
> Remove this jump, fallthough to checkWithoutRelation.

Fixed.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:3227
>> +    m_registerAllocator.deallocateRegister(parentNode);
> 
> Remove this by using LocalRegister.

Yup.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:3228
>> +
> 
> How about putting checkWithoutRelation here?
> 
> Assembler::Label checkWithoutRelation(m_assembler.label());
> notResolvingStyle.link(&m_assembler);
> failureCases.append(m_assembler.branchTest32(Assembler::NonZero, isFirstChildRegister));
> // fallthough to successCases.

Oh, that's a lot cleaner! Fixed here and for generateElementIsLastChild.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:3294
>> +    Assembler::RegisterID parentNode = m_registerAllocator.allocateRegister();
> 
> Let's use `LocalRegister parentNode(m_registerAllocator)`.

Fixed.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:3335
>> +    m_assembler.jump().linkTo(checkWithoutRelation, &m_assembler);
> 
> Remove this jump, fallthough to checkWithoutRelation.

Yup, applied the same refactoring here.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:3337
>> +    m_registerAllocator.deallocateRegister(parentNode);
> 
> Remove this by using LocalRegister.

Fixed.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:3338
>> +
> 
> How about putting checkWithoutRelation here?
> 
> Assembler::Label checkWithoutRelation(m_assembler.label());
> notResolvingStyle.link(&m_assembler);
> failureCases.append(m_assembler.branchTest32(Assembler::NonZero, isLastChildRegister));
> // fallthough to successCases.

Fixed.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:3362
>> +    Assembler::RegisterID parentNode = m_registerAllocator.allocateRegister();
> 
> Ditto.

Fixed.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:3405
>> +    successCases.append(m_assembler.jump());
> 
> Ditto.

Fixed.

>> Source/WebCore/cssjit/SelectorCompiler.cpp:3412
>> +    m_registerAllocator.deallocateRegister(parentNode);
> 
> Ditto.

Fixed.

[Ryosuke Niwa]

Created attachment 349359 [details]
Addressed the review comments

[Yusuke Suzuki]

Comment on attachment 349359 [details]
Addressed the review comments

r=me

[Ryosuke Niwa]

Committed r235917: <https://trac.webkit.org/changeset/235917>