Closed Bug 1441102 Opened 2 years ago Closed 2 years ago

Replace --titlebar-text-color with a class

Categories

(Firefox :: Theme, enhancement, P3)

59 Branch
Unspecified
Windows
enhancement

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: xidorn, Assigned: dao)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

I looked at the profile and noticed that there is a big restyle in the later part of tpaint. I investigate a bit and it turns out to be roughly a full document restyle because of a change to custom property --titlebar-text-color in rule containing :-moz-window-inactive.

If I commenting out setting of --titlebar-text-color in all those rules, the big restyle disappears.

(This actually makes me wonder, how we have performance gain at all from bug 1428978 on windows when we still have such custom property...)

From what I can see, it is referenced in .titlebar-button, #toolbar-menubar, #TabsToolbar, so it is a bit annoying to move it.

Probably we can add a new class, e.g. titlebar-color, and apply it to #titlebar-buttonbox, #toolbar-menubar, #TabsToolbar, so that we have a nicer selector for them.
Gijs, what do you think about this?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #0)
> Probably we can add a new class, e.g. titlebar-color, and apply it to
> #titlebar-buttonbox, #toolbar-menubar, #TabsToolbar, so that we have a nicer
> selector for them.

Why would this avoid the full-document restyle?

I mean, to me using a class seems fine, but the selectors look like they rely on the root [inFullScreen] attribute so it would end up using descendant selectors still, which is a bit unfortunate. Is there a better solution?

I suspect :dao may be a good person to check with as well...
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(xidorn+moz)
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #0)
> I looked at the profile and noticed that there is a big restyle in the later
> part of tpaint. I investigate a bit and it turns out to be roughly a full
> document restyle because of a change to custom property
> --titlebar-text-color in rule containing :-moz-window-inactive.

Is there anything we can do to avoid first styling the window as inactive?
(In reply to :Gijs from comment #2)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #0)
> > Probably we can add a new class, e.g. titlebar-color, and apply it to
> > #titlebar-buttonbox, #toolbar-menubar, #TabsToolbar, so that we have a nicer
> > selector for them.
> 
> Why would this avoid the full-document restyle?
> 
> I mean, to me using a class seems fine, but the selectors look like they
> rely on the root [inFullScreen] attribute so it would end up using
> descendant selectors still, which is a bit unfortunate. Is there a better
> solution?

It doesn't matter what selector is used here. Restyling an element means we recompute the property value for the element, which is expensive.

Selectors only affect how we choose what element we need to restyle (the mechanism we choose them for restyle is called invalidation). In this case, we need to traverse the whole document for invalidation anyway. And actually descendant combinator is probably the easier/cheapest one to handle in invalidation.

(In reply to Florian Quèze [:florian] from comment #3)
> Is there anything we can do to avoid first styling the window as inactive?

It's not about styling the newly-opened window. It's the opener window gets deactivated in this case.

(That said, this is probably pretty specific to tpaint, since ts_paint may not have window to deactivate? Maybe sessionrestore manywindow may has some. And specific to Windows, since other platforms don't seem to have this kind of stuff.)
Flags: needinfo?(xidorn+moz)
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4)

> (In reply to Florian Quèze [:florian] from comment #3)
> > Is there anything we can do to avoid first styling the window as inactive?
> 
> It's not about styling the newly-opened window. It's the opener window gets
> deactivated in this case.

Ah ok. Indeed when opening new windows I see in profiles that we spend time deactivating the previous window before we paint the new one.

> (That said, this is probably pretty specific to tpaint, since ts_paint may
> not have window to deactivate? Maybe sessionrestore manywindow may has some.
> And specific to Windows, since other platforms don't seem to have this kind
> of stuff.)

I'm pretty sure that for a couple of our supported OSes (OS X and Win7 if I remember correctly), we paint the new windows inactive before activating them.
(In reply to Florian Quèze [:florian] from comment #5)
> I'm pretty sure that for a couple of our supported OSes (OS X and Win7 if I
> remember correctly), we paint the new windows inactive before activating
> them.

From my profile on win10 in bug 1420423 comment 76, it seems we do style the window inactive, but we don't paint it before. Activating doesn't trigger a significant restyle in that case, although I'm not sure why given existing of this custom property...
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #6)
> (In reply to Florian Quèze [:florian] from comment #5)
> > I'm pretty sure that for a couple of our supported OSes (OS X and Win7 if I
> > remember correctly), we paint the new windows inactive before activating
> > them.
> 
> From my profile on win10 in bug 1420423 comment 76, it seems we do style the
> window inactive, but we don't paint it before.

Whether we actually paint the window as inactive before re-styling it as active may be somehow timing dependent.
Priority: -- → P3
(In reply to Florian Quèze [:florian] from comment #5)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4)
> 
> > (In reply to Florian Quèze [:florian] from comment #3)
> > > Is there anything we can do to avoid first styling the window as inactive?
> > 
> > It's not about styling the newly-opened window. It's the opener window gets
> > deactivated in this case.
> 
> Ah ok. Indeed when opening new windows I see in profiles that we spend time
> deactivating the previous window before we paint the new one.
> 
> > (That said, this is probably pretty specific to tpaint, since ts_paint may
> > not have window to deactivate? Maybe sessionrestore manywindow may has some.
> > And specific to Windows, since other platforms don't seem to have this kind
> > of stuff.)
> 
> I'm pretty sure that for a couple of our supported OSes (OS X and Win7 if I
> remember correctly), we paint the new windows inactive before activating
> them.

The titlebar-text-color value only exists (and therefore only changes) on Windows, which is why this is only an issue on Windows.
So this patch makes the restyle under deactivate reduced from ~7ms down to ~3ms in my local test. Not that lot but still something I guess.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #10)
> So this patch makes the restyle under deactivate reduced from ~7ms down to
> ~3ms in my local test. Not that lot but still something I guess.

Not under deactivate, but that triggered by deactivating.
Try pushes show this change improves a11yr on win32 pgo by quite a bit, although I don't really think that's real...

Anyway, it shrinks the restyle shows up in the tpaint path, although it's not a lot, it's not nothing either. Since it's a simple patch, I guess it's worth landing.
Comment on attachment 8956303 [details]
Bug 1441102 - Move --titlebar-text-color from :root to elements closer to its references.

https://reviewboard.mozilla.org/r/225178/#review231206

::: browser/themes/windows/browser-aero.css:44
(Diff revision 1)
>  
>          @media (-moz-windows-accent-color-in-titlebar: 0) {
>            :root[tabsintitlebar]:not(:-moz-lwtheme) {
>              background-color: hsl(235,33%,19%);
> +          }
> +          :root[tabsintitlebar]:not(:-moz-lwtheme) .titlebar-color {

nit: :root[tabsintitlebar] .titlebar-color:not(:-moz-lwtheme) {

::: browser/themes/windows/browser-aero.css:45
(Diff revision 1)
>          @media (-moz-windows-accent-color-in-titlebar: 0) {
>            :root[tabsintitlebar]:not(:-moz-lwtheme) {
>              background-color: hsl(235,33%,19%);
> +          }
> +          :root[tabsintitlebar]:not(:-moz-lwtheme) .titlebar-color {
>              --titlebar-text-color: hsl(240,9%,98%);

If we add this class, we should just get rid of the variable and set the color directly in these rules.
Attachment #8956303 - Flags: review-
Comment on attachment 8956303 [details]
Bug 1441102 - Move --titlebar-text-color from :root to elements closer to its references.

https://reviewboard.mozilla.org/r/225178/#review231238

Dão is a better reviewer for this css, and I see he already had a look at the patch, so please request review from him on the next version, thanks!
Attachment #8956303 - Flags: review?(florian)
Picking this up as discussed with Xidorn.
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Summary: Consider moving --titlebar-text-color from :root into somewhere else → Replace --titlebar-text-color with a class
Attachment #8956303 - Attachment is obsolete: true
Comment on attachment 8956841 [details]
Bug 1441102 - Replace --titlebar-text-color with a class.

https://reviewboard.mozilla.org/r/225796/#review232378

Looks good to me (although I'm slightly out of my reviewing comfort zone here).
Attachment #8956841 - Flags: review?(florian) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/275400cc371d
Replace --titlebar-text-color with a class. r=florian
https://hg.mozilla.org/mozilla-central/rev/275400cc371d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1468077
You need to log in before you can comment on or make changes to this bug.