Attach position:fixed elements to the minimum scale size viewport

RESOLVED FIXED in Firefox 67

Status

()

defect
P3
normal
RESOLVED FIXED
6 months ago
3 days ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

(Blocks 1 bug, Regressed 1 bug)

Trunk
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 wontfix, firefox67 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

6 months ago
No description provided.
Assignee

Comment 1

5 months ago

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

Assignee

Updated

5 months ago
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Assignee

Updated

4 months ago
Blocks: 1524865
Assignee

Comment 2

4 months ago

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.

Assignee

Comment 3

4 months ago

(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.

Assignee

Comment 4

4 months ago

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

Assignee

Comment 5

4 months ago
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
Attachment #9037122 - Attachment is obsolete: true
Assignee

Comment 6

4 months ago

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

Assignee

Comment 7

4 months ago

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

4 months ago
Pushed by hikezoe@mozilla.com:
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

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Updated

Last month
Regressions: 1549292
You need to log in before you can comment on or make changes to this bug.