728807 - "ASSERTION: We don't handle correct positioning of fixed frames with scrollbars in odd positions"

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

1. Set user_pref("layout.scrollbar.side", 3);
2. Load the testcase

Result:

###!!! ASSERTION: We don't handle correct positioning of fixed frames with scrollbars in odd positions: 'GetAbsoluteContainingBlock()->GetChildList().IsEmpty() || (offset.x == 0 && offset.y == 0)', file layout/generic/nsViewportFrame.cpp, line 265

The pref layout.scrollbar.side is handled by nsGfxScrollFrameInner::IsScrollbarOnRight:
http://hg.mozilla.org/mozilla-central/annotate/561771f01881/layout/generic/nsGfxScrollFrame.cpp#l2945

Comment 1

[Simon Montagu :smontagu]

Attachment: Patch

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 750312 [details] [diff] [review]
Patch

So my understanding of the meaning of mScrollPort is that GetActualScrollbarSizes is already correct.  (My understanding matches what GetScrollPosition() and GetLogicalScrollPosition() do, anyway.)

Is my understanding wrong somehow?  (If so, how do GetScrollPosition and GetLogicalScrollPosition work?)

Or is the problem here elsewhere?  (e.g., should ViewportFrame::AdjustReflowStateForScrollbars be returning top,right in RTL?)

Comment 3

[Simon Montagu :smontagu]

Attachment: Alternative patch

So like this?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 770682 [details] [diff] [review]
Alternative patch

So based on my investigation so far, it seems like the assertion here is correct:  ViewportFrame::AdjustReflowStateAsContainingBlock is behaving incorrectly when the scrollbar is on the right.  (Do we ship with the pref set that way in any cases?  If not, should we care?)

In particular, bug 844178 was about a dynamic change handling fast-path for top/left/right/bottom changes producing a different result from the normal codepath.  It seems that when the scrollbars are on the left, they *both* produce the wrong result, in that they consider scrollbars but act as though they're on the right.  I'll attach a testcase that should test both codepaths.

It seems like the thing to do is fix this function (it might be possible to fix both codepaths within ViewportFrame::AdjustReflowStateForScrollbars, though it might require changes to the callers of ViewportFrame::AdjustReflowStateAsContainingBlock (the one in nsViewportFrame for the normal path, and the one in nsCSSFrameConstructor for the RecomputePosition fast path).

The point of those functions is to change the area available to position:fixed frames so that their position is relative to the area inside the scrollbars.  Presumably this matches the spec and other browsers, though I didn't check either, and it is probably worth checking before investing too much energy in it.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase showing incorrect behavior with layout.scrollbar.side==3

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 728807 - Adjust containing block origin for fixed positioned elements.

The containing block rect which the fixed position elements are relative to
should exclude the scrollbar or scrollbar-gutter area. Current, we always
use (0,0), which makes fixed position elements draw behind the gutter area on
the left edge of the viewport.

This patch fixed the offset so that the containing block's origin is adjusted
when there is a scrollbar or scrollbar-gutter on the left edge of the viewport.

The "-001" test uses scrollbar-gutter: stable, and we already pass it without
this patch. The "-002" variant tests scrollbar-gutter: stable both-edges,
which requires this patch to pass.

Comment 8

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 728807 - Add a mozilla wpt to test scrollbar on the left edge of viewport.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/34fcdc11eb39
Adjust containing block origin for fixed positioned elements. r=layout-reviewers,emilio,dholbert
https://hg.mozilla.org/integration/autoland/rev/ebcc45e3aa11
Add a mozilla wpt to test scrollbar on the left edge of viewport. r=dholbert

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/44231 for changes under testing/web-platform/tests

Comment 11

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/34fcdc11eb39
https://hg.mozilla.org/mozilla-central/rev/ebcc45e3aa11

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot