Open Bug 1462235 Opened 6 years ago Updated 1 year ago

Not recalculated the scroll amount of an element which has a child element that is set "position: sticky" and "height: 100%"

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

Tracking Status
firefox62 - affected

People

(Reporter: daisuke, Unassigned)

References

(Blocks 2 open bugs)

Details

(Keywords: testcase)

Attachments

(2 files, 1 obsolete file)

Attached file test.html (obsolete) —
I attach a test file.
In this test, there is one element ( .container ) which has two elements. One sets "position: sticky" and "height: 100% "( .sticky ). Another one sets "position: absolute" and "height: 500px" which is taller than .container.
Then, set the scrollTop to .container element by JavaScript so as to scroll down, set display: none to .absolute element after 3 seconds.
Because already there are no overflowing element, the scrollTop should be 0, however that was as it is. ( I have set the amount of scrollTop to textContent of .sticky element. Also, we can confirm when we scroll on .container element. ) 
However, if the height of .sticky sets less than 100% (e.g. calc(100% - 1px);), scrollTop will be 0, looks working well.
So, I guess, the calculation in case of using "position: sticky" is something wrong?
Blocks: 1456828
Let me update the test html since the height of .sticky already is calc(100% - 1px)
Also, I made it to easy understand a bit.

STRs:
* Open test-scroll.html
* Wait 1sec

Expected:
* Background color of result element will be lime and its text is "scrollTop: 0"

Actual:
* Background color of result element is red and its text is "scrollTop: 300"
  (This means, the scroll amount was not recalculated.)
Attachment #8976416 - Attachment is obsolete: true
Priority: -- → P3
This might really be a question for the CSS Working Group.  I'm not sure the spec says what should happen here right now.

But it looks like Firefox, Chrome, and Edge all do the same thing on the attached testcase, so that might be the easiest thing to standardize on (and maybe that's already what the spec requiress).  Safari does something different -- they match your expectations and return scrollTop:0 at the end.  But their implementation is older (the original one for this feature IIRC), so they might not have updated to what was specified.

I think the spec supports what we do, roughly. In particular:
 - The testcase's dynamic element-removal doesn't directly cause any changes to layout. It just so happens that *that removed element* is no longer the thing causing overflow, and now it's the sticky-positioned element that is the thing causing overflow + a scrollbar. And the spec does imply that this is fine -- i.e. a sticky-positioned element can be the thing that causes overflow. It says:
  # However, if sticky positioning causes an overflow: auto or overflow: scroll box
  # to have overflow, the user agent must allow the user to access this content
  # (at its offset position), which, through the creation of a scrolling mechanism,
  # may affect layout.

I think that's roughly consistent with what Firefox/Edge/Chrome are doing here.
Brian, you mentioned in email this seems concerning and we should fix it for 62 so as not to block shipping the feature in bug 1399830. Can you  help find an owner for this issue? We might want to also bump up the priority to P1.
Flags: needinfo?(bbirtles)
Daisuke is going to try another approach that works around this so that it no longer blocks 1456828. I'll clear that blocking dependency when he has the other approach up for review (so we know it works).
Flags: needinfo?(bbirtles)
Daniel, thanks for looking into this. I think the reason Daisuke suspect this was buggy is that the behavior is markedly different between when .stick has height: 100% vs height: calc(100% - 1px).

(That sort of sharp change in behavior is normally something we try to avoid in CSS so if this is working per spec, it's probably a spec bug.)
Attachment #8982586 - Attachment description: testcase 2, with calc(100% - 1px) on .sticky → testcase 2, with height:calc(100% - 1px) on .sticky
Attachment #8979827 - Attachment description: test-scroll.html → testcase 1, with height:100% on .sticky
It seems Edge & Chrome agree (and are internally consistent), comparing testcases 1 and 2.  Both Edge & Chrome end up with scrollTop=300 on testcase 1, and scrolltop=299 on testcase 2. 

In comparison, Firefox has a sharp difference in behavior, snapping to scrollTop=0 for testcase 2, as Brian noted. And Safari ends up with scrollTop=0 on both testcases (internally consistent like Chrome/Edge, but with a different behavior).

I think it's reasonable to say that our inconsistency seems to be a bug -- at least a webcompat & internal-consistency bug, though perhaps not an explicit deviation from the spec.  It's not clear to me that the spec really says what to do in this case, aside from requiring that the sticky thing must be reachable via scrollbars (which it is in all browsers right now).

The spec text in question is https://drafts.csswg.org/css-position/#sticky-pos
Daisuke, are you still working on this for 62?
Flags: needinfo?(dakatsuka)
Brian or Daisuke, is enough of this fixed in bug 1456828 that we don't need to worry about this for 62?
If there is a spec bug we still need to file, I think dholbert is going to do that.
Flags: needinfo?(bbirtles)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #9)
> Brian or Daisuke, is enough of this fixed in bug 1456828 that we don't need
> to worry about this for 62?
> If there is a spec bug we still need to file, I think dholbert is going to
> do that.

Hi Liz, 
As Brian commented in comment 4, we resolved by another approach.
So, this bug is no longer blocking for animation inspector which we are going to release in 62.
Flags: needinfo?(dakatsuka)
OK, thanks! 
 
Since this isn't blocking the feature and we aren't going to resolve the spec issue for 62, I won't track the bug.  If it doesn't need to be open, let's close it too.
Flags: needinfo?(bbirtles)
See Also: → 1598112
See Also: → 1414874

scrollTop is not updated.

It's not clear to me that the spec really
says what to do in this case, aside from requiring that the sticky thing
must be reachable via scrollbars (which it is in all browsers right now).

scrollHeight is also not updated. If div#abs-pos-to-be-removed is dynamically removed, then the scrollHeight of the div#scrolling-relpos-container should be modified accordingly.

http://www.gtalbot.org/BugzillaSection/Bug1462235-non-updated-DHTML-scroll-values-001.html

The failure in Firefox also happens with 'overflow: hidden':

http://www.gtalbot.org/BugzillaSection/Bug1462235-non-updated-DHTML-scroll-values-002.html

Firefox 78.15.0 ESR and Firefox 96.0a1 buildID=20211112092317 fail those 2 tests.

Chromium 90, Chromium 98 and Epiphany 3.38.2 (WebKitGTK 2.34.1) pass those 2 tests.


If div#abs-pos-to-be-removed overlaps the div#sticky and is dynamically removed, then Firefox also fails in updating scrollHeight of the div#scrolling-relpos-container:

http://www.gtalbot.org/BugzillaSection/Bug1462235-non-updated-DHTML-scroll-values-003.html

http://www.gtalbot.org/BrowserBugsSection/CSS3PositionedLayout/sticky/position-sticky-scrolled-remove-horiz-sibling.html

I am convinced I should add this test in this bug report. It is the horizontal axis correspondent/equivalent of tests included in this bug report.

The test checks the div#scrollingContainer's scrollWidth value after dynamically removing one of its children, the div#wideItem. The div#scrollingContainer's scrollWidth value should become 100px.

Severity: normal → S3
Keywords: testcase
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: