Add support for scrollbars per Scroll Frame #2015

nc4rrillo · 2017-11-08T18:25:08Z

Hi,
This PR adds support for specifying scrollbars being enabled per Scroll Frame by adding an additional bool to define_scroll_frame.

This is a (albeit tiny) breaking API change, so I've bumped the WR API version. I hope I did that right :).
I also updated the scrolling sample to show off the new functionality.

This change is

nc4rrillo · 2017-11-08T20:19:43Z

There are two reftest failures I'm looking into related to the root scroll frame scrollbar being offset a bit.

I've tried to revert my change, and instead move the logic to add the scrollbar for other scroll frames into flatten_scroll_frame where it seems more appropriate, however its drawing under the content of the scroll frame now.

nc4rrillo · 2017-11-08T20:47:55Z

So I decided to keep the RendererOptions enable_scrollbars option around to control whether or not the root scroll frame gets a scrollbar, since it is created implicitly.

nc4rrillo · 2017-11-08T20:50:02Z

webrender/src/frame_builder.rs

@@ -458,6 +458,7 @@ impl FrameBuilder {
            &viewport_rect,
            content_size,
            ScrollSensitivity::ScriptAndInputEvents,
+            false,


Previously I set this to true, but it caused reftest failures, I'm wondering if its because of some special handling of the root scroll frame.

Yes. Root scroll frame scrollbars are handled automatically by the ENABLE_SCROLLBARS option. We don't want this turned on in Gecko anyhow.

nc4rrillo · 2017-11-08T22:22:34Z

webrender/src/frame.rs

@@ -128,6 +129,23 @@ impl<'a> FlattenContext<'a> {
            );
        }

+        // Draw scrollars for other scroll frames
+        for (clip_id, clip_node) in &self.clip_scroll_tree.nodes {


I'm thinking in flatten_root I should filter the clip scroll nodes by the current PipelineId, else we'll run into trouble with a nested Iframe?

Hrm. It seems like it would be a lot easier just to add the item when processing the scroll frame item during flattening. Otherwise I think that stacked scroll frames will have all their scrollbars on the top of the scene.

bors-servo · 2017-11-09T16:39:13Z

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

glennw · 2017-11-10T03:10:27Z

Apologies for not getting to this - will look at it on Monday if no-one else gets to it before me.

mrobinson · 2017-11-10T10:34:26Z

@nc4rrillo Sorry! I hadn't seen this PR when I made my recent scrollbar change. It looks like the two PRs are compatible, even though there is now a merge conflict.

nc4rrillo · 2017-11-11T16:42:07Z

@mrobinson No worries. I'll rebase my changes and update this PR.

nc4rrillo · 2017-11-11T17:44:01Z

So there is still an issue that's visible in the scrolling example, where if you scroll the outer scroll frame, the inner scrollbar isn't clipped by its parents bounds.

nc4rrillo · 2017-11-12T00:45:58Z

Here's what I mean by the last comment:

It only occurs if I scroll the outer scroll frame

nc4rrillo · 2017-11-12T02:24:52Z

Okay I fixed that issue, I think this is ready for review.

glennw · 2017-11-12T21:29:42Z

r? @mrobinson

bors-servo · 2017-11-13T00:03:13Z

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

mrobinson

Really sorry for the delay in reviewing this PR! I have added some suggestions below, but this is looking pretty good. This PR also needs to be rebased in order to remove the two extra commits that got lumped along with it.

mrobinson · 2017-11-14T17:37:24Z

webrender_api/src/display_list.rs

@@ -1162,6 +1162,7 @@ impl DisplayListBuilder {
        complex_clips: I,
        image_mask: Option<ImageMask>,
        scroll_sensitivity: ScrollSensitivity,
+        enable_scrollbars: bool


I think it would be better to use an enum for this argument. Just judging by the changes to the examples, it's already a little hard to tell what this argument does. I suggest something like

enum IframeScrollbars { Enabled, Disabled, }

mrobinson · 2017-11-14T17:37:24Z

webrender/src/frame.rs

@@ -128,6 +129,23 @@ impl<'a> FlattenContext<'a> {
            );
        }

+        // Draw scrollars for other scroll frames
+        for (clip_id, clip_node) in &self.clip_scroll_tree.nodes {


Hrm. It seems like it would be a lot easier just to add the item when processing the scroll frame item during flattening. Otherwise I think that stacked scroll frames will have all their scrollbars on the top of the scene.

mrobinson · 2017-11-14T17:37:24Z

webrender/src/frame_builder.rs

@@ -500,6 +502,7 @@ impl FrameBuilder {
            frame_rect,
            content_size,
            scroll_sensitivity,
+            enable_scrollbars


I'm not sure we need to pass this value to the ClipScrollNode. I think you can just add the scrollbar here as a child of the parent ClipScrollNode. You might need to do some special calculation to ensure that the scrollbar stays within the rectangle of the parent node.

mrobinson · 2017-11-14T17:37:24Z

webrender/src/frame_builder.rs

@@ -458,6 +458,7 @@ impl FrameBuilder {
            &viewport_rect,
            content_size,
            ScrollSensitivity::ScriptAndInputEvents,
+            false,


Yes. Root scroll frame scrollbars are handled automatically by the ENABLE_SCROLLBARS option. We don't want this turned on in Gecko anyhow.

nc4rrillo · 2017-11-14T20:43:04Z

@mrobinson I've tried to move creating the scrollbars to add_scroll_frame, however an issue I'm having is that the scrollbar gets drawn before the content of the scroll frame.

Here's a screen shot with debug alpha primitives turned on to demonstrate:

nc4rrillo · 2017-11-14T20:44:27Z

I believe this is because add_scroll_frame is called, and then the builder gets invoked for the children of the rest of the display items that make up the contents, which draws over what I've pushed into the frame builder.

One of the reasons why doing it in flatten_root worked was because flatten_items had already been called at that point.

nc4rrillo · 2017-11-14T21:37:03Z

Okay so my proposed fix is to do the following:

Introduce a pending_scroll_frames stack to FrameBuilder
On every add_scroll_frame, check if the stack contains a pending scroll frame, and pop it off and draw scrollbars for it
Push the current scroll frame to the stack.

This takes care of most cases, I add a final check to pop_stacking_context to pop what should be the final remaining pending scroll frame, and draw scrollbars for it.

My thinking is that this works around the there not being a specific PopScrollFrame display item, so by drawing the previous scroll frame's scrollbars on the next scroll frame's add_scroll_frame invocation, the drawing order is preserved and we avoid the stacked scroll frame issue you mentioned.

nc4rrillo · 2017-11-14T21:57:20Z

webrender/src/frame.rs

@@ -612,6 +594,9 @@ impl<'a> FlattenContext<'a> {
                    .clip_rect()
                    .translate(&reference_frame_relative_offset);
                let content_rect = item.rect().translate(&reference_frame_relative_offset);
+
+                let enable_scrollbars = IframeScrollbars::Enabled == info.enable_scrollbars;


I turned the enum into a bool here, but I could just as easily thread it all the way down into FrameBuilder if need be.

nc4rrillo · 2017-11-14T21:58:24Z

webrender/src/frame_builder.rs

@@ -129,6 +129,9 @@ pub struct FrameBuilder {
    /// parent new scroll nodes.
    reference_frame_stack: Vec<ClipId>,

+    parent_scroll_frame_id: Option<ClipId>,
+    pending_scroll_frame_stack: Vec<(ClipId, ClipId, LayerRect)>,
+


I doubt think this needs to be a Vec, I don't think there will be more than one pending scroll frame that hasn't drawn scrollbars at a time.

nc4rrillo · 2017-11-14T21:59:32Z

I pushed a commit to address your review and implement the proposed change I outlined above.

nc4rrillo · 2017-11-14T22:50:45Z

CI failures are because I need to update wrench

glennw · 2017-11-19T23:15:06Z

Looks like this needs a rebase, sorry!


        support font subpixel AA with content transforms


        Enable scrollbars per Scroll Frame

nc4rrillo · 2017-11-21T09:47:36Z

Rebased and bumped crate versions for wr, wr api, and wrench

glennw · 2017-11-23T02:33:48Z

r? @mrobinson

mrobinson · 2017-11-26T14:29:33Z

@nc4rrillo Sorry for not giving this as much attention as I should have. I have been rolling this around the back of my head for the past week or so. The approach you take, which is to wait until the display list flattening sees another add_scroll_frame is not going to work for the general case and definitely not for Servo (which pushes all ScrollFrames at the start of StackingContexts).

Instead of doing this now and then having to redo it in the future, I think the right approach here is to let the client control adding scrollbars completely. To me, the most obvious way to do this is to expose a scrollbar display item. This display item would, for the moment, be a rectangle with a height, width, and color. WebRender would be responsible for moving that item around in the ScrolFrame's parent just like for existing scrollbars.

Again, sorry for the delay and perhaps leading you down a dead end here.

nc4rrillo · 2017-11-26T15:15:07Z

No worries. I like your approach much more. I’m going to close this and work on that.

nc4rrillo changed the title ~~Add support for scrollbars per Scroll Frame~~ [WIP] Add support for scrollbars per Scroll Frame Nov 8, 2017

nc4rrillo reviewed Nov 8, 2017

View changes

nc4rrillo force-pushed the nc4rrillo:scrollbar-per-scrollframe branch 2 times, most recently from 1c5254f to 2ca7b1f Nov 11, 2017

nc4rrillo force-pushed the nc4rrillo:scrollbar-per-scrollframe branch from 2ca7b1f to b8e55e5 Nov 11, 2017

nc4rrillo force-pushed the nc4rrillo:scrollbar-per-scrollframe branch from b8e55e5 to 2fb4303 Nov 12, 2017

nc4rrillo changed the title ~~[WIP] Add support for scrollbars per Scroll Frame~~ Add support for scrollbars per Scroll Frame Nov 12, 2017

mrobinson requested changes Nov 14, 2017

View changes

nc4rrillo reviewed Nov 14, 2017

View changes

nc4rrillo force-pushed the nc4rrillo:scrollbar-per-scrollframe branch 2 times, most recently from 671be1d to f21e969 Nov 18, 2017

nc4rrillo force-pushed the nc4rrillo:scrollbar-per-scrollframe branch from f21e969 to 74e1209 Nov 21, 2017

nc4rrillo changed the base branch from master to blend-mode-clear Nov 21, 2017

nc4rrillo changed the base branch from blend-mode-clear to master Nov 21, 2017

nc4rrillo force-pushed the nc4rrillo:scrollbar-per-scrollframe branch 2 times, most recently from 9dfc841 to e9944a7 Nov 21, 2017

lsalzman and others added 2 commits Nov 14, 2017

support font subpixel AA with content transforms

deebe9c

Enable scrollbars per Scroll Frame

Loading status checks…

c48cfcb

nc4rrillo force-pushed the nc4rrillo:scrollbar-per-scrollframe branch from e9944a7 to c48cfcb Nov 21, 2017

nc4rrillo closed this Nov 26, 2017

servo / webrender

Add support for scrollbars per Scroll Frame #2015

Add support for scrollbars per Scroll Frame #2015

nc4rrillo commented Nov 8, 2017 •

edited

nc4rrillo commented Nov 8, 2017 •

edited

nc4rrillo commented Nov 8, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Nov 9, 2017

glennw commented Nov 10, 2017

mrobinson commented Nov 10, 2017

nc4rrillo commented Nov 11, 2017

nc4rrillo commented Nov 11, 2017

nc4rrillo commented Nov 12, 2017

nc4rrillo commented Nov 12, 2017

glennw commented Nov 12, 2017

bors-servo commented Nov 13, 2017

mrobinson left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nc4rrillo commented Nov 14, 2017

nc4rrillo commented Nov 14, 2017

nc4rrillo commented Nov 14, 2017 •

edited

This comment has been minimized.

This comment has been minimized.

nc4rrillo commented Nov 14, 2017

nc4rrillo commented Nov 14, 2017 •

edited

glennw commented Nov 19, 2017

nc4rrillo commented Nov 21, 2017

glennw commented Nov 23, 2017

mrobinson commented Nov 26, 2017

nc4rrillo commented Nov 26, 2017

servo / webrender

Join GitHub today

Add support for scrollbars per Scroll Frame #2015

Add support for scrollbars per Scroll Frame #2015

Conversation

nc4rrillo commented Nov 8, 2017 • edited

nc4rrillo commented Nov 8, 2017 • edited

nc4rrillo commented Nov 8, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Nov 9, 2017

glennw commented Nov 10, 2017

mrobinson commented Nov 10, 2017

nc4rrillo commented Nov 11, 2017

nc4rrillo commented Nov 11, 2017

nc4rrillo commented Nov 12, 2017

nc4rrillo commented Nov 12, 2017

glennw commented Nov 12, 2017

bors-servo commented Nov 13, 2017

mrobinson left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nc4rrillo commented Nov 14, 2017

nc4rrillo commented Nov 14, 2017

nc4rrillo commented Nov 14, 2017 • edited

This comment has been minimized.

This comment has been minimized.

nc4rrillo commented Nov 14, 2017

nc4rrillo commented Nov 14, 2017 • edited

glennw commented Nov 19, 2017

nc4rrillo commented Nov 21, 2017

glennw commented Nov 23, 2017

mrobinson commented Nov 26, 2017

nc4rrillo commented Nov 26, 2017

nc4rrillo commented Nov 8, 2017 •

edited

nc4rrillo commented Nov 8, 2017 •

edited

nc4rrillo commented Nov 14, 2017 •

edited

nc4rrillo commented Nov 14, 2017 •

edited