1977511 - Consider disabling overlay scrollbars if a `-webkit-scrollbar` style rule is specified with nonzero width and height

Status: RESOLVED FIXED

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

Description

[Daniel Holbert [:dholbert]]

We've got at least two cases where it seems like a Google site is trying to provide a thickness for scrollbars, which (in Safari and Edge with overlay scrollbars enabled) also successfully makes the scrollbars be "non-overlay-style", which happens to be a usability win in these cases:
bug 1836872
bug 1973196

I know we've already investigated and opted not to implement -webkit-scrollbar in general (bug 1432935). We might want to investigate doing a limited intervention for cases like this to at least take certain bundles of styles as a signal that we should switch to traditional scrollbars, though... Since that's how they end up working in Chromium and WebKit right now at least.

As an example, here's a testcase where the upper block uses scrollbars if they're enabled, and the lower two use ::-webkit-scrollbar to toggle non-overlay scrollbars with specified thicknesses in Chromium and WebKit:
https://jsfiddle.net/dholbert/L83ejpvd/

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

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

As an example, here's a testcase where the upper block uses scrollbars if they're enabled, and the lower two use ::-webkit-scrollbar to toggle non-overlay scrollbars with specified thicknesses in Chromium and WebKit:
https://jsfiddle.net/dholbert/L83ejpvd/

Here's that fiddle as a bugzilla attachment, for convenience/better-archival.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (demonstrating the brokenness from nested scroll-containers, at e.g. Gmail)

Here's a testcase to demonstrate how this affects Gmail (bug 1973196).
A) In Firefox with overlay scrollbars, you can't scroll to reach SUCCESS. The inner area's overlay scrollbar isn't shown (it's covered up by the outer area's scrollbar, so you can't horizontally-scroll the inner area to see SUCCESS at all (unless you have some mechanism for touchpad/wheel-based horizontal scrolling).
B) In Firefox with legacy scrollbars, this is fine; you can scroll; you can see the two horizontal-scrollbars stacked and everything is fine.
C) In Chromium/WebKit, this is fine & you can scroll. This is true regardless of whether they're configured to use overlay scrollbars -- they react to the -webkit-scrollbar styles here by disabling overlay scrollbars and honoring the specified scrollbar width.

In this bug, I'm arguing that we should react to the prefixed scrollbar styles just like Chromium/WebKit do, and just activate legacy scrollbars (matching our (B) rendering).

We could do this on the level of the entire document, if that's easier to do (rather than turning overlay-scrollbars on/off for certain elements). Basically: whatever styles might deactivate overlay scrollbars in Chromium/WebKit should probably do the same for us (whether or not they also add additional fanciness in other browser-engines that we can ignore).

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: screencast showing behaviors (A)/(B)/(C) on testcase 2

Here's a screencast showing the behaviors A/B/C from the previous comment.

Behavior (A) at the beginning is the problematic one -- that's where we're leaving content unreachable, basically.

Comment 4

[Alex Jakobi [:ajakobi]]

testcase 2 shows different behaviors between OSes.

On macOS (with default settings), we're always showing both toolbars with both being reachable and not placed on top of each other (on the z-axis). There seems to be no "always show scrollbars" setting available in the settings.

On Windows 11, I'm seeing the behavior as shown in the screencast in comment #3 with the setting "always show scrollbars" not available.

On Linux (Ubuntu 24.04, X11), I'm seeing the behavior as shown in the screencast in comment #3 with the setting "always show scrollbars" available.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

(In reply to Alex Jakobi [:ajakobi] from comment #4)

On macOS (with default settings), we're always showing both toolbars with both being reachable and not placed on top of each other (on the z-axis). There seems to be no "always show scrollbars" setting available in the settings.

On macOS this setting is in the OS settings: https://www.macrumors.com/how-to/make-scroll-bars-always-visible/

On Windows 11, I'm seeing the behavior as shown in the screencast in comment #3 with the setting "always show scrollbars" not available.

Same on Windows (https://www.windowscentral.com/how-always-show-scrollbars-windows-11)

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1977511 - Add reftests for ::-webkit-scrollbar. r?#layout-reviewers

There are 6 test cases;

1. webkit-scrollbar.html has
   ::-webkit-scrollbar { width: 8px; height: 8px; }
2. webkit-scrollbar-zero-width.html has
   ::-webkit-scrollbar { width: 0px; }
3. webkit-scrollbar-no-width-height.html has
   ::-webkit-scrollbar { background-color: #aaa; }
4. disable-horizontal-scrollbar-dynamically.html has no
   ::webkit-scrollbar styles, checks non-overlay scrollbars
   properly disappear once after the contents in the scroll container
   does no longer overflow
5. webkit-scrollbar-focus.html has
   :focus::-webkit-scrollbar { width: 8px; height: 8px; }
6. webkit-scrollbar-display-none.html has
   ::webkit-scrollbar { width: 8px; display: none; }

As of now 6) doesn't work as expected.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: WIP: Bug 1977511 - Disable overlay scrollbars if there's any non-zero width and height -webkit-scrollbar.

This change does;

1. not make the current <scrollbar> element be a ::-webkit-scrollbar
   pseudo element
2. rather, while traversing <scrollbar> element for its styling,
   additionally collect ::-webkit-scrollbar rules and match them
   as if the <scrollbar> is a ::-webkit-scrollbar pseudo
3. then if we found width > 0 or height > 0 styles on any
   non-root <scrollbar> element, disable overlay scrollbar globally per
   document, and kicks a media feature change evaluation

There are some restrictions;

0. The machinery for property restrictions doesn't work for
   ::-webkit-scrollbar since it's not implemented as a genuine psuedo
   element
1. ::-webkit-scrollbar is not applicable to the root scroll container
   I haven't figured out how to obtain the originating element, i.e. the
   <html> or <body>, while traversing the root <scrollbar>
2. Disabling overlay scrollbars does just work on the entire document,
   it doesn't disable overlay on an individual scrollbar. We may be able
   to do it by adding inline styles such as opacity: 1 ! important to
   each <scrollbar> to negate overlay styles defined in a UA style sheet
   [1] with -moz-overlay-scrollbar media query.

[1] https://searchfox.org/firefox-main/rev/58f365ba0eb5761a182f1925e4654cc75212b8ac/layout/style/res/scrollbars.css#140-149

Comment 8

[Hiroyuki Ikezoe (:hiro)]

I think I am being stuck at finding ways to apply property restrictions to ::-webkit-scrollbar styles.

The current approach (written in D280698) is to not make <scrollbar> be a ::-webkit-scrollbar, it just additionally collects ::-webkit-scrollbar styles while traversing <scrollbar>. The current D280698 works in most cases, but if there's any CSS property other than width and height for ::-webkit-scrollbar, the property is also applicable to <scrollbar>, thus it will break <scrollbar>, one observable result I have seen is an assertion for cached scrollbar styles hits.

Emilio, do you have opinions on this issue? I'd personally like the original approach that is to make <scrollbar> be a ::-webkit-scrollbar pseudo element since the machinery for property restrictions works as-it-is. And the current approach also has a couple of hacks in selector matching.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Yeah, adding a SetPseudoElement(webkitScrollbar) to the scrollbar node seems more straight-forward, but that said, that's gonna break the scrollbar caching as well. also, we should disable overlay scrollbars only on that particular scroll container.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1977511 - Use ScrollContainerFrame::UseOverlayScrollbars in ResizeObserver and ScrollbarAreaToExcludeFromCompositionBoundsFor. r?#layout-reviewers

Most of the remaining call sites of nsPresContext::UseOverlayScrollbars
for scrollbar parts' frames will be replaced by a new function in the
next change.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1977511 - Introduce nsLayoutUtils::UseOverlayScrollbars and use it in proper places instead of nsPresContext::UseOverlayScrollbars. r?#layout-reviewers

There remain some direct call sites of
nsPresContext::UseOverlayScrollbars.

0. two call sites in nsCSSFrameConstructor::GetAnonymousContent [0]
   These call sites for used for scrollbar style caching stuff,
   thoguh I am not 100% sure whether these two call sites needs to be
   replaced, in the next patch only width/height are applicable to
   ::-webkit-scrollbar pseudo elements and those properties are flagged
   as has_effect_on_gecko_scrollbars=false, so it's okay-ish?
   anyway the first one can not be replaced since it gets called before
   styling the <scrollbar> itself

1. in Element::GetClientAreaRect [1]
   it's for an optimization to avoid layout flush on the root scroll
   container, we will not apply ::-webkit-scrollbar to the root one, so it
   doesn't matter

2. in Gecko_GetScrollbarInlineSize [2]
   it's used only for internal CSS env scrollbar-inline-size, so it
   doesn't matter unless we use ::-webkit-scrollbar internally

3. in nsTreeBodyFrame::Init [3]
   it's XUL we won't use ::-webkit-scrollbar in chrome contents

4. in Gecko_MediaFeatures_UseOverlayScrollbars [4]
   it's used for -moz-overlay-scrollbars media query, we leave the
   styles in the media query, to disable overlay we will set active to
   the scrollbar element.

[0] https://searchfox.org/firefox-main/rev/33fd6bd39c625067a29f153adce6a4646e45750f/layout/base/nsCSSFrameConstructor.cpp#3964,4035
[1] https://searchfox.org/firefox-main/rev/e480af4adea0d838f97b105d7efdd32fa18b73c1/dom/base/Element.cpp#1057
[2] https://searchfox.org/firefox-main/rev/e480af4adea0d838f97b105d7efdd32fa18b73c1/layout/style/GeckoBindings.cpp#317
[3] https://searchfox.org/firefox-main/rev/e480af4adea0d838f97b105d7efdd32fa18b73c1/layout/xul/tree/nsTreeBodyFrame.cpp#293
[4] https://searchfox.org/firefox-main/rev/e480af4adea0d838f97b105d7efdd32fa18b73c1/layout/style/nsMediaFeatures.cpp#112

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1977511 - Resolve ::-webkit-scrollbar pseudo element styles soon after the scroll container style was set. r?#layout-reviewers

While traversing ::-webkit-scrollbar pseudo element for its styling,
additionally collect rules for <scrollbar> defined in UA style sheet
and match them.

There are two caveats;

1. we tell a lie in @supports selector as if we don't handle
   ::-webkit-scrollbar at all
2. only width and height properties are applicable to ::webkit-scrollbar
   pseudo elements, but these values are never used for <scrollbar>
   elements becasue of the nature of our scrollbar implementation, i.e.
   it rather respects scrollbar-width.

There's a restriction;

1. ::-webkit-scrollbar is not applicable to the root scroll container
   I haven't figured out how to obtain the originating element, i.e. the
   <html> or <body>, while traversing the root <scrollbar>

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1977511 - Disable overlay scrollbars if there's any non-zero width and height defined in ::-webkit-scrollbar. r?#layout-reviewers

To disable scrollbars on a given scrollbar we set active attribute to
the vertical and horizontal scrollbar elements as if the user is
hovering over the scrollbars so that we don't specifically need to
cancel out styles in @media (-moz-overlay-scrollbars) query in
scrollbars.css even though the media query is document wide.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Thank you, Emilio. I've somehow managed to make it happen. I am still not confident scrollbar style caching, I've written what I am thinks is in the commit message of D281571. I hope it does make sense to you!

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Hmm, looking a bit closer at this and all the special cases that were needed, I wonder if it'd be easier to just add a RefPtr<ComputedStyle> ScrollContainerFrame::mWebKitScrollbarStyle; and update it in DidSetComputedStyle, via ProbePseudoElementStyle... Seems that'd work and wouldn't be a penalty for pages that don't use ::-webkit-scrollbar (we don't selector-match / compute a style at all in ProbePseudoElementStyle...)

It might be faster than breaking the scrollbar style caching perhaps? Hiro, that seems like a maybe easier approach? I think we didn't consider this approach when discussing it (only either "adding more NAC" or "turning scrollbar element into ::-webkit-scrollbar").

Assuming it'd be fast enough to probe on a page with no ::-webkit-scrollbar rules, and that we only probe one style (::-webkit-scrollbar really has some extra pseudo-classes too like ::-webkit-scrollbar:horizontal etc, not sure if they work now), it might be faster and less invasive.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Thank you, Emilio! With the new approach the code change became much simpler!

Comment 17

[Hiroyuki Ikezoe (:hiro)]

With supporting ::-webkit-scrollbar in @supports some wpts fail (here is a try run)

For example, scrollbar-width-010.html is one of the failure tests.

From the test title;

CSS Scrollbars: scrollbar-width auto on the root defers to ::-webkit-scrollbar on the root

From the relevant spec;

3.1. Interaction with non-standard features
On any element or pseudo-element where the computed value of scrollbar-width is anything other than the initial auto value, user agents must ignore any alternative non-standard means for authors to influence the rendering of scrollbars, such as the ::webkit-scrollbar family of pseudo-elements.

So I believe the test is invalid, the spec doesn't mention that ::-webkit-scrollbar needs to be respected in the case where scrollbar-width is auto.

Daniel, what do you think? What's a reasonable step to progress this bug? e.g., marking these wpts as FAIL in our tree and opening a PR to remove them from web-platform-tests?

Or, if we do a lie for @supports, these tests pass. Emilo, what do you think?

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #17)

For example, scrollbar-width-010.html is one of the failure tests.

[ Explaining the test failure, largely for the benefit of future-me; I expect you already know the following: ]
For this specific test, and I imagine many of the others that you referenced -- the test is turning off scrollbars with a ::-webkit-scrollbar { display: none } rule, and it checks that they were in fact turned off (by comparing the root element's width against the viewport width). That should trivially fail in Firefox because we'll ignore the ::-webkit-scrollbar style, but the reason we currently pass the test is that it has this fallback style for browsers-that-lack-support-for-::-webkit-scrollbar which independently turns off the scrollbars:

@supports not selector(::-webkit-scrollbar) {
  :root {
    overflow: hidden;
  }

This bug's current patch makes us skip that @supports clause (because we now claim to support ::-webkit-scrollbar), so we end up at the mercy of the ::-webkit-scrollbar{ display:none } rule to do the scrollbar-hiding; and IIUC the current patch doesn't actually implement any such display:none scrollbar-hiding behavior (right?), which is why we fail.

From the relevant spec;
[...]
So I believe the test is invalid, the spec doesn't mention that ::-webkit-scrollbar needs to be respected in the case where scrollbar-width is auto.

I think the test's expectations are fine, though probably the test should be renamed to have .tentative since it's testing something nonstandard -- it's expecting that ::-webkit-scrollbar DOES have a particular effect; and the relevant standards only make declarations about situations when ::-webkit-scrollbar should NOT have an effect. (i.e. the spec for the modern feature just says that the modern feature "wins" if the page specifies both, but it's not trying to define how ::-webkit-scrollbar behaves on its own)

So. Having said all that - I'm not super worried about the WPT failure in-and-of-itself (I don't think it's part of an interop score or anything, and I'm not entirely sure whether it's reflective of real-world content) -- but for the purposes of this bug here, I think this WPT failure raises two questions:

(A) As part of this work, maybe we should treat ::-webkit-scrollbar { display: none } as effectively the same as scrollbar-width: none? (when scrollbar-width is unspecified) I assume that's essentially what Chrome and Safari are doing, and that's what this one test at least is expecting. And I believe we've seen that styling in real-world sites before, so it'd probably be nice to support that.

(B) Should we worry about sites specifying scrollbar-styles using @supports conditions for ::-webkit-scrollbar styles, with graceful fallback that we're currently benefiting from, where we'll start losing out if we start pretending to support ::-webkit-scrollbar?

My thoughts on these questions:

• I tend to think we should do (A), though it could easily happen as a separate/followup work item. We just might want to consider it as something that blocks letting layout.css.fake-webkit-scrollbar.enabled ride the trains.
• I'm not super worried about (B); I suspect the types of sites that bother to use @supports conditions to check for ::-webkit-scrollbar support will also be the types of sites that would default to using the standard properties. (which means partially-implemented ::-webkit-scrollbar would still be fine). And in many cases maybe it won't end up mattering too much.

Comment 19

[Daniel Holbert [:dholbert]]

(Also: before landing, it might be worth sending an intent-to-prototype email about this feature, since it'll be a new web-exposed API surface, even if really we're just having a minimal reaction to a legacy API -- e.g. disabling overlay scrollbars as proposed in this bug, and possibly turning off scrollbars if you agree that that's worth pursuing per (A), and responding to @supports since we do now parse the syntax even if we don't have full support for most aspects of the legacy feature.)

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Clearing NI to Emilio. I've started working on the (A) plan. Thank you Daniel!

Comment 21

[Daniel Holbert [:dholbert]]

Thanks! I also found some confirmation that ::-webkit-scrollbar { display: none } is indeed a useful thing to support -- just a few weeks ago, we ran into bug 2016775 where Slack used that exact formulation to hide the "real" scrollbars (so that Slack can draw its own synthetic scrollbar). And because we don't support that syntax, Firefox showed two superimposed scrollbars: the real one and slack's synthetic one.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1977511 - Handle `::-webkit-scrollbar { display:none }` as if it's `scrollbar-width: none`. r?#layout-reviewers

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/19303dead119
https://hg.mozilla.org/integration/autoland/rev/2ead425234a5
Use ScrollContainerFrame::UseOverlayScrollbars in ResizeObserver and ScrollbarAreaToExcludeFromCompositionBoundsFor. r=layout-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/0694ea2bf875
https://hg.mozilla.org/integration/autoland/rev/0c2657e75214
Introduce nsLayoutUtils::UseOverlayScrollbars and use it in proper places instead of nsPresContext::UseOverlayScrollbars. r=layout-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/b8f0d32cfb32
https://hg.mozilla.org/integration/autoland/rev/542767479d68
Resolve ::-webkit-scrollbar pseudo element styles soon after the scroll container style was set. r=layout-reviewers,devtools-reviewers,firefox-style-system-reviewers,nchevobbe,emilio
https://github.com/mozilla-firefox/firefox/commit/904febaca78e
https://hg.mozilla.org/integration/autoland/rev/535df2422590
Disable overlay scrollbars if there's any non-zero width and height defined in ::-webkit-scrollbar. r=layout-reviewers,firefox-style-system-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/1ef6a456dc89
https://hg.mozilla.org/integration/autoland/rev/365ed3302c3c
Add reftests for ::-webkit-scrollbar. r=layout-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/e7e66a8471ec
https://hg.mozilla.org/integration/autoland/rev/3981bcd36b50
Handle `::-webkit-scrollbar { display:none }` as if it's `scrollbar-width: none`. r=layout-reviewers,firefox-style-system-reviewers,emilio

Comment 24

[amarc]

https://hg.mozilla.org/mozilla-central/rev/2ead425234a5
https://hg.mozilla.org/mozilla-central/rev/0c2657e75214
https://hg.mozilla.org/mozilla-central/rev/542767479d68
https://hg.mozilla.org/mozilla-central/rev/535df2422590
https://hg.mozilla.org/mozilla-central/rev/365ed3302c3c
https://hg.mozilla.org/mozilla-central/rev/3981bcd36b50

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a6cb0e6249aa
https://hg.mozilla.org/mozilla-central/rev/305f77cef750
Revert "Bug 1977511 - Handle `::-webkit-scrollbar { display:none }` as if it's `scrollbar-width: none`. r=layout-reviewers,firefox-style-system-reviewers,emilio" for causing a top crash in bug 2026487.

Comment 26

[Atila Butkovits]

Backed out for causing a top crash in bug 2026487.

Backout link: https://hg.mozilla.org/mozilla-central/rev/305f77cef750

Comment 27

[Emilio Cobos Álvarez [:emilio]]

FWIW I think it's probably wise to enable on nightly for a while before letting it ride the trains since it's a significant change and also a partial implementation of ::-webkit-scrollbar. So I'm not super confident we won't find other issues (authors relying on the width being exactly the one they set, etc).

Comment 28

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #27)

FWIW I think it's probably wise to enable on nightly for a while before letting it ride the trains since it's a significant change and also a partial implementation of ::-webkit-scrollbar. So I'm not super confident we won't find other issues (authors relying on the width being exactly the one they set, etc).

Yeah, that's a good idea. I changed the pref nightly only in D281572.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/5baba0ed09ed
https://hg.mozilla.org/integration/autoland/rev/5047deb05c0e
Use ScrollContainerFrame::UseOverlayScrollbars in ResizeObserver and ScrollbarAreaToExcludeFromCompositionBoundsFor. r=layout-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/8856d354717a
https://hg.mozilla.org/integration/autoland/rev/1b1ab4a53441
Introduce nsLayoutUtils::UseOverlayScrollbars and use it in proper places instead of nsPresContext::UseOverlayScrollbars. r=layout-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/8164020c7683
https://hg.mozilla.org/integration/autoland/rev/694f465a3e92
Resolve ::-webkit-scrollbar pseudo element styles soon after the scroll container style was set. r=layout-reviewers,devtools-reviewers,firefox-style-system-reviewers,nchevobbe,emilio
https://github.com/mozilla-firefox/firefox/commit/926eea5a10ff
https://hg.mozilla.org/integration/autoland/rev/4fd7972d5c9f
Disable overlay scrollbars if there's any non-zero width and height defined in ::-webkit-scrollbar. r=layout-reviewers,firefox-style-system-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/1b6d4641c7d4
https://hg.mozilla.org/integration/autoland/rev/cb5330671cff
Add reftests for ::-webkit-scrollbar. r=layout-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/fe392b1cd5ff
https://hg.mozilla.org/integration/autoland/rev/2f9a308efc82
Handle `::-webkit-scrollbar { display:none }` as if it's `scrollbar-width: none`. r=layout-reviewers,firefox-style-system-reviewers,emilio

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a2d880fc9bb4
https://hg.mozilla.org/integration/autoland/rev/e85303a757eb
Revert "Bug 1977511, Bug 2026430 - Guard mScrollbarActivity access with WeakFrame. r=emilio,layout-reviewers"

Comment 31

[Cosmin Sabou [:CosminS]]

Backed out for causing build bustages on ScrollContainerFrame

Push with failures

Failure log

Backout link

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Geez.

ERROR - /builds/worker/checkouts/gecko/layout/generic/ScrollContainerFrame.cpp:5900:7: error: variable of type '(lambda at /builds/worker/checkouts/gecko/layout/generic/ScrollContainerFrame.cpp:5900:7)' only valid on the heap

What I originally wrote was violating it. Actually I didn't notice that WeakFrame is only used in heap. But with;

    nsContentUtils::AddScriptRunner(NS_NewRunnableFunction(
        "ScrollContainerFrame::DisableOverlayScrollbars",
        [weakFrame = WeakFrame{this}] {

It's not allocated in heap along with the runnable instance? It's just the copied over one? WeakFrame{this} is still allocated on the stack of the call site?

Comment 33

[Hiroyuki Ikezoe (:hiro)]

I've updated https://phabricator.services.mozilla.com/D289998, with wrapping the WeakFrame with unique_ptr and pushed a try run to see whether the compiler complains it or not.

Comment 34

[Hiroyuki Ikezoe (:hiro)]

Geez, the try push didn't happen. Lando doesn't allows us pushing changes including fixes for security bugs. It does make sense in normal cases, but in this specific case it doesn't make any sense since the security bug (bug 2026430) in question was caused by the patches here in this bug.

Comment 35

[Hiroyuki Ikezoe (:hiro)]

With ac_add_options --enable-clang-plugin option (RyanVM told me it on Matrix) I do see the compile error locally and with the revised change (with unique_ptr wrap), I don't see the error. I will go with it.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/4a9694dee4e6
https://hg.mozilla.org/integration/autoland/rev/1085ddb446f8
Use ScrollContainerFrame::UseOverlayScrollbars in ResizeObserver and ScrollbarAreaToExcludeFromCompositionBoundsFor. r=layout-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/9b778db5249c
https://hg.mozilla.org/integration/autoland/rev/58196de66b55
Introduce nsLayoutUtils::UseOverlayScrollbars and use it in proper places instead of nsPresContext::UseOverlayScrollbars. r=layout-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/6dcee483eb76
https://hg.mozilla.org/integration/autoland/rev/9defa83e4762
Resolve ::-webkit-scrollbar pseudo element styles soon after the scroll container style was set. r=layout-reviewers,devtools-reviewers,firefox-style-system-reviewers,nchevobbe,emilio
https://github.com/mozilla-firefox/firefox/commit/12febf80728d
https://hg.mozilla.org/integration/autoland/rev/d0f87d0f2b6a
Disable overlay scrollbars if there's any non-zero width and height defined in ::-webkit-scrollbar. r=layout-reviewers,firefox-style-system-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/0ab2ebb66be3
https://hg.mozilla.org/integration/autoland/rev/e40a4b5768d0
Add reftests for ::-webkit-scrollbar. r=layout-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/f65f37da8972
https://hg.mozilla.org/integration/autoland/rev/decda03a4596
Handle `::-webkit-scrollbar { display:none }` as if it's `scrollbar-width: none`. r=layout-reviewers,firefox-style-system-reviewers,emilio

Comment 37

[rperta]

https://hg.mozilla.org/mozilla-central/rev/1085ddb446f8
https://hg.mozilla.org/mozilla-central/rev/58196de66b55
https://hg.mozilla.org/mozilla-central/rev/9defa83e4762
https://hg.mozilla.org/mozilla-central/rev/d0f87d0f2b6a
https://hg.mozilla.org/mozilla-central/rev/e40a4b5768d0
https://hg.mozilla.org/mozilla-central/rev/decda03a4596