1516377 - Attach position:fixed elements to the minimum scale size viewport

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Use the layout viewport size for position:fixed elements

FWIW, here is a patch that what Botond suggested me in bug 1520455.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

FWIW, here is a try with the fix;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4e7672cf2c7cd9c3b840a55315fa96fa2041b9f

As you can see, an added reftest along with the fix fails both on desktop and mobile, but the failure reasons are different. On desktop the reference image is not rendered properly, whereas on mobile the test image is not rendered properly. Possibly on desktop reftest-async-scroll doesn't work for the reference case.

As for mobile, if the height of the element, which is used for expanding the layout viewport, is greater than 1022px, the test passes. I am suspecting the browser toolbar affects reftest-async-scroll somehow. I will investigate bit more.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)

As for mobile, if the height of the element, which is used for expanding the layout viewport, is greater than 1022px, the test passes. I am suspecting the browser toolbar affects reftest-async-scroll somehow. I will investigate bit more.

Note that the reftest works fine on GeckoView as it is.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

I did give up the investigation why the reftest doesn't work on Fennec. I've decided to use reftest-async-scroll-y instead of reftest-async-scroll-x. Also I am going to skip the reftest on desktops.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d766485ce5c85fd01236c268399f9c0f6360a7a

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9037122 [details] [diff] [review]
Use the layout viewport size for position:fixed elements

># HG changeset patch
># Parent  56d6aff822a5376e149fcc03c0f2a961c891ccf3
>Bug 1516377 - Use the layout viewport for position:fixed elements. r?botond
>
>diff --git a/layout/base/PresShell.cpp b/layout/base/PresShell.cpp
>--- a/layout/base/PresShell.cpp
>+++ b/layout/base/PresShell.cpp
>@@ -10119,6 +10119,14 @@ nsPoint nsIPresShell::GetLayoutViewportO
>   return result;
> }
> 
>+nsSize nsIPresShell::GetLayoutViewportSize() const {
>+  nsSize result;
>+  if (nsIScrollableFrame* sf = GetRootScrollFrameAsScrollable()) {
>+    result = sf->GetScrollPortRect().Size();
>+  }
>+  return result;
>+}
>+
> void nsIPresShell::RecomputeFontSizeInflationEnabled() {
>   mFontSizeInflationEnabled = DetermineFontSizeInflationState();
> 
>diff --git a/layout/base/nsIPresShell.h b/layout/base/nsIPresShell.h
>--- a/layout/base/nsIPresShell.h
>+++ b/layout/base/nsIPresShell.h
>@@ -1696,6 +1696,7 @@ class nsIPresShell : public nsStubDocume
>   }
> 
>   nsPoint GetLayoutViewportOffset() const;
>+  nsSize GetLayoutViewportSize() const;
> 
>   virtual void WindowSizeMoveDone() = 0;
>   virtual void SysColorChanged() = 0;
>diff --git a/layout/generic/ViewportFrame.cpp b/layout/generic/ViewportFrame.cpp
>--- a/layout/generic/ViewportFrame.cpp
>+++ b/layout/generic/ViewportFrame.cpp
>@@ -258,15 +258,20 @@ nsRect ViewportFrame::AdjustReflowInputA
>                "We don't handle correct positioning of fixed frames with "
>                "scrollbars in odd positions");
> 
>-  // Layout fixed position elements to the visual viewport size if and only if
>+  // Layout fixed position elements to the layout viewport size if and only if
>   // it has been set and it is larger than the computed size, otherwise use the
>   // computed size.
>   nsRect rect(0, 0, aReflowInput->ComputedWidth(),
>               aReflowInput->ComputedHeight());
>   nsIPresShell* ps = PresShell();
>-  if (ps->IsVisualViewportSizeSet() &&
>-      rect.Size() < ps->GetVisualViewportSize()) {
>-    rect.SizeTo(ps->GetVisualViewportSize());
>+  if (ps->IsVisualViewportSizeSet()) {
>+    if (rect.Size() < ps->GetVisualViewportSize()) {
>+      rect.SizeTo(ps->GetVisualViewportSize());
>+    }
>+    nsSize layoutViewportSize = ps->GetLayoutViewportSize();
>+    if (rect.Size() < layoutViewportSize) {
>+      rect.SizeTo(layoutViewportSize);
>+    }
>   }
>   return rect;
> }
>diff --git a/layout/painting/nsDisplayList.cpp b/layout/painting/nsDisplayList.cpp
>--- a/layout/painting/nsDisplayList.cpp
>+++ b/layout/painting/nsDisplayList.cpp
>@@ -969,11 +969,16 @@ nsDisplayListBuilder::OutOfFlowDisplayDa
>         nsRect(nsPoint(0, 0), aFrame->GetParent()->GetSize());
> 
>     nsIPresShell* ps = aFrame->PresShell();
>-    if (ps->IsVisualViewportSizeSet() &&
>-        dirtyRectRelativeToDirtyFrame.Size() < ps->GetVisualViewportSize()) {
>-      dirtyRectRelativeToDirtyFrame.SizeTo(ps->GetVisualViewportSize());
>-    }
>-
>+    if (ps->IsVisualViewportSizeSet()) {
>+      if (dirtyRectRelativeToDirtyFrame.Size() < ps->GetVisualViewportSize()) {
>+        dirtyRectRelativeToDirtyFrame.SizeTo(ps->GetVisualViewportSize());
>+      }
>+
>+      nsSize layoutViewportSize = ps->GetLayoutViewportSize();
>+      if (dirtyRectRelativeToDirtyFrame.Size() < layoutViewportSize) {
>+        dirtyRectRelativeToDirtyFrame.SizeTo(layoutViewportSize);
>+      }
>+    }
>     visible = dirtyRectRelativeToDirtyFrame;
>   }
> #endif

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Why did I paste the patch as a comment???

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1516377 - Use the layout viewport for position:fixed elements if the viewport is larger than. r=botond

Both of reftests in this commit are based on an exmaple [1] in the Viewports
Explainer written by David Bokan.

position-fixed-out-of-view.html fails without the fix because the position:fixed
element is rendered at the right edge of the visual viewport so that it's
visible in the first place.

position-fixed-on-minimum-scale-size.html does NOT fail without the fix either
because the position:fixed element sticks at the bottom edge of the visual
viewport so that it still be there even after the visual viewport offset has
been changed.

[1] https://github.com/bokand/bokand.github.io/blob/master/web_viewports_explainer.md#chrome-2

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96d77e9c3f29
Use the layout viewport for position:fixed elements if the viewport is larger than. r=botond

Comment 9

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/96d77e9c3f29