Closed Bug 1516377 Opened 6 years ago Closed 6 years ago

Attach position:fixed elements to the minimum scale size viewport

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox66 --- wontfix
firefox67 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1516377 - Use the layout viewport for position:fixed elements if the viewport is larger than. r=botond
(deleted), text/x-phabricator-request
Details
No description provided.

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

Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Blocks: 1524865

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.

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

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

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

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

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

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

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Regressions: 1549292
Regressions: 1563649
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: