1921257 - Content area box-shadow and outline cut off when sidebar is set to positionend in revamp.

Status: REOPENED

(Firefox :: Sidebar, defect, P1)

Description

[Nikki Sharpley (:nikkis) (she/her)]

Attachment: Screenshot 2024-09-26 at 10.23.56 AM.png

Regressed by:

Bug 1916004 - Use a box shadow for chrome/content separator. r=dao,desktop-theme-reviewers,tabbrowser-reviewers

This makes it work in fullscreen and regardless of whether the sidebar
(or the toolbox for that matter) is shown.

Differential Revision: https://phabricator.services.mozilla.com/D220770

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1916004

:emilio, since you are the author of the regressor, bug 1916004, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1921257 - Make painting order of sidebar items consistent regardless of DOM order. r=#sidebar-reviewers

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e488803f8f2d
Make painting order of sidebar items consistent regardless of DOM order. r=sidebar-reviewers,desktop-theme-reviewers,sfoster

Comment 4

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/e488803f8f2d

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Sheriffs: can we back this out from beta for causing bug 1921811 on macOS? Can stay in nightly, this needs more sorting out there.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(See comment above)

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/3a12df568bdf7ca1918ebc0a9d22b61eb12a6a4a

So is this a "wontfix" for beta?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Probably... Need to check with the sidebar folks.

Comment 9

[Sarah Clements [:sclements]]

If there's time for a low risk fix before the last early beta, then let's do it, otherwise its probably fine to "won't fix" for 132.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

I can't think of an easy fix that doesn't regress bug 1916004 / bug 1915525 and other dupes.

Comment 11

[Sarah Clements [:sclements]]

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

I can't think of an easy fix that doesn't regress bug 1916004 / bug 1915525 and other dupes.

Yeah, having looked at those I agree and this bug is a minor styling issue. Thanks for looking into all of that quickly.

Comment 12

[Serban Stanca [:SerbanS]]

Backed out as requested for causing cosmetic issue that still needs more work.

• Backout link

Comment 13

[Sam Foster [:sfoster] (he/him)]

I think we still want to resolve this in nightly, right? Marking as P1.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Yes, so... This is unfortunately all a bit tricky, and depends on what we want to do with, at least, bug 1921811 and bug 1921819.

This is my understanding of the situation:

• In order for the shadow and separator to draw, we want to always draw the content area (#tabbrowser-tabbox) on top, since it's what provides the outline and shadow.
• That breaks the fullscreen animation (which is already kinda broken, but not so much, see bug 1921819).
• More importantly, that breaks the toolbox if it becomes its own stacking context, because now the urlbar doesn't pop out.

The way I would personally go about fixing it would be:

• Make the urlbar a popover (bug 1921811 has a WIP that seems to work locally).
• Make the macOS animation not slide down (bug 1921819 has some thoughts).
• Only then, fix the paint order to be: content-area, on top of sidebar splitters, on top of sidebars, on top of toolbox.

I'm happy to assist, but I honestly don't have the cycles to do all that right now. Sam, does my understanding above make sense to you?

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Once all that is done, then the fix for this bug is just my patch above, but with --browser-area-z-index-toolbox: 0, and --browser-area-z-index-sidebar-splitter and tabbox swapped.

Hopefully painting always the content area on top doesn't cause any regression like bug 1921997 (but hard to say).

Comment 16

[Sam Foster [:sfoster] (he/him)]

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

I'm happy to assist, but I honestly don't have the cycles to do all that right now. Sam, does my understanding above make sense to you?

Make the urlbar a popover (bug 1921811 has a WIP that seems to work locally).

Yes, that all makes sense. But I really want Dao's thoughts on the urlbar thing. That was quite recently overhauled, and I think performance was a major constraint. We'd also want to make sure that doesnt regress anything a11y-wise.

Comment 17

[Dão Gottwald [:dao]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #16)

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

I'm happy to assist, but I honestly don't have the cycles to do all that right now. Sam, does my understanding above make sense to you?

Make the urlbar a popover (bug 1921811 has a WIP that seems to work locally).

Yes, that all makes sense. But I really want Dao's thoughts on the urlbar thing. That was quite recently overhauled, and I think performance was a major constraint. We'd also want to make sure that doesnt regress anything a11y-wise.

popover didn't exist at the time this was overhauled. Seems like it should do the trick; I don't foresee performance or a11y problems.

Comment 18

[Alex Finder]

(In reply to Serban Stanca [:SerbanS] from comment #12)

Backed out as requested for causing cosmetic issue that still needs more work.

• Backout link

Perfherder has detected a talos performance change from push 92d79fb7e800f1a7f8b814d09746c8e33fa9af8d.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
8%	tart	windows11-64-shippable-qr	e10s fission stylo webrender	1.96 -> 1.80

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 2321

For more information on performance sheriffing please see our FAQ.

Comment 19

[Alex Finder]

(In reply to Serban Stanca [:SerbanS] from comment #12)

Backed out as requested for causing cosmetic issue that still needs more work.

• Backout link

Perfherder has detected a browsertime performance change from push 92d79fb7e800f1a7f8b814d09746c8e33fa9af8d.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
7%	yahoo-mail loadtime	windows11-64-shippable-qr	bytecode-cached cold fission webrender	447.63 -> 416.88
4%	pinterest PerceptualSpeedIndex	windows11-64-shippable-qr	fission warm webrender	736.75 -> 704.41
4%	pinterest loadtime	windows11-64-shippable-qr	fission warm webrender	626.19 -> 602.93
4%	google-mail FirstVisualChange	windows11-64-shippable-qr	fission warm webrender	566.92 -> 546.36	Before/After
4%	google-mail ContentfulSpeedIndex	windows11-64-shippable-qr	fission warm webrender	567.93 -> 547.57	Before/After
...	...	...	...	...	...
2%	nytimes LastVisualChange	windows11-64-shippable-qr	fission warm webrender	802.83 -> 786.37

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 2373

For more information on performance sheriffing please see our FAQ.