1873749 - {inc} Incremental layout doesn't match static layout, when adding a vertical scrollbar will trigger linewrapping which makes content tall enough to need that scrollbar

Status: NEW

(Core :: Layout: Scrolling and Overflow, defect, P3)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:

1. Make sure you're in an "Always Show Scrollbars" configuration.
2. Load attached testcase. (Make sure your mouse-cursor isn't hovering the cyan blocks in the testcase when it loads).
3. After the testcase has loaded, hover and then unhover a cyan block (just toggling some dynamic styles)

ACTUAL RESULTS:
The layout changes unexpectedly. In particular:

• The initial layout (in step 2) has a vertical scrollbar, with the cyan boxes stacked vertically ("LAYOUT 1")
• After you hover and unhover the cyan content, the vertical scrollbar disappears, and the cyan boxes end up stacked horizontally instead. ("LAYOUT 2")

EXPECTED RESULTS:
The initial and final layout should arguably be consistent.

This might not be something we care to do anything about; it's sort of an edge case, though it's one that we've been triggering in bug 1522442. It's a case where we're falling prey to the heuristics in GuessVScrollbarNeeded leading us in two different directions for the initial layout vs. for certain incremental layouts.

• When arriving at the initial layout ("LAYOUT 1"), we have no prior knowledge about whether a scrollbar will be needed, so we pessimistically start out using one, per this path which returns true:
  https://searchfox.org/mozilla-central/rev/6f90f50b7a32cc062ab755e0653b3d3f512fe3bd/layout/generic/nsGfxScrollFrame.cpp#1043-1048

  // Assume that there will be a scrollbar; it seems to me
  // that 'most pages' do have a scrollbar, and anyway, it's cheaper
  // to do an extra reflow for the pages that *don't* need a
  // scrollbar (because on average they will have less content).
  return true;
}

And with that scrollbar present, the testcase has to wrap (the two 50vw-wide boxes don't fit side-by-side with the scrollbar present), and that makes us overflow the viewport vertically, so we judge that the vertical scrollbar is in fact necessary, and we keep it.

• Then: after you hover the testcase to make the content smaller (which makes the scrollbar trivially-unnecessary) and then unhover to restore the original sizes, then we return to that same function, but now we take the following early-return since we're no longer in the initial reflow, and we return false (since mHasVerticalScrollbar is currently false), and we end up with "LAYOUT 2":
  https://searchfox.org/mozilla-central/rev/6f90f50b7a32cc062ab755e0653b3d3f512fe3bd/layout/generic/nsGfxScrollFrame.cpp#1021-1029

// If we've had at least one non-initial reflow, then just assume
// the state of the vertical scrollbar will be what we determined
// last time.
if (mHadNonInitialReflow) {
  // We only care about scrollbars that might take up space when trying to
  // guess if we need a scrollbar, so we ignore scrollbars only created to
  // scroll the visual viewport inside the layout viewport because they take
  // up no layout space.
  return mHasVerticalScrollbar && !mOnlyNeedVScrollbarToScrollVVInsideLV;

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)

This might not be something we care to do anything about; it's sort of an edge case, though it's one that we've been triggering in bug 1522442.

Safari 17.2.1 and Chrome 122dev (~latest versions) are interoperable with us on the ACTUAL RESULTS here, so this is a bug that's common to all browsers.

So: there's definitely no pressure to change behavior here, given that interop situation and the fact that this is pretty edge-casey.