Closed Bug 1590247 Opened 2 years ago Closed 3 months ago

Consider optimizing changes from/to overflow: hidden better.

Categories

(Core :: Layout, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
95 Branch
Tracking Status
firefox95 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Right now they always have to reframe, because we optimize the scrollbars away for overflow: hidden / scrollbar-width: none.

This causes a lot of work when you toggle overflow a lot. For example, the Youtube notification popup goes from overflow: hidden to overflow: auto on hover, which is slow if you have a ton of notifications.

I think a good approach to preserve the optimization but avoid silly work would be something like what we do for the root element but conditionally.

For example. If you get a frame with overflow: hidden / scrollbar-width: none, we don't create scrollbars (as we do now). When you toggle to other sorts of scrollable overflow, we reframe, and create scrollbars. But from then on we don't need to reframe at all. We have scrollbars and can handle all scrollable overflow like we do for the viewport.

Is there any reason why this wouldn't work? Seems like extending ScrollbarChange to deal with this should be somewhat straight-forward.

Daniel, wdyt about this, since you implemented the initial implementation of bug 1344398?

Flags: needinfo?(dholbert)

So with overflow: hidden we have a scrollframe, but just no scrollbars? And instead we could have a scrollframe but hidden scrollbars on the scrollframe and just unhide if it changes to "auto"? And we'd still reframe on changes to/from "visible"?

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #2)

So with overflow: hidden we have a scrollframe, but just no scrollbars? And instead we could have a scrollframe but hidden scrollbars on the scrollframe and just unhide if it changes to "auto"? And we'd still reframe on changes to/from "visible"?

Yes, that'd be the proposal. We already need to handle the "hidden overflow but has scrollbars" for the viewport. And yeah, changes from visible / -moz-hidden-unscrollable would reframe to create the scrollframe and all that dance.

The proposal sounds reasonable to me.

Flags: needinfo?(dholbert)

Over to the queue then.

Flags: needinfo?(emilio)
Blocks: 1735451
Assignee: nobody → emilio
Flags: needinfo?(emilio)

This prevents jank when switching from overflow: auto -> hidden or such.

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/57193e853f39
Factor out ScrollbarChange handling. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/9b1d970ca9c2
Don't reframe scrollable frames if we already have all needed anonymous content. r=dholbert

This was pre-existing but caught by some scrollbar-width tests. A
test-case that reproduces on current nightly:

data:text/html,<html style="overflow: scroll; scrollbar-color: red red; scrollbar-width: thin">

Toggling the scrollbar-width declaration on devtools doesn't change
the effective scrollbar width. Ensure we correctly reflow the scrollbars
in this case.

Attachment #9245719 - Attachment description: Bug 1590247 - Factor out ScrollbarChange handling. r=#layout-reviewers! → Bug 1590247 - Factor out ScrollbarChange handling. r=dholbert
Attachment #9245715 - Attachment description: Bug 1590247 - Don't reframe scrollable frames if we already have all needed anonymous content. r=dholbert! → Bug 1590247 - Don't reframe scrollable frames if we already have all needed anonymous content. r=dholbert
Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1492d117aca4
Factor out ScrollbarChange handling. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/cb21a032e4ad
Don't reframe scrollable frames if we already have all needed anonymous content. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/16504649f489
Fix some pre-existing dynamic restyling issues with scrollbar-width. r=dholbert
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/31251 for changes under testing/web-platform/tests
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 95 Branch
Upstream PR merged by moz-wptsync-bot
Duplicate of this bug: 1740609
You need to log in before you can comment on or make changes to this bug.