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

[sjmelia]

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.

---

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors
• 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

[highfive]

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @metajack (or someone else) soon.

[highfive]

Heads up! This PR modifies the following files:

• @bholley: components/style/element_state.rs, components/style/selector_impl.rs
• @KiChjang: components/script/dom/document.rs, components/script/dom/element.rs, components/script/script_thread.rs

[highfive]

Warning

• These commits modify style, layout, and script code, but no tests are modified. Please consider adding a test!

[mbrubeck]

I can review this but I'll be travelling for the next couple of days so I may not be online much. If another reviewer can get to it sooner, feel free to steal it.

I would like some help because i'm not sure how to resolve this; I can only think to call window.reflow.

Yes, calling window.reflow is correct. You can look at Document::handle_mouse_move_event for reference; it calls window.reflow after calling element.set_hover_state.

(Side note: While glancing at this code I noticed that Element::active_state and set_active_state seem to be unused. If you (or anyone else) would like to figure out why that is (are they no longer needed? or are we missing some code that should be calling them?) and file an issue or PR, that would be great.)

[sjmelia]

Thanks Matt, I'll add the reflow call and more than happy to investigate active_state!

[sjmelia]

Ok added the reflow (I chose ReflowReason::DocumentLoaded) then did some more flailing;

I think that (a) the script_thread.handle_navigate function is dealing with Step 7 of https://html.spec.whatwg.org/multipage/browsers.html#navigating-across-documents and is functional, but that (b) loading a new document with an URL fragment is Step 6 of https://html.spec.whatwg.org/multipage/browsers.html#update-the-session-history-with-the-new-page and is currently non-functional.

There is a sporadic scroll; but this occurs due to handle_resize_event so i'm not sure it's intentional and didn't look like the right place to add code setting the pseudo-selector.

For this reason; i've added a second commit to the PR that scrolls to the fragment and adds the pseudoselector after the document loads. I'm not convinced it's in the right place, and the test no longer fails; but instead times out... so i'm not sure that's much progress.

I think I still need to add a test for scenario a) - if so, do I just commit one to the web-platform-tests?

[highfive]

New code was committed to pull request.

[sjmelia]

active state is not yet implemented; it's referenced in an existing issue, #8719

[mbrubeck]

This looks good! Some minor comments below.

For this reason; i've added a second commit to the PR that scrolls to the fragment and adds the pseudoselector after the document loads. I'm not convinced it's in the right place, and the test no longer fails; but instead times out... so i'm not sure that's much progress.

I think that scrolling on load is correct. It looks like this test is just so big that it always times out in debug builds (even without these changes). In a release build it passes.

-S-awaiting-review +S-needs-code-changes

---

Reviewed 7 of 7 files at r1, 3 of 3 files at r2, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

components/script/dom/document.rs, line 1844 [r1] (raw file):

    pub fn set_target_element(&self, node: Option<&Element>) {
        match self.target_element.get()  {

Suggestion: This function could be simplified slightly by using if let instead of match.

---

components/script/dom/document.rs, line 1859 [r2] (raw file):

        self.window.reflow(ReflowGoal::ForDisplay,
                               ReflowQueryType::NoQuery,
                               ReflowReason::DocumentLoaded);

I think we should add a new ReflowReason, something like ElementStateChanged.

---

Comments from Reviewable

[Manishearth]

Looks ready to merge?

[Ms2ger]

I'd like to take a look; hopefully today, but might be tomorrow.

[bors-servo]

☔ The latest upstream changes (presumably #11754) made this pull request unmergeable. Please resolve the merge conflicts.

[sjmelia]

I've rebased this - maybe the label will get removed next time Homu visits?

[emilio]

ping @Ms2ger? You wanted to review this :)

[sjmelia]

...sorry about that. have removed the entries from ParentNode-querySelector-all-xht.xht

[jdm]

@bors-servo: r=mbrubeck

[bors-servo]

📌 Commit 04f5369 has been approved by mbrubeck

[bors-servo]

⌛ Testing commit 04f5369 with merge 1936da9...

[bors-servo]

💔 Test failed - mac-rel-css

[highfive]

  ▶ TIMEOUT [expected PASS] /css21_dev/html4/colors-005.htm

  ▶ TIMEOUT [expected PASS] /css21_dev/html4/colors-006.htm

[jdm]

@bors-servo: retry

• #12713

[bors-servo]

⌛ Testing commit 04f5369 with merge 71fa006...

[bors-servo]

💔 Test failed - mac-rel-wpt

[emilio]

@bors-servo: retry

• #12654

[bors-servo]

⌛ Testing commit 04f5369 with merge 7a7bdf5...

[bors-servo]

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev