Implement scroll, scrollLeft, scrollTop and friends, addressing issue #9650

[izgzhen]

This is a work to solve #9650. Thanks a lot for helping the review.

• scroll
• scrollTo
• scrollBy
• scrollTop (setter and getter)
• scrollLeft (setter and getter)

The setters will be implemented in another PR after this is merged.

---

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 @KiChjang (or someone else) soon.

[highfive]

Warning

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

[asajeffrey]

Hi there @izgzhen, thanks for contributing!

@danlrobertson, does this PR overlap with #9824 at all?

[dlrobertson]

The spec is similar, but there shouldn't be too much overlap. I wanted to implement scrollWidth and scrollHeight first so we could create a method for the scrolling area, which is needed for a few of the methods here, but that is the only overlap I can see at the moment.

@izgzhen: I think you're missing SetScrollLeft in your checklist. Great work!

[izgzhen]

@asajeffrey There is something different, and I will try to avoid duplication by abstracting common utilities out.

@danlrobertson thanks, I don't write it on the list because I haven't write a single line of code yet :P

[asajeffrey]

#9824 landed!

[bors-servo]

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

[asajeffrey]

@izgzhen would you like me to start reviewing, or hold off for the moment?

[izgzhen]

@asajeffrey Please wait for a minute, I will need a little time to rebase and think about incorporating changes introduced in PR #9824.

[izgzhen]

@asajeffrey rebase and some more polishing is done, I am hitting the wall now :P

I have two problems now:

1. How to get the computed style for a given element (i.e. what get_computed does)? The motivation is to "check whether the element has any associated CSS layout box, and whether the element has any associated scrolling box. The pull Implement scroll, scrollLeft, scrollTop and friends, addressing issue #9650 #9968 did a similar thing through layout_rpc and process the query in layout::query, but I don't know it is also proper to my case and if there is an easier way to complete this.
2. I intend to use the get_scroll_area by pull Implement scroll, scrollLeft, scrollTop and friends, addressing issue #9650 #9968 at the final line of getting on ScrollLeft and ScrollTop, but first there is a coercion, and second some checking over document and window did overlap, although the spec of ScrollLeft and ScrollWidth differ in a very interleaving way :(

[asajeffrey]

I did some preliminary reading through, which makes me think that there needs to be a bit of care about what is handled by the script thread, and what is handled by the layout thread (c.f. the approach in #9824). I think you're going to need to construct a message which is sent from script to layout requesting the necessary calculated values, and there should be at most one such message for each call to a DOM method.

@danlrobertson: you recently looked at this code, care to weigh in?

---

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

---

components/script/dom/element.rs, line 173 [r4] (raw file):
I think "elements that have a computed value of the display property that is table-column or table-column-group must be considered to have an associated CSS layout box" is intended as a comment, not a definition. Indeed, the spec says "ISSUE 1: The terms CSS layout box and SVG layout box are not currently defined by CSS or SVG."

For the moment I think this function should just return true, but we should probably check on IRC that this is correct. If so, we should file an issue saying that the spec is unclear here.

---

components/script/dom/element.rs, line 180 [r4] (raw file):
I think this can be simplified to:

• The element has an associated CSS layout box, and
• The element’s used value of the overflow-x or overflow-y properties is not visible.

This is the first method that requires computed values.

---

Comments from the review on Reviewable.io

[dlrobertson]

Agreed. A good portion of this will definitely involve the layout thread. I'll do some more reading as well and come back with hopefully more info.

did overlap, although the spec of ScrollLeft and ScrollWidth differ in a very interleaving way :(

Yeah, I noticed there were a couple very small differences (for the getters). I wonder how much of it is just wording and how much of it is actually different. I can ask on IRC and hope for some responses. Which parts in particular are you talking about?

Just a suggestion. For these particular methods I would focus on implementing the "working" or "hard" part of the algorithm first. In your case the last step of each method. Furthermore, I would work on the setters first. It's a bit complicated as you'll have to perform a scroll (not sure if it has been implemented yet), but once you have that, i think you might have a better foundation to work off of and it should be easier to go back and implement the restrictions. Just a suggestion... feel free not to take it 😄

---

Review status: 0 of 2 files reviewed at latest revision, 4 unresolved discussions.

---

components/script/dom/element.rs, line 180 [r4] (raw file):
You could use Fragment::style to get ComputedValues::get_box to get the Box which contains overflow_x and overflow_y, but I'm not sure if that can be accessed outside of the layout thread (correct me if I'm wrong).

---

components/script/dom/element.rs, line 1534 [r4] (raw file):
Is this preferred or is quirks_mode() == Quirks? This will allow LimmitedQuirks

---

components/script/dom/element.rs, line 1545 [r4] (raw file):
Same here.

---

Comments from the review on Reviewable.io

[izgzhen]

Review status: 0 of 2 files reviewed at latest revision, 4 unresolved discussions.

---

components/script/dom/element.rs, line 1534 [r4] (raw file):
It sounds sensible. I should take spec more literally.

---

Comments from the review on Reviewable.io

[asajeffrey]

@izgzhen would you like me to dive into the current code base some more, or hold off? It sounds like we're in agreement that the code is going to need quite a bit of re-architecting to move computation from the script thread to the layout thread.

[izgzhen]

@asajeffrey You idea sounds great. Hope I can be helpful to you.

[asajeffrey]

@izgzhen is this PR still active?

[izgzhen]

@asajeffrey Yes, I believe so. Sorry that I have been busy with GSoC proposal and coursework these days. Also, I am kind of waiting more comments so I procrastinated :(

If there is no major problem about "moving computation from the script thread to the layout thread", I am going to review the code and write some test cases this weekend.

[izgzhen]

@asajeffrey hi, I have investigated and experimented throughly this weekend, but the result turns out to be not very pleasing.

First, the origin.x and origin.y is implemented as window.ScrollX() and window.ScrollY() in PR #9824, which seems not to be what we want. Also, I take a look at the code about scrolling in compositor and layout, and it looks horribly complex here. pub scroll_offset: TypedPoint2D<LayerPixel, f32> field in components/compositing/compositor_layer.rs, line 46 seems related. I think I should somehow get the value from here and send it to the script thread. Also, I might need to call the interfaces there to perform setting on scrollTop and friends.

Second, a lot of other scrolling utilities are not supported in Element interface, such as scroll. It might not be a big problem now, but I think it is hard to implement one without referring to another.

As a conclusion, I think I am going to investigate a bit deeper and try to give a more complete solution. It is very rewarding process to understand and identify the problem, although there is not any fruit yet. I thin k we can leave this PR open here as a place for discussion and experimentation, but I am afraid it might linger a very long time. What is your idea?

[dlrobertson]

First, the origin.x and origin.y is implemented as window.ScrollX() and window.ScrollY() in PR #9824, which seems not to be what we want

get_scroll_area will return the scroll area. The origin being

the top left padding edge of the element when the element has its default scroll position...

Now that I'm looking at the spec again, there is a chance that our current implementation might need some attention once scroll is implemented due to "when the element has its default scroll position".

Second, a lot of other scrolling utilities are not supported in Element interface, such as scroll.

scroll is definitely not implemented yet, and will need to be implemented to correctly implement SetScroll(Top|Left). Maybe handle_scroll_event would be a good place to start? I don't know much about scrolling, but wouldn't mind looking into it more if you could use some help.

[izgzhen]

Here are some notes of my latest "discoveries":

First, as stated a week ago, the scroll_offset of extra_data field of CompositorData is what we want. As tested, the handle_scroll_event will trigger the updating of this field. Note that, the data is type parameter to an abstract Layer as defined in layers crate. However, the actual layer is identified by LayerId as declared in gfx_traits. As a result, we need two id, pipeline id and layer id, to get the right data from compositor layer.

Second, the compositor_thread defines some messages. The ScriptToCompositor message is one we are interested in. We can send the extended message from window, since it has a IpcSender<ScriptToCompositorMsg> field.

The final problem is how to relate element to such a layer. As tested with tests/html/simple-overflow-scroll.html, the document viewport's pipeline and the section element's pipeline is same, while they have different layer id: Document one is 0-FragmentBody-0, and section element one is 4734743072-FragmentBody-0. So we can assume that there is kind of relationship. Actually, the Fragment as defined in layout component has two methods: fn layer_id(&self) -> LayerId and fn layer_id_for_overflow_scroll(&self) -> LayerId. This is exactly what we want!

The above is idea for getting, and setting should be similar (calling handle_scroll_event seems feasible).

Up the implementation, I think I can reuse the UnioningFragmentScrollAreaIterator as contributed by @danlrobertson by adding a layer_id field. And back to window, the scroll_area_query can be inspect the layer_id from layout RPC, send with its own pipeline_id to the compositor thread (XXX: Hoping it is the right pipeline, I am kinda arbitrary here). Then, we can get the scroll_offset, which is the ScrollTop and ScrollLeft we want. This is a quick hack I think.

Some other consideration is to make a new fragment iterator getting solely LayerId, so it might be reused in further as well. And on the query side, we will define new queries as getter and setter for scroll_offset, and deal with further semantics inside the script thread.

Plus the existing hack to retrieve the computed value of overflow-x and overflow-y, this sounds a bit of engineering. Welcome comments!

UPDATE: I have tested the idea in this commit, but it is not working due to a invalid layer_id. It is a bit puzzling now.

[izgzhen]

In the new commits, I finally fetched the right layer_id and sent the scroll_offset back to the script thread. However, it still has two problems:

1. The correct layer_id is the first one in the process of iterating the fragments (74695da#diff-8002b77bfa2ea6411d84e780a9955b85R438). But this hack is based on observable test results, rather than reasoning.
2. The returned value (scroll_offset) has one border-width difference from WebKit and Chrome testing. But since this is an offset (i.e. a relative distance), I can't simply add the border-width on it. I suspect that the scroll_offset implemented in Servo previously has subtle semantics difference from spec.

Despite these issues, the current code is basically working as intended. Welcome reviewing.

[jdm]

Well done! 💃

[izgzhen]

Renamed, hope it looks better

---

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[izgzhen]

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

components/compositing/compositor.rs, line 723 [r14] (raw file):
I think that might complicate the interface. Maybe I should use a warn or panic like other branches in message handler?

---

Comments from Reviewable

[izgzhen]

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

components/script/dom/element.rs, line 1266 [r13] (raw file):
An infinite loop? Do you mean that root_element might not terminate?

---

Comments from Reviewable

[asajeffrey]

Getting there!

---

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

components/compositing/compositor.rs, line 723 [r14] (raw file):
You should at least emit a warning.

---

components/script/dom/element.rs, line 1266 [r13] (raw file):
We're defining scroll, and calling it recursively on the window, we need to make sure we're not calling it on the same object!

---

Comments from Reviewable

[izgzhen]

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

components/script/dom/element.rs, line 1266 [r13] (raw file):
Thanks for the reminder. The scroll is specific to window as an internal interface, logically it shouldn't call DOM API again (as I suppose), and practically the code didn't call DOM API either. This invocation is converging so I think there is no need to check.

---

Comments from Reviewable

[asajeffrey]

Reviewed 4 of 4 files at r15.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

Comments from Reviewable

[asajeffrey]

OK, I'm happy, ready for an r+? @danlrobertson look good to you?

[izgzhen]

Great, thanks for helping the review these days

---

Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

Comments from Reviewable

[dlrobertson]

Looks great! Excited to see what homu has to say!

[asajeffrey]

@bors-servo r+

[bors-servo]

📌 Commit 11b12f6 has been approved by asajeffrey

[bors-servo]

⌛ Testing commit 11b12f6 with merge 8d988f2...

[bors-servo]

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

[peterjoel]

@izgzhen This is some really impressive work!