Allow positional selectors to match elements without a parent.

Allow positional selectors to match elements without a parent.

BUG=675670
TEST=fast/selectors/child-indexed-pseudo-classes.html

Review-Url: https://codereview.chromium.org/2588643004
Cr-Commit-Position: refs/heads/master@{#444956}
Committed: https://chromium.googlesource.com/chromium/src/+/464e47a82e253d08a7c7236fcf5cb5ec5bf8660b

[emilio, 2016-12-19 21:42:03]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 21:42:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 23:24:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-19 23:24:18]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[emilio, 2016-12-19 23:43:40]

The CQ bit was checked by ecobos@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 23:44:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 02:58:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 02:58:49]

Dry run: This issue passed the CQ dry run.

[emilio, 2016-12-20 07:28:31]

ecobos@igalia.com changed reviewers:
+ rune@opera.com

[emilio, 2016-12-20 07:28:31]

Hi rune,

Want to review this? Was a straight-forward change I did while waiting for a CQ
run for another CL that should be in your queue today (sorry!) :)

I'm slightly concerned about style invalidation bugs, but I think we're fine
since there should be no chance of two sibling elements without a common parent,
right? Maybe with shadow dom that's a little bit more complicated? Anyway,
that's why I'm flagging you for review :)

[emilio, 2016-12-20 07:56:29]

On 2016/12/20 07:28:31, emilio wrote:
> Hi rune,
> 
> Want to review this? Was a straight-forward change I did while waiting for a
CQ
> run for another CL that should be in your queue today (sorry!) :)
> 
> I'm slightly concerned about style invalidation bugs, but I think we're fine
> since there should be no chance of two sibling elements without a common
parent,
> right? Maybe with shadow dom that's a little bit more complicated? Anyway,
> that's why I'm flagging you for review :)

Heh, after seeing a duplicated issue
(https://bugs.chromium.org/p/chromium/issues/detail?id=666655) I've seen you
mentioned this needs an intent to ship.

Gecko doesn't seem finally backing that out, and it seems the consensus per
https://github.com/w3c/csswg-drafts/issues/695 is that this should be the way to
go.

That being said, I can also write it down in an intent to ship if you think it's
still necessary.

[rune, 2016-12-20 09:10:54]

On 2016/12/20 07:56:29, emilio wrote:

> That being said, I can also write it down in an intent to ship if you think
it's
> still necessary.

Please do, so we're in the clear.

[rune, 2016-12-20 09:18:47]

https://codereview.chromium.org/2588643004/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/misc/acid3.html (right):

https://codereview.chromium.org/2588643004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/misc/acid3.html:1264:
expect(doc.documentElement, first, "root element, with no parent node, claims to
be a :first-child");
Right, we're breaking Acid3. That should be mentioned in the intent.

https://codereview.chromium.org/2588643004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/NthIndexCache.cpp (right):

https://codereview.chromium.org/2588643004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NthIndexCache.cpp:86: if
(element.parentNode() && nthIndexCache && nthIndexCache->m_parentMap)
If you don't have a parent node, won't N always be one? Then you could do a
parentNode() check with the isPseudoElement() check instead.

[esprehn, 2016-12-21 20:27:30]

Can you explain in what situations you have no parentNode? It's possible to have
no parentElement but anything inside the tree should have a parentNode except
the ShadowRoots.

[rune, 2016-12-21 21:55:03]

On 2016/12/21 20:27:30, esprehn wrote:
> Can you explain in what situations you have no parentNode? It's possible to
have
> no parentElement but anything inside the tree should have a parentNode except
> the ShadowRoots.

You can querySelector like this:

  <!DOCTYPE html>
  <script>
    var outer = document.createElement("div");
    var inner = document.createElement("span");
    outer.appendChild(inner);
    document.write(outer.querySelector(":first-child > span"));
  </script>

outer does not have a parent node when trying to match it with :first-child.

[emilio, 2017-01-18 16:10:49]

The intent to ship three LGTMs:

 *
https://groups.google.com/a/chromium.org/d/msg/blink-dev/RgdSWmv0s4g/vIJi1H5j...
 *
https://groups.google.com/a/chromium.org/d/msg/blink-dev/RgdSWmv0s4g/Xlc8_FJl...
 *
https://groups.google.com/a/chromium.org/d/msg/blink-dev/RgdSWmv0s4g/eDDs1N-v...

Just uploaded an updated patch that addresses rune's comment.

Let me know if I should do anything else to move this forward.

[emilio, 2017-01-18 16:11:23]

On 2017/01/18 16:10:49, emilio wrote:
> The intent to ship three LGTMs:

s/three/got three/

[emilio, 2017-01-18 16:24:01]

NVM, rego just pointed me out the difference between module owners and API
owners, sorry for the noise.

[rune, 2017-01-18 21:33:19]

https://codereview.chromium.org/2588643004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/selectors/child-indexed-pseudo-classes.html
(right):

https://codereview.chromium.org/2588643004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/selectors/child-indexed-pseudo-classes.html:22:
[ ":last-child", true ],
Test :only-child as well?

https://codereview.chromium.org/2588643004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/NthIndexCache.cpp (right):

https://codereview.chromium.org/2588643004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NthIndexCache.cpp:91: if
(element.parentNode() && nthIndexCache && index > kCachedSiblingCountLimit)
parentNode() is always non-null here.

https://codereview.chromium.org/2588643004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NthIndexCache.cpp:101: if
(element.parentNode() && nthIndexCache && nthIndexCache->m_parentMap)
Check !parentNode and return 1 as for nthChildIndex and skip the null check
here?

https://codereview.chromium.org/2588643004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NthIndexCache.cpp:106: if
(element.parentNode() && nthIndexCache && index > kCachedSiblingCountLimit)
No need to add the parentNode() check if you early-return 1 for !parentNode().

https://codereview.chromium.org/2588643004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NthIndexCache.cpp:112: if
(!m_parentMapForType || !element.parentNode())
Early return in nthOfTypeIndex and nthLastOfTypeIndex instead?

[emilio, 2017-01-18 23:33:07]

https://codereview.chromium.org/2588643004/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/selectors/child-indexed-pseudo-classes.html
(right):

https://codereview.chromium.org/2588643004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/selectors/child-indexed-pseudo-classes.html:22:
[ ":last-child", true ],
On 2017/01/18 21:33:19, rune wrote:
> Test :only-child as well?

Yeah, tested that and :only-of-type too, and querySelector/querySelectorAll on
the document and a document fragment.

https://codereview.chromium.org/2588643004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/NthIndexCache.cpp (right):

https://codereview.chromium.org/2588643004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NthIndexCache.cpp:91: if
(element.parentNode() && nthIndexCache && index > kCachedSiblingCountLimit)
On 2017/01/18 21:33:19, rune wrote:
> parentNode() is always non-null here.

Good catches, sorry, should've paged in better all the conversation and the
previous review before updating the patch. You shouldn't have needed to point
these out.

[rune, 2017-01-19 00:16:05]

lgtm, but seems like you need one more lgtm on the intent to ship.

[emilio, 2017-01-19 22:59:52]

The CQ bit was checked by ecobos@igalia.com

[commit-bot: I haz the power, 2017-01-19 23:00:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 02:19:04]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484866792665790,
"parent_rev": "8127a839bf18e618aaf7a8cad95440c6cd3f9f76", "commit_rev":
"464e47a82e253d08a7c7236fcf5cb5ec5bf8660b"}

[commit-bot: I haz the power, 2017-01-20 02:19:32]

Description was changed from

==========
Allow positional selectors to match elements without a parent.

BUG=675670
TEST=fast/selectors/child-indexed-pseudo-classes.html
==========

to

==========
Allow positional selectors to match elements without a parent.

BUG=675670
TEST=fast/selectors/child-indexed-pseudo-classes.html

Review-Url: https://codereview.chromium.org/2588643004
Cr-Commit-Position: refs/heads/master@{#444956}
Committed:
https://chromium.googlesource.com/chromium/src/+/464e47a82e253d08a7c7236fcf5c...
==========

[commit-bot: I haz the power, 2017-01-20 02:19:33]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/464e47a82e253d08a7c7236fcf5c...