915302 - sticky positioned elements should stay within a containing block established by their scroll container

Status: NEW

(Core :: Layout, defect)

Description

[Corey Ford [:coyotebush]]

Attachment: testcase

In StickyScrollContainer::ComputeStickyLimits I have

>   if (cbFrame != scrolledFrame) {
>     *aContain->SetRect(...

but the scroll container can establish the sticky element's containing block (the scrolled content, not the outer scrollable frame, of course), and the sticky element should stay inside that.

I believe WebKit's behavior on this test case is correct.

Comment 1

[Corey Ford [:coyotebush]]

Oh, right, the main problem is that the result of mScrollFrame->GetScrolledFrame()->GetRect() and friends is only as big as the outer scrollframe, not the scrolled content. (And with no intervening block-level element, aFrame->GetContainingBlock() == mScrollFrame->GetScrolledFrame())

Comment 2

[Corey Ford [:coyotebush]]

Which is a reasonable definition of containing block for most purposes, but here I guess using the scrollable overflow area might be more correct (but still maybe not quite).

Comment 3

[:kip (Kearwood Gilbert)]

Attachment: v1 fix for Bug 915302

Please review the proposed fix.  In particular, please advise if I am using the best function (GetScrollableOverflowRectRelativeToSelf) for the required coordinate system.

Comment 4

[Corey Ford [:coyotebush]]

That's probably the way to do it. But the extra Deflate steps are still necessary (so should be outside the if statement).

I also have wondered whether we might even want to *unconditionally* use the containing block's (continuations') overflow rect. Probably not, though.

This issue was brought up in a recent discussion on www-style (which I intend to respond to soon):
http://lists.w3.org/Archives/Public/www-style/2014Mar/0013.html
in particular
> - Should the ReleasePosition be computed any differently if the ContainingBox is also the ScrollerBox?

Comment 5

[Corey Ford [:coyotebush]]

(In reply to Corey Ford [:corey] from comment #4)
> (so should be outside the if statement).

Er, nevermind, misread your code structure change.

Comment 6

[:kip (Kearwood Gilbert)]

Attachment: V1 Mochitest for Bug 915302

Here is a Mochitest to confirm the fix.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8386256 [details] [diff] [review]
V1 Mochitest for Bug 915302

A few drive-by stylistic nits:

>+++ b/layout/generic/test/mochitest.ini
>@@ -107,8 +107,9 @@ support-files = page_scroll_with_fixed_p
> skip-if = toolkit == 'android'
> [test_plugin_position.xhtml]
> [test_selection_expanding.html]
> support-files = selection_expanding_xbl.xml
> [test_selection_splitText-normalize.html]
> [test_selection_touchevents.html]
> [test_taintedfilters.html]
> support-files = file_taintedfilters_feDisplacementMap-tainted-1.svg file_taintedfilters_feDisplacementMap-tainted-2.svg file_taintedfilters_feDisplacementMap-tainted-3.svg file_taintedfilters_feDisplacementMap-tainted-ref.svg file_taintedfilters_feDisplacementMap-untainted-ref.svg file_taintedfilters_feDisplacementMap-untainted-1.svg file_taintedfilters_feDisplacementMap-untainted-2.svg file_taintedfilters_red-flood-for-feImage-cors.svg file_taintedfilters_red-flood-for-feImage-cors.svg^headers^ file_taintedfilters_red-flood-for-feImage.svg
>+[test_bug915302.html]

I think the list in mochitest.ini is supposed to be alphabetically ordered, so this probably wants to be added up above with the rest of the "[test_bugNNNN]" lines.

>diff --git a/layout/generic/test/test_bug915302.html b/layout/generic/test/test_bug915302.html
>+<div id="testscroll" style="overflow: hidden; height: 200px; border: 10px solid black">
>+    <div id="teststicky" style="position: -webkit-sticky; position: sticky; top: 180px; height: 50px; background: black"></div>

(I'm not sure that -webkit prefixing is useful in our mochitests, since webkit/blink-based browsers don't support mochitests. Maybe this is useful if someone copypastes from this test code out into an actual live testcase, though?)

>+is(document.getElementById("teststicky").getBoundingClientRect().top - document.getElementById("testscroll").getBoundingClientRect().top, 160, "Position of sticky element is offset from the containing block by the expected amount.");

Gecko style (particularly in layout) is to (try to) keep lines less than 80 chars, so that they can be displayed nicely in side-by-side editors on a reasonably-sized laptop screen).

Could you rewrap it so that it doesn't protrude past 80 characters? e.g. add a newline before the quoted message, maybe split the message into two quoted lines (concatenated with +) if necessary, and perhaps split the arithmetic out into a helper-variable, before the "is()" check, if that helps.

Comment 8

[:kip (Kearwood Gilbert)]

Attachment: V2 Mochitest for Bug 915302

Updated with feedback from dholbert

Comment 9

[Corey Ford [:coyotebush]]

By the way, do we need a mochitest for this, or would a reftest added to layout/reftests/position-sticky suffice?

Comment 10

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

Comment on attachment 8386393 [details] [diff] [review]
V2 Mochitest for Bug 915302

Yeah, agree that this should be a reftest rather than a mochitest.

Comment 11

[:kip (Kearwood Gilbert)]

Can be implemented as a reftest, new patch inbound..

Comment 12

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

Comment on attachment 8386218 [details] [diff] [review]
v1 fix for Bug 915302

>-  if (cbFrame != scrolledFrame) {
>+  if (cbFrame == scrolledFrame) {
>+    // The scroll frame is the containing-block element of the sticky element.
>+    // Use the scrollable overflow size as the containing block limits.
>+    *aContain = nsRect(nsPoint(0,0),
>+              scrolledFrame->GetScrollableOverflowRectRelativeToSelf().Size());

So a few thoughts here:
 * it's not clear to me if the size we want is scrolledFrame->GetScrollableOverflowRectRelativeToSelf() or if it's mScrollFrame->GetScrolledRect().  Their sizes (ignoring origins) differ in that some overflow can't be reached when you scroll, such as overflow above the top of the scroll frame, or for lef-to-right (right-to-left) directionality, overflow to the left (right).  I strongly suspect you want the latter, though, but I haven't fully wrapped my head around what this is changing yet.
 * it's not immediately obvious to me that nsPoint(0, 0) is the right origin, nor is it yet obvious that it's wrong.  Did you test with margin, border, and padding?  (Also, space after the comma, please.)

Comment 13

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

Oh, and it's definitely worth testing with:
 * overflow above the top
 * overflow to both the left and right, with both ltr and rtl directionality

(I suspect that the nsPoint(0, 0) may well be wrong for rtl directionality, but I also suspect GetScrolledRect() might give you the right thing, or something more easily convertible to it.)

Comment 14

[:kip (Kearwood Gilbert)]

Attachment: V3 test for Bug 915302

The mochitest has been replaced with a reftest.

Comment 15

[:kip (Kearwood Gilbert)]

Origin from GetScrollableOverflowRectRelativeToSelf() returned as (60, 60) with a 10px margin applied to the containing scroll frame.  Am hoping there is a cleaner method than zero-ing out the origin to get the scrollable overflow rect in the needed coordinate space.  I am trying some permutations of the attached reftest to see if the ReleasePosition is being calculated intuitively.

Comment 16

[Corey Ford [:coyotebush]]

(In reply to Corey Ford [:corey] from comment #0)
> >   if (cbFrame != scrolledFrame) {
> >     *aContain->SetRect(...

> I believe WebKit's behavior on this test case is correct.

Actually, now that I think about it, WebKit's behavior matches what we would get by simply removing that if check (i.e. it uses the outer size of the scroll container, and *not* the size of the scrolled contents, which in my original testcase was only 50px).

Sorry for overlooking that, but IMO using the scrolled contents still seems most sensible. We probably ought to wait for a bit more discussion on www-style, though:
http://lists.w3.org/Archives/Public/www-style/2014Mar/0141.html

Comment 17

[:kip (Kearwood Gilbert)]

Attachment: testcase that illustrates different releaseposition handling in Gecko vs Webkit

I have added a testcase, which illustrates the difference in releaseposition handling in WebKit vs Gecko.

Comment 18

[Corey Ford [:coyotebush]]

Per a little discussion with Kip, it sounds like GetScrolledRect() is the way to go, but its result is at minimum as large as the scroll container, even if the content doesn't overflow (so 200px tall in my original testcase). That also seems reasonable, so maybe we will match WebKit on the original testcase, but I think we shouldn't try to do so in general.

Comment 19

[:kip (Kearwood Gilbert)]

Attachment: v2 fix for Bug 915302

Updated fix to use GetScrolledRect()

Comment 20

[Corey Ford [:coyotebush]]

I'm now kind of ambivalent as to which is the better behavior here, but I'll follow up with www-style soon to see whether others have an opinion on this matter.

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 8386484 [details] [diff] [review]
V3 test for Bug 915302

A few drive-by nits on the reftest patch:

>+++ b/layout/reftests/position-sticky/container-is-scroll-1-ref.html
>@@ -0,0 +1,35 @@
>+<!DOCTYPE html>
>+<!-- Any copyright is dedicated to the Public Domain.
>+   - http://creativecommons.org/publicdomain/zero/1.0/ -->
>+<html>
>+  <head>
>+    <title>CSS Test: Sticky Positioning - Containing block is scrollable</title>

Title for the reference case should be something like "CSS Reference" instead of being the same as the testcase.

>+      #testcb {
>+        height: 200px;
>+      }
>+    
  ^^^^
nit: drop whitespace-on-blank-line. (both in the testcase and the reference case)

>+      #teststicky {
>+        position: sticky;
>+        top: 180px;
>+        height: 50px;
>+        background: green
>+      }

nit: where possible, it's nice to have reference cases not depend on the feature being tested (position:sticky in this case), so that
 (A) the reference reliably shows the "expected" rendering, even if the feature is turned off or unimplemented
 (B) if the reference depends on the feature, there's a higher possibility that we could accidentally regress things such that *both* the testcase and the reference case change their rendering, and hence the test continues to "pass".

In particular, if I view the testcase in Firefox Release (which has sticky disabled), the test "passes" spuriously (with the green bar rendering at the top instead of the bottom, in both cases).

I'd suggest that you change this to make #teststicky use "margin-top: 150px", or (alternately) add a spacer div with "height:150px" above #teststicky.  Then, you can should drop "overflow:hidden" and "position:sticky" and the "#testcb" div (and anything else that ends up being unnecessary).

Comment 22

[:kip (Kearwood Gilbert)]

Attachment: V4 test for Bug 915302

Updated based on feedback from dholbert in Comment 21

Comment 23

[:kip (Kearwood Gilbert)]

Attachment: v3 fix for Bug 915302

Corrected indentation and wrapping

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 8392048 [details] [diff] [review]
V4 test for Bug 915302

Thanks for fixing up the reftest! Looks good, assuming that this is the behavior that the CSSWG agree[s|d] on.

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 8392050 [details] [diff] [review]
v3 fix for Bug 915302

>+  if (cbFrame == scrolledFrame) {
>+    // The scroll frame is the containing-block element of the sticky element.

s/containing-block element/containing block/

>+    // Use the scrollable overflow size as the containing block limits.
>+    *aContain = mScrollFrame->GetScrolledRect();

This seems reasonable, though I don't know enough about GetScrolledRect() to be sure whether it's actually what we want here.

Note that the comment for GetScrolledRect() in nsGfxScrollFrame has some caveats ("This should only be called when...") and a mention of special behavior in RTL content ("allows scrolling down and to the left fornsHTMLScrollFrames with RTL directionality.")

Source: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.h#227

I'm not sure if that has any implications for this, but it'd definitely be worth adding a test with sticky content that's getting pushed off of the other sides of the scroll port (top, right, left), plus a reftest with content getting pushed off of the right/left of a document with <html dir="rtl"> to exercise the RTL scrolling behavior.

These could all be part of the same reftest, if you like (or perhaps split into two testcases, one LTR and one RTL), and each test would have a series of 4 small (e.g. width: 20px;height:20px) scrollable frames, with each one having a single sticky-positioned child with "height: 10px; width: 10px; [side]:15px" for some side.

Having those tests would increase my confidence that this is correct :) though I'd still probably defer to someone who's more familiar with scroll frames (maybe roc or tn?) for officially signing off on this. Or I'd need to do a bit more research myself, to increase my confidence further.

Comment 26

[:kip (Kearwood Gilbert)]

Attachment: V4 fix for Bug 915302

Small comment text change based on Comment 25

Comment 27

[:kip (Kearwood Gilbert)]

Attachment: V5 test for Bug 915302

Reftests now include sticky elements attached to all 4 sides as well as with rtl direction.

Comment 28

[Daniel Holbert [:dholbert]]

Comment on attachment 8392625 [details] [diff] [review]
V5 test for Bug 915302

Thanks for adding these! Just a few nits, the first two of which potentially affect all 4 test files -- might be easiest to address them by opening the patch file itself in an editor and doing a search-and-replace.

>+      #teststicky_c {
>+        width: 50px;
>+        height: 20px;
>+        margin-left: 150px;
>+        background: red;
>+      }

nit: red has special implications for reftests -- if any red is visible, it implies that the test has failed, to humans viewing the testcase.  While the harness doesn't enforce this, it's a rule-of-thumb that other organizations' testsuites use (in particular, the CSSWG testsuite).  It matters for our tests because we ideally like our tests to be importable into testsuites like that.)

This means it's best to avoid using red in testcases, *except* when you really want it to indicate failure.

So, please replace red with e.g. purple or pink in all of these files.

>diff --git a/layout/reftests/position-sticky/container-is-scroll-1.html b/layout/reftests/position-sticky/container-is-scroll-1.html
>+    <div id="testscroll">
>+      <div id="teststicky_a"></div>
>+      <div id="teststicky_c"></div>
>+      <div id="teststicky_d"></div>
>+      <div id="teststicky_b"></div>

Seems a bit strange that these are out of order in the testcases. ("b" being at the end)  If they have to be in this order for some reason, maybe rename them to _top _bottom _right _left, so there's less of an expected ordering here?

>diff --git a/layout/reftests/position-sticky/container-is-scroll-2.html b/layout/reftests/position-sticky/container-is-scroll-2.html
>+      body {
>+        direction: rtl;
>+      }
>+      #testscroll {
>+        direction: rtl;

direction is an inherited-by-default property, so no need to specify it on #testscroll here.

>diff --git a/layout/reftests/position-sticky/reftest.list b/layout/reftests/position-sticky/reftest.list
>+== container-is-scroll-1.html container-is-scroll-1-ref.html

Looks like you need to add a line to the manifest for the second test.

r=me on the tests with the above. Thanks!

Comment 29

[:kip (Kearwood Gilbert)]

Attachment: V6 test for Bug 915302, r=dholbert

Updated with feedback from Comment 28

Comment 30

[Daniel Holbert [:dholbert]]

Comment on attachment 8392111 [details] [diff] [review]
V4 fix for Bug 915302

Per comment 25, this looks good, but I'm not familiar enough with scroll frames & GetScrolledRect()'s assumptions/guarantees enough to be sure about r+'ing this.

From a brief hg blame check, though, it looks like roc & dbaron were involved with adding/modifying GetScrolledRect at various points in time; one of them should probably be the one to review this.

Comment 31

[:kip (Kearwood Gilbert)]

I spoke to :dbaron regarding the use of GetScrolledRect() and confirmed that it is safe to use in this case as ReflowContents() is called within nsHTMLScrollFrame::Reflow prior to calling ScrollFrameHelper::UpdateSticky().

Comment 32

[:kip (Kearwood Gilbert)]

Patch submitted to try:

https://tbpl.mozilla.org/?tree=Try&rev=4e9e51078b57

Comment 33

[Corey Ford [:coyotebush]]

Has there been some more discussion about whether this is actually what we want to do? I was still hoping for feedback from the CSSWG.

Comment 34

[:kip (Kearwood Gilbert)]

(In reply to Corey Ford [:corey] from comment #33)
> Has there been some more discussion about whether this is actually what we
> want to do? I was still hoping for feedback from the CSSWG.

There has been no further feedback since your last question:

http://lists.w3.org/Archives/Public/www-style/2014Mar/0310.html

Comment 35

[:kip (Kearwood Gilbert)]

(In reply to :kip (Kearwood Gilbert) from comment #34)
> (In reply to Corey Ford [:corey] from comment #33)
> > Has there been some more discussion about whether this is actually what we
> > want to do? I was still hoping for feedback from the CSSWG.
> 
> There has been no further feedback since your last question:
> 
> http://lists.w3.org/Archives/Public/www-style/2014Mar/0310.html

Please advise if further discussion is still required from the CSSWG before this can be landed.

Comment 36

[Corey Ford [:coyotebush]]

Sorry for holding this up. And, sorry for my initial mischaracterization of WebKit's behavior that led me to think that by making this change we'd match WebKit.

The behavior introduced by this change might be useful, but I'm primarily worried that it would be difficult to spec -- does GetScrolledRect() correspond to anything defined in CSS? While I guess we don't need input from other implementors as such, I think you and dbaron should consider that before landing.

Comment 37

[:kip (Kearwood Gilbert)]

Additional experimentation with WebKit seems to indicate that the containing block is established by the scroll frame's viewport rather than the scrolled rect.  This causes the differences seen in the attached testcase, "testcase that illustrates different releaseposition handling in Gecko vs Webkit".

Comment 38

[:kip (Kearwood Gilbert)]

(In reply to :kip (Kearwood Gilbert) from comment #37)
> Additional experimentation with WebKit seems to indicate that the containing
> block is established by the scroll frame's viewport rather than the scrolled
> rect.  This causes the differences seen in the attached testcase, "testcase
> that illustrates different releaseposition handling in Gecko vs Webkit".

The result will match Webkit in both of the attached testcases by replacing within the patch:

  *aContain = mScrollFrame->GetScrolledRect();

With:

  *aContain = nsLayoutUtils::
    GetAllInFlowRectsUnion(cbFrame, aFrame->GetParent(),
                           nsLayoutUtils::RECTS_USE_CONTENT_BOX);

  *aContain += mScrollPosition;

Comment 39

[:kip (Kearwood Gilbert)]

Attachment: V5 (Alternate) fix for Bug 915302

This is an alternate fix, which uses the content box of the scroll frame as the containing block limits when the containing block is the scroll frame.  This results in behavior more consistent with Webkit

Comment 40

[:kip (Kearwood Gilbert)]

Comment on attachment 8414815 [details] [diff] [review]
V5 (Alternate) fix for Bug 915302

Pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=dc60b9acc9ff

Comment 41

[Corey Ford [:coyotebush]]

Do you think that's useful behavior, other than in that it matches WebKit's implementation? (To clarify, my opinion is that matching WebKit would have helped justify the GetScrolledRect() approach, but that it doesn't seem sufficient to justify this one.)

Comment 42

[:kip (Kearwood Gilbert)]

Perhaps this behavior may be useful in cases such as when you want to show a ruled line along the edges of a horizontal and vertical scrolling box.

Comment 43

[:kip (Kearwood Gilbert)]

Attachment: Bi-Axis, sticky rulers (Demonstrates that fixing this bug is not needed for this feature)

Bi-Axis, sticky rulers (Demonstrates that fixing this bug is not needed for this feature)

Comment 44

[:kip (Kearwood Gilbert)]

(In reply to :kip (Kearwood Gilbert) from comment #42)
> Perhaps this behavior may be useful in cases such as when you want to show a
> ruled line along the edges of a horizontal and vertical scrolling box.

I have attached a quick example that indicates that in-fact this patch would not be necessary for things such as ruled lines.

I can not think of other cases where this would have a benefit other than simply matching WebKit's behavior.

Comment 45

[Corey Ford [:coyotebush]]

So I guess we should keep our current behavior and close this bug, but leave the question open with the CSSWG. This situation ought to have standardized behavior eventually.

Comment 46

[:kip (Kearwood Gilbert)]

This bug has been set to no longer block Bug 916315.

This bug has been kept open until a decision can be made on which behavior should be followed in this case, rather than leaving the spec as "undefined behavior".

Comment 47

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

I'm inclined to agree that the WebKit behavior doesn't make much sense.  In all other cases, the containing block position is position that is anchored to the inside of the scrolling region; making it anchored to the outside of the scrolling region in this one case seems odd, and doesn't fit with the original reason for checking against the containing block in the first place (which is to keep a header visible while it's relevant).

Using the scrolled rect of the scrollable area in this case at least fits with that purpose, and seems reasonable, at least if I'm understanding the behavior correctly.  But it also seems reasonable to stick with our current behavior.

Sticking to our current behavior until the spec gets sorted out seems reasonable, and I agree (as we discussed earlier today) that this doesn't need to block enabling sticky.

Comment 48

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

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