Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it possible to configure overscroll behavior #1314

Closed
paulrouget opened this issue May 31, 2017 · 22 comments
Closed

Make it possible to configure overscroll behavior #1314

paulrouget opened this issue May 31, 2017 · 22 comments

Comments

@paulrouget
Copy link
Contributor

paulrouget commented May 31, 2017

This issue is part of an effort to improve Servo's embedding story. See https://github.com/paulrouget/servoshell/projects/2

From the embedder perspective, we want to be able to configure and change the overscroll behavior.

Maybe something like this:

pub struct ViewOverscrollBehavior {
    top: OverscrollBehavior,
    right: OverscrollBehavior,
    bottom: OverscrollBehavior,
    left: OverscrollBehavior,
}

pub enum OverscrollBehavior {
    Enabled(OverscrollParameters),
    Disabled,
}

pub struct OverscrollParameters {
  minPanDistanceRatio: f32,
  springFiction: f32,
  springStiffness: f32,
  stopDistanceThreshold: f32,
  stopVelocityThreshold: f32,
}
@shinglyu
Copy link

@paulrouget I'm interested in this. Is overscroll already implemented in WR and we only need to add a way to turn it on and off, or do we have to implement the toggle AND the behavior?

@paulrouget
Copy link
Contributor Author

I'm interested in this.

Cool!

Is overscroll already implemented in WR

It is implemented, but it's very basic.

we only need to add a way to turn it on and off, or do we have to implement the toggle AND the behavior?

The toggle first. Behavior could be done later.

The important bit is that we want to configure, at anytime, which side support overscroll.

Here is an example: when the user overscrolls horizontally, the page is "stretched". On the left and on the right. But maybe the embedder wants, when the user overscrolls, to go back or forward in the history, instead of stretching the page. So when the page can go back, we want to disable the left stretching. When the page can go forward, we want to disable the right stretching. But that's up to the embedder. So we need a new WindowEvent that let us configure the overscroll behavior from the embedder.

Does it make sense?

@shinglyu
Copy link

Sounds useful! I'd be very grateful if you can point me to the related code to get me started faster.

@paulrouget
Copy link
Contributor Author

I wrote a proof of concept last year:

Servo side: paulrouget/servo@15c0c88

WR side: paulrouget@424f44b

The main part about scrolling is missing. Basically, this code sets the options. We need ClipScrollNode::scroll to use these options.

This draft uses the Browser API. We don't want that anymore. We want a new WindowEvent. Maybe WindowEvent::SetOverscrollBehavior(…). When caught by the compositor, this will call the webrender_api associated method.

Let me know if there's anything I can do to help.

@shinglyu
Copy link

Thanks so much!

@shinglyu
Copy link

@paulrouget In your old Browser API implementation, IIUC, the embedder set the option by getting the <iframe mozbrowser> element then set it like mozbrowser.setOverscrollOptions(...). If we discard there, how do we allow allow the embedder to set it? I looked around in your servoshell code and I believe it's something like this?

So something like

let event = WindowEvent::SetOverscrollBehavior(...);
self.events_for_servo.borrow_mut().push(event);

?

@paulrouget
Copy link
Contributor Author

Do not look at servoshell for this, but instead, servo's /ports/servo/. See how the embedder sends a Reload event: https://github.com/servo/servo/blob/ee95bda3bff01d4ca0e1987a0355d01238d60cf6/ports/glutin/window.rs#L1285

@shinglyu
Copy link

I have some prototype but I can test it because of servo/servo#17642, probably need to wait for it to be resolved. I really don't want to mess around with multiple versions of rustc myself.

@shinglyu
Copy link

@paulrouget I see that in your code you pass the PipelineId down, but If I'm sending the WindowEvent from glutin::Window, how do I get the PipelineId?

@shinglyu
Copy link

Let me rephrase that: If we are going to set the overscroll behavior in ports/glutin, how do we know which iframe to change? Do we change every pipeline we found in the current compositor? How does the embedder identify the iframe it wants to set and put it in the WindowEvent?

@paulrouget
Copy link
Contributor Author

paulrouget commented Jul 14, 2017

It was necessary in the past. It is not anymore as we don't want to set these options on iframes but on top level browsing contexts.

How does the embedder identify the iframe it wants to set and put it in the WindowEvent?

So when this event is sent to the servo, it should come with the TopLevelBrowsingContextId of the browser we are targeting.

This is not a thing we do yet. But I am working on exactly this at the moment.

You can either wait until servo/servo#17425 lands, or base your work on top my servo branch attach-pipeline-2.

Here you will see that WindowEvents can target a BrowserId, aka TopLevelBrowsingContextId, aka "Tab" aka "frame" :)

So you send the WindowEvent with a BrowserId (in the case of the glutin port, there's only one id, because tabs are not supported).

Once this event arrives in the compositor, you need to check if the current compositor frame_tree is the one associated to the BrowserId. If so, you need to update webrender.

If it is not, store the scroll options somewhere, and when the frame tree is sent to the compositor, you also need to send scroll options at the same time.

Does it make sense?

@shinglyu
Copy link

Thanks! That's very helpful.

Once this event arrives in the compositor, you need to check if the current compositor frame_tree is the one associated to the BrowserId. If so, you need to update webrender.

Is this because we have one compositor handling frame trees from all tabs/frames?

Also I have a side question, do you know how things in ports/glutin, ports/cef and broswer.html` fits together?

@paulrouget
Copy link
Contributor Author

Is this because we have one compositor handling frame trees from all tabs/frames?

The compositor (WR) knows about one frame_tree/browser at a time.
The constellation knows about all the frame_tree/browser.
The constellation changes the frame of the compositor via send_frame_tree.

do you know how things in ports/glutin, ports/cef and broswer.html` fits together?

glutin and cef are 2 different embedders. glutin port is a standalone application. cef port translates the libservo into the CEF interface.

browserhtml is a html page rendered by the glutin port. It has a special mode where iframes behave like top level browsing context, and are controlled via an API called the Browser API. But we plan to remove all of this. So it's safe to ignore browserhtml and the browser api for now.

@shinglyu
Copy link

I'm asking the last question because I was confused about how browser.html make tabbing possible

in the case of the glutin port, there's only one id, because tabs are not supported

So does browser.html not rely on the glutin code, or is it using one window only and create a feeling of tabs using iframe only?

@paulrouget
Copy link
Contributor Author

So does browser.html not rely on the glutin code

It uses the glutin port.

is it using one window only and create a feeling of tabs using iframe only?

Yes.

@shinglyu
Copy link

@paulrouget Thank you! Now I have a clear picture of how they fit together. How far are you from landing servo/servo#17425 ? I need to weight the effort and risk of whether to fork your branch now or wait. :)

@paulrouget
Copy link
Contributor Author

@shinglyu hopping to ask for a review today.

@shinglyu
Copy link

I'll wait then. Let me find some other bug to work on :)

@paulrouget
Copy link
Contributor Author

@shinglyu servo/servo#17425 has landed.

@shinglyu
Copy link

@paulrouget Thanks. I was blocked on servo/servo#18166 when I tried to get back to this bug. But Simon's suggestion seems to solved it.

@staktrace
Copy link
Contributor

See also #1612. There's some discussion about moving the scrolling implementation out of webrender proper into a separate crate.

@shinglyu
Copy link

Hmm I'm taking a vacation next week, so probably no meaningful progress soon. Sorry!

glennw pushed a commit to glennw/webrender that referenced this issue Apr 6, 2018
bors-servo pushed a commit that referenced this issue Apr 6, 2018
Remove overscroll code and api.

Fixes #2545.
Fixes #1762.
Fixes #1314.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2629)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants