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

overscroll code doesn't work properly anymore #19718

Open
paulrouget opened this issue Jan 8, 2018 · 6 comments
Open

overscroll code doesn't work properly anymore #19718

paulrouget opened this issue Jan 8, 2018 · 6 comments

Comments

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Jan 8, 2018

/cc @glennw

If window::window_rect() returns TypedRect::new(TypedPoint2D::new(0, 50), size), the page size is correct, but its position is wrong (not shifted down to 50px).

Here is a screenshot:

screen shot 2018-01-08 at 04 31 29

The whole page should be shifted down.

Could this be the consequence of what @mrobinson was mentioning here: servo/webrender#2159 (comment)

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Jan 8, 2018

STR:

Change this in /ports/:

diff --git a/ports/glutin/window.rs b/ports/glutin/window.rs
index f2f16b0600..e2dbe533c3 100644
--- a/ports/glutin/window.rs
+++ b/ports/glutin/window.rs
@@ -996,8 +996,10 @@ impl WindowMethods for Window {
     }

     fn window_rect(&self) -> DeviceUintRect {
-        let size = self.framebuffer_size();
-        let origin = TypedPoint2D::zero();
+        let mut size = self.framebuffer_size();
+        size.height -= 50;
+
+        let origin = TypedPoint2D::new(0, 50);
         DeviceUintRect::new(origin, size)
     }

Load this page:

<style>
  * {
    margin: 0;
    padding: 0;
  }
  div, p {
    position: absolute;
    top: 0;
    left: 0;
  }
  body { background: yellow; }
  div {
    width: 100%;
    height: 100%;
    background: red;
  }
  p {
    width: 100px;
    height: 100px;
    background: green;
  }
</style>

<div></div>
<p></p>

Good:

screen shot 2018-01-08 at 09 52 19

Bad:

screen shot 2018-01-08 at 09 44 38

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Jan 8, 2018

Bisecting tells me the cdc1635 is the issue.

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Jan 8, 2018

@mrobinson / @glennw does that help or do you also need me to bisect the webrender changes?

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Jan 8, 2018

Let me describe a bit more what the expected behavior is.

I'm not sure why we call that overscroll as it's not directly related to scrolling. We want to be able to draw outside of the DOM window:

The HTML page is usually clipped by the DOM window dimension (x:0;y:0 is the CSS top left corner of the page, but also where the page is clipped). But we might want to draw the HTML content outside of the DOM window. A typical example is the ability to see the background of the page below a toolbar. Here is Gif that shows the effect:

https://cloud.githubusercontent.com/assets/373579/23516132/9f437e2c-ff6c-11e6-8340-97c7de94b411.gif

(the page is painted below the toolbar as weel)

So we want to be able to clip an area that is larger than the DOM window.

To do so, in servo/webrender#951, we introduced the notion of inner_rect which represent the DOM window, and window_size the represent the region to actually clip.

What is important is to not mess up the layout. In the case of the toolbar example, we want a position:fixed;top:0 element to be painted under (in y coordinates) the toolbar, while the top scrolled-out content would be painted below (in z coordinates) the toolbar.

I hope this clarify the behavior.

Do not hesitate to ask for clarifications.

@jdm jdm added the A-gfx/rendering label Jan 8, 2018
@mrobinson
Copy link
Member

@mrobinson mrobinson commented Jan 23, 2018

@paulrouget I'm pretty sure I have tracked down the PR that caused this (servo/webrender#2043). Is this issue still relevant now that work on BrowserHTML is ceasing? If it is, I can try to fix it in the upcoming weeks.

@paulrouget
Copy link
Contributor Author

@paulrouget paulrouget commented Jan 29, 2018

Is this issue still relevant now that work on BrowserHTML is ceasing?

Yes. BrowserHTML doesn't use this feature. This is used by regular embedders.

I can try to fix it in the upcoming weeks.

It is not a high priority, but that would be great to get that fixed soon.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.