Closed Bug 1812868 Opened 1 year ago Closed 1 year ago

Expose scrollbar-inline-size as a CSS variable to chrome code

Categories

(Thunderbird :: Folder and Message Lists, enhancement)

Thunderbird 111
enhancement

Tracking

(thunderbird111 affected, thunderbird112 affected)

RESOLVED FIXED
112 Branch
Tracking Status
thunderbird111 --- affected
thunderbird112 --- affected

People

(Reporter: aleca, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Supernova3p])

Attachments

(1 file)

Since the entire tree list container is scrollable and the table header is in a sticky position, the scrollbar goes on top of it.
This is a bit annoying when the OS scrollbar has a "floating" style and it's selected, which ends up covering a good part of the column picker button when selected.

We should add a padding at the end of the column picker button based on an environmental variable detecting the width of the expanded OS scrollbar.
E.g.: padding-inline-end: env(scrollbar-inline-size);

Gentle ping to Emilio to see if it's possible to get something like that.

Flags: needinfo?(emilio)
Depends on: 1813046

For that we need to:

  • Make GetDPIRatioForScrollbarPart thread-safe: This was using the
    widget for bug 1727289, but just looking at the print preview scale
    is enough to fix that.

  • Make nsPresContext::UseOverlayScrollbars() thread-safe: We store the
    RDM pane stuff in the pres context.

The rest is pretty straight-forward.

TODO: Make the test actually work.

Assignee: nobody → emilio
Attachment #9314621 - Attachment description: WIP: Bug 1812868 - Expose scrollbar-inline-size as a CSS variable. → Bug 1812868 - Expose scrollbar-inline-size as a CSS variable. r=#style,#layout,spohl,mstange
Status: NEW → ASSIGNED
Flags: needinfo?(emilio)
Whiteboard: [Supernova]
Attachment #9314621 - Attachment description: Bug 1812868 - Expose scrollbar-inline-size as a CSS variable. r=#style,#layout,spohl,mstange → Bug 1812868 - Expose scrollbar-inline-size as a CSS variable to chrome code. r=#style,#layout,spohl,mstange
Attachment #9314621 - Attachment description: Bug 1812868 - Expose scrollbar-inline-size as a CSS variable to chrome code. r=#style,#layout,spohl,mstange → Bug 1812868 - Expose scrollbar-inline-size as a CSS variable. r=#style,#layout,spohl,mstange
Attachment #9314621 - Attachment description: Bug 1812868 - Expose scrollbar-inline-size as a CSS variable. r=#style,#layout,spohl,mstange → Bug 1812868 - Expose scrollbar-inline-size as a CSS variable to chrome code. r=#style,#layout,spohl,mstange
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e7d370501c50
Expose scrollbar-inline-size as a CSS variable to chrome code. r=mstange
Pushed by imoraru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/07c689de250c
fix mismatch in scrollbar size. r=emilio CLOSED TREE
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 112 Branch

Thanks Emilio for adding this feature.
If I'm reading the code correctly, scrollbar-inline-size is only exposed to chrome and not content. If that's the case, I don't think we can use it since the new about3Pane is in a browser, so it's all content.

Flags: needinfo?(emilio)
Backout by smolnar@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/d63352149381
Backed out 2 changesets for causing bug 1817539. CLOSED TREE
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 112 Branch → ---

(In reply to Alessandro Castellani [:aleca] from comment #5)

Thanks Emilio for adding this feature.
If I'm reading the code correctly, scrollbar-inline-size is only exposed to chrome and not content. If that's the case, I don't think we can use it since the new about3Pane is in a browser, so it's all content.

As long as the document uri scheme is chrome:// it should work, so I don't think that's an issue.

Attachment #9314621 - Attachment description: Bug 1812868 - Expose scrollbar-inline-size as a CSS variable to chrome code. r=#style,#layout,spohl,mstange → Bug 1812868 - Expose scrollbar-inline-size as a CSS variable to chrome code. r=mstange
Flags: needinfo?(emilio)

Tim, I don't understand how this patch can cause bug 1817539 tbh, any ideas?

I'm not touching any display-list code, nor tweaking stuff in a way that could cause more stack space to be used... The weirdest of things is that this only happens on opt builds rather than debug (or both).

No worries if you don't have any idea off-hand tho, I'll figure out. I'm splitting the patch up and trying to find the culprit, but I'm confused. Also it seems this was already on file as bug 1771983 (but only happened once, which is really weird).

Flags: needinfo?(tnikkel)

Somehow https://hg.mozilla.org/try/rev/d2a026c25ce96b79892e43cebb7a99fe5ec11732 causes this. I'm a bit baffled, but DL building does call into that quite a bit, maybe the compiler is optimizing something in a bad way?

I moved nsPresContext::Theme() out of line and that fixed it, but it does seem like that test is a bit of a time bomb...

Flags: needinfo?(tnikkel)

Given I made the effort to split the patch, I'll just land them split up, since I believe it's better.

Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/1e989416b614
Tweak RDM and theme setup. r=mstange
https://hg.mozilla.org/integration/autoland/rev/808a91c49890
Tweak scrollbar scaling set-up. r=mstange
https://hg.mozilla.org/integration/autoland/rev/0e53db091ffb
Expose chrome-only environment variables to all chrome:// documents, not just chrome-docshells. r=mstange
https://hg.mozilla.org/integration/autoland/rev/6fd4ae812ff2
Expose scrollbar-inline-size as a chrome-only environment variable. r=mstange
https://hg.mozilla.org/integration/autoland/rev/4f75949dc3d7
Add tests. r=mstange

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

Somehow https://hg.mozilla.org/try/rev/d2a026c25ce96b79892e43cebb7a99fe5ec11732 causes this. I'm a bit baffled, but DL building does call into that quite a bit, maybe the compiler is optimizing something in a bad way?

So we think this is a miscompilation that you were just unlucky to have tripped over? What followup actions should we look into?

Flags: needinfo?(emilio)

It doesn't seem a miscompilation, just that the optimizer probably got more aggressive in a way that it increased the stack frame size of BuildDisplayList just enough to trigger the stack overflow.

Flags: needinfo?(emilio)

This is a bit annoying when the OS scrollbar has a "floating" style and it's selected, which ends up covering a good part of the column picker button when selected.

This is still the case (tested on macOS 13.2). It looks like these patches added platform support for scrollbar-inline-size, but it still has to be used, right? Should this ticket be re-opened?

I also still see this when using 2023-02-28 build on mac

Flags: needinfo?(alessandro)

The work done on this bug was exclusively to introduce a scrollbar-inline-size environmental variable we can use in CSS.
I'm updating the bug title to reflect that, but the bug should also be moved into Core to be more accurate.
Creating a follow up to start using that variable.

Flags: needinfo?(alessandro)
Summary: Add scrollbar padding at the end of the column picker table header button → Expose scrollbar-inline-size as a CSS variable to chrome code
Blocks: 1820262
Blocks: 1820280

Just leaving a comment to note that bug 1824565 is about similar stackoverflow crashes on win32 that were experienced here.

Whiteboard: [Supernova] → [Supernova3p]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: