Bug 166748

Summary: :first-child, :last-child, :nth-child, and :nth-of-type don't work on shadow root's children
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: CSSAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, ews, hyatt, koivisto, rniwa, simon.fraser, webkit, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 189392    
Bug Blocks: 148695, 189738    
Attachments:
Description Flags
Reduction
none
WIP
none
Archive of layout-test-results from ews103 for mac-sierra
none
Archive of layout-test-results from ews107 for mac-sierra-wk2
none
Archive of layout-test-results from ews112 for mac-sierra
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Fixes the bug
none
Addressed the review comments ysuzuki: review+

Description Ryosuke Niwa 2017-01-05 16:46:34 PST
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.
Comment 1 Ryosuke Niwa 2017-01-05 16:47:33 PST
Created attachment 298151 [details]
Reduction
Comment 2 Ryosuke Niwa 2017-01-05 16:48:47 PST
<rdar://problem/29649177>
Comment 3 Ryosuke Niwa 2018-09-05 16:25:26 PDT
*** Bug 184273 has been marked as a duplicate of this bug. ***
Comment 4 Ryosuke Niwa 2018-09-07 00:19:14 PDT
Created attachment 349116 [details]
WIP
Comment 5 Build Bot 2018-09-07 01:24:33 PDT
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
Comment 6 Build Bot 2018-09-07 01:24:35 PDT
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
Comment 7 Build Bot 2018-09-07 01:35:05 PDT
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
Comment 8 Build Bot 2018-09-07 01:35:07 PDT
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
Comment 9 Build Bot 2018-09-07 01:59:25 PDT
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
Comment 10 Build Bot 2018-09-07 01:59:27 PDT
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
Comment 11 Build Bot 2018-09-07 02:17:03 PDT
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
Comment 12 Build Bot 2018-09-07 02:17:05 PDT
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
Comment 13 Ryosuke Niwa 2018-09-08 01:08:57 PDT
Created attachment 349252 [details]
Fixes the bug
Comment 14 Yusuke Suzuki 2018-09-08 10:39:33 PDT
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.
Comment 15 Ryosuke Niwa 2018-09-10 17:14:21 PDT
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.
Comment 16 Ryosuke Niwa 2018-09-10 17:17:00 PDT
Created attachment 349359 [details]
Addressed the review comments
Comment 17 Yusuke Suzuki 2018-09-11 02:31:04 PDT
Comment on attachment 349359 [details]
Addressed the review comments

r=me
Comment 18 Ryosuke Niwa 2018-09-11 16:00:34 PDT
Committed r235917: <https://trac.webkit.org/changeset/235917>