Add support for :target pseudo-selector #7720

Manishearth · 2015-09-23T18:10:09Z

See #7720 (comment) for instructions on how to solve this.

Relevant spec: https://html.spec.whatwg.org/multipage/scripting.html#selector-target

Manishearth · 2015-09-23T18:25:26Z

For efficiency, first check if the fragment matches the elements id or name, and then proceed with doing a document traversal.

@SimonSapin It occurs to me that pseudo-selectors that depend on a property of the document, rather than a direct property of the element (e.g. :target, :current, etc) are quite inefficient with querySelector(), since for each element, one must access the document, fetch some properties, and possibly do a tree traversal. This is in contrast with other selectors where it's always a local check.

I'm not sure how Servo's style updates happen, but there's also a chance for inefficiency there. Would these selectors cause a lot of churn on DOM manipulations?

mbrubeck · 2015-09-23T18:58:18Z

For what it's worth, Gecko and Blink do not seem to recalculate :target matching when the DOM changes, as seen in this test case:

<html>
  <head>
    <style>
      :target { color: red; }
    </style>
  </head>
  <body>
    <ul><li id="foo">hello</li></ul>
    <script>
      document.querySelector("ul").addEventListener("click", function() {
        this.insertBefore(this.firstChild.cloneNode(true), this.firstChild);
      });
    </script>
  </body>
</html>

SimonSapin · 2015-09-23T21:17:31Z

Yeah, we probably don’t want to traverse the document every time :target is matched on a given element. Could we maintain a "targeted" flag on every element? These flags would need to be updated (at least) when an element is inserted or removed, when an id or name attribute is added/removed/modified, or when the fragment identifier of the document’s URL changes.

Manishearth · 2015-09-24T06:14:41Z

Using NodeFlags? That sounds like a good idea. I don't like stuffing lots of flags into NodeFlags, but for now that seems okay.

This still requires a full dom traversal whenever a node is moved. We can improve it so that it first does a traversal of the moved node though.

gilles-leblanc · 2015-10-16T01:47:49Z

I'd like to take a look at this one. Will be the hardest thing I have tackled on Servo as of yet.

Manishearth · 2015-10-16T01:49:17Z

This really depends on that spec issue being resolved. We could do a correct but somewhat inefficient implementation (as detailed in the comments above) now, though.

gilles-leblanc · 2015-10-16T01:54:07Z

@Manishearth Ok, I'll look for another one then. I hadn't realized it was waiting on the spec being resolved.

jdm · 2016-05-24T22:36:01Z

@Manishearth What's the specification status here? If this isn't ready for someone to work on it, we should remove the less easy tag.

Manishearth · 2016-05-25T13:36:58Z

I plan to fix the spec issue, but detagged for now

highfive · 2016-06-07T13:24:55Z

Please make a comment here if you intend to work on this issue. Thank you!

SimonSapin · 2016-06-07T13:28:00Z

The spec was changed in whatwg/html#1370

Manishearth · 2016-06-07T13:28:43Z

The specification has been updated.

To solve this bug, you need to:

~~Undo servo/rust-selectors#54, get a new version published (until the changes are published, use a local cargo override)~~
Add a target_element: MutNullableHeap<JS<Element>> field to the document (https://html.spec.whatwg.org/multipage/browsers.html#target-element)
Set/reset it whenever the document scrolls to the indicated part of the document (https://html.spec.whatwg.org/multipage/browsers.html#the-indicated-part-of-the-document), in handle_navigate in script_thread

Code: components/script/dom/document.rs, components/script/script_thread.rs

SimonSapin · 2016-06-07T14:52:01Z

Undo servo/rust-selectors#54, get a new version published (until the changes are published, use a local cargo override)

That is not needed: since then, the handling of pseudo-classes other than "tree structural" ones has been made generic and left to users of rust-selectors to implement themselves. In Servo, that happens in components/style/selector_impl.rs where you’ll need to add a variant to the NonTSPseudoClass enum and parse it in parse_non_ts_pseudo_class.

sjmelia · 2016-06-11T06:00:47Z

I'd like to pick this up - it looks like the spec issue is cleared up now; although it still has the flag?

i've made some progress already but have a few questions, do I open a pull request?

Manishearth · 2016-06-11T06:18:08Z

Sure, feel free to open a pull request and ask any questions you have.

What flag?

sjmelia · 2016-06-11T07:21:01Z

Great! R.e. flag, sorry - I meant the "i-spec-unclear" label, I read your comment above as indicating that the spec was clarified and the selected element is not affected by dom changes.

Manishearth · 2016-06-11T07:30:58Z

oh, yeah, that label can be removed


        Auto merge of #11726 - sjmelia:7720_add_target_selector, r=mbrubeck

Issue 7720: Add target selector and update when scrolling to fragment  Add the target pseudo selector and set/unset it during scrolling to fragment. This change is not complete as no repaint is triggered after the selector is added - it will only take effect after a repaint is triggered by e.g. hovering over another element. (See manual test) I would like some help because i'm not sure how to resolve this; I can only think to call window.reflow. I added a manual test case, don't think this counts really! I think the applicable automated test is in /tests/wpt/web-platform-tests/dom/nodes/Element-matches.html but it currently fails, I think due to the above. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #7720 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11726)


        Auto merge of #11726 - sjmelia:7720_add_target_selector, r=mbrubeck

Issue 7720: Add target selector and update when scrolling to fragment  Add the target pseudo selector and set/unset it during scrolling to fragment. This change is not complete as no repaint is triggered after the selector is added - it will only take effect after a repaint is triggered by e.g. hovering over another element. (See manual test) I would like some help because i'm not sure how to resolve this; I can only think to call window.reflow. I added a manual test case, don't think this counts really! I think the applicable automated test is in /tests/wpt/web-platform-tests/dom/nodes/Element-matches.html but it currently fails, I think due to the above. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #7720 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11726)


        Auto merge of #11726 - sjmelia:7720_add_target_selector, r=mbrubeck

Issue 7720: Add target selector and update when scrolling to fragment  Add the target pseudo selector and set/unset it during scrolling to fragment. This change is not complete as no repaint is triggered after the selector is added - it will only take effect after a repaint is triggered by e.g. hovering over another element. (See manual test) I would like some help because i'm not sure how to resolve this; I can only think to call window.reflow. I added a manual test case, don't think this counts really! I think the applicable automated test is in /tests/wpt/web-platform-tests/dom/nodes/Element-matches.html but it currently fails, I think due to the above. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #7720 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11726)


        Auto merge of #11726 - sjmelia:7720_add_target_selector, r=mbrubeck

Issue 7720: Add target selector and update when scrolling to fragment  Add the target pseudo selector and set/unset it during scrolling to fragment. This change is not complete as no repaint is triggered after the selector is added - it will only take effect after a repaint is triggered by e.g. hovering over another element. (See manual test) I would like some help because i'm not sure how to resolve this; I can only think to call window.reflow. I added a manual test case, don't think this counts really! I think the applicable automated test is in /tests/wpt/web-platform-tests/dom/nodes/Element-matches.html but it currently fails, I think due to the above. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #7720 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11726)


        Auto merge of #11726 - sjmelia:7720_add_target_selector, r=mbrubeck

Issue 7720: Add target selector and update when scrolling to fragment  Add the target pseudo selector and set/unset it during scrolling to fragment. This change is not complete as no repaint is triggered after the selector is added - it will only take effect after a repaint is triggered by e.g. hovering over another element. (See manual test) I would like some help because i'm not sure how to resolve this; I can only think to call window.reflow. I added a manual test case, don't think this counts really! I think the applicable automated test is in /tests/wpt/web-platform-tests/dom/nodes/Element-matches.html but it currently fails, I think due to the above. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #7720 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11726)

Manishearth added A-content/css A-content/dom E-easy I-enhancement labels Sep 23, 2015

Manishearth added E-less easy and removed E-easy labels Sep 24, 2015

Manishearth mentioned this issue Sep 24, 2015

:target should be stickier, to match what browsers do, to be more efficient, and to be more in line with the use case whatwg/html#190

Closed

jdm added the I-spec-unclear label Oct 16, 2015

Manishearth added the E-easy label Jun 7, 2016

Manishearth removed the I-spec-unclear label Jun 11, 2016

sjmelia mentioned this issue Jun 11, 2016

Issue 7720: Add target selector and update when scrolling to fragment #11726

Merged

3 of 5 tasks complete

KiChjang added C-assigned C-has open PR labels Jun 17, 2016

bors-servo closed this in #11726 Aug 3, 2016

servo / servo

Add support for :target pseudo-selector #7720

Add support for :target pseudo-selector #7720

Manishearth commented Sep 23, 2015 •

edited

Manishearth commented Sep 23, 2015

mbrubeck commented Sep 23, 2015

SimonSapin commented Sep 23, 2015

Manishearth commented Sep 24, 2015

gilles-leblanc commented Oct 16, 2015

Manishearth commented Oct 16, 2015

gilles-leblanc commented Oct 16, 2015

jdm commented May 24, 2016

Manishearth commented May 25, 2016

highfive commented Jun 7, 2016

SimonSapin commented Jun 7, 2016

Manishearth commented Jun 7, 2016 •

edited by SimonSapin

SimonSapin commented Jun 7, 2016

sjmelia commented Jun 11, 2016 •

edited

Manishearth commented Jun 11, 2016

sjmelia commented Jun 11, 2016

Manishearth commented Jun 11, 2016

servo / servo

Join GitHub today

Add support for :target pseudo-selector #7720

Add support for :target pseudo-selector #7720

Comments

Manishearth commented Sep 23, 2015 • edited

Manishearth commented Sep 23, 2015

mbrubeck commented Sep 23, 2015

SimonSapin commented Sep 23, 2015

Manishearth commented Sep 24, 2015

gilles-leblanc commented Oct 16, 2015

Manishearth commented Oct 16, 2015

gilles-leblanc commented Oct 16, 2015

jdm commented May 24, 2016

Manishearth commented May 25, 2016

highfive commented Jun 7, 2016

SimonSapin commented Jun 7, 2016

Manishearth commented Jun 7, 2016 • edited by SimonSapin

SimonSapin commented Jun 7, 2016

sjmelia commented Jun 11, 2016 • edited

Manishearth commented Jun 11, 2016

sjmelia commented Jun 11, 2016

Manishearth commented Jun 11, 2016

Manishearth commented Sep 23, 2015 •

edited

Manishearth commented Jun 7, 2016 •

edited by SimonSapin

sjmelia commented Jun 11, 2016 •

edited