Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(791)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2505543004: Let querySelector(All) match (nth-)last with unclosed parent. (Closed)

Created:
4 years, 1 month ago by rune
Modified:
4 years, 1 month ago
Reviewers:
sashab
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Let querySelector(All) match (nth-)last with unclosed parent. While parsing, we don't match :last*, :nth-last* etc until we finish parsing children to avoid alternating between different computed styles during loading. For querying selectors, however, we should. I couldn't find this explicitly mentioned in w3c or whatwg specs for querySelector(All), but Firefox and IE does this. This could happen if you have: <body> <p></p> <p></p> <script>document.querySelector("p:last-of-type")</script> </body> Adding expectations file for a wpt which now fails. The modifications to the test has been upstreamed to the github repo. See PR [1]. [1] https://github.com/w3c/web-platform-tests/pull/4216 R=sashab@chromium.org BUG=662036 Committed: https://crrev.com/2b2e6f1d8ded02c97fbdda7088e24f84ad035ea3 Cr-Commit-Position: refs/heads/master@{#432493}

Patch Set 1 #

Patch Set 2 : Added expectation file #

Unified diffs Side-by-side diffs Delta from patch set Stats (+42 lines, -6 lines) Patch
A third_party/WebKit/LayoutTests/imported/wpt/shadow-dom/untriaged/shadow-trees/upper-boundary-encapsulation/test-009-expected.txt View 1 1 chunk +17 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/SelectorChecker.cpp View 6 chunks +6 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/dom/SelectorQueryTest.cpp View 2 chunks +19 lines, -0 lines 0 comments Download

Messages

Total messages: 17 (9 generated)
rune
ptal
4 years, 1 month ago (2016-11-15 14:43:46 UTC) #3
sashab
lgtm but make sure you check for regressions, I think the failing test might be ...
4 years, 1 month ago (2016-11-16 02:01:21 UTC) #6
rune
On 2016/11/16 02:01:21, sashab wrote: > lgtm but make sure you check for regressions, I ...
4 years, 1 month ago (2016-11-16 09:47:10 UTC) #7
rune
On 2016/11/16 09:47:10, rune wrote: > On 2016/11/16 02:01:21, sashab wrote: > > lgtm but ...
4 years, 1 month ago (2016-11-16 12:27:10 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2505543004/20001
4 years, 1 month ago (2016-11-16 13:49:54 UTC) #12
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 1 month ago (2016-11-16 15:22:56 UTC) #14
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/2b2e6f1d8ded02c97fbdda7088e24f84ad035ea3 Cr-Commit-Position: refs/heads/master@{#432493}
4 years, 1 month ago (2016-11-16 15:28:31 UTC) #16
sashab
4 years, 1 month ago (2016-11-17 02:06:26 UTC) #17
Message was sent while issue was closed. 
Great work with the pull request.

Powered by Google App Engine
This is Rietveld 408576698