1429293 - Try rewriting all selectors using :-moz-window-inactive so that it continues working when we make it an element state of root element

Status: RESOLVED INVALID

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

I'm considering making :-moz-window-inactive an element state rather than document state. To do that, we need to rewrite existing selectors using this pseudo-class to work after that change.

It is not clear whether it would cause other problems so I'd like to do it in a separate bug so that we can experiment and land it first.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1429293 - Rewrite selectors using :-moz-window-inactive so that they continue working when it only matches root.

Review commit: https://reviewboard.mozilla.org/r/211668/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211668/

Comment 2

[:Gijs (he/him)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #0)
> I'm considering making :-moz-window-inactive an element state rather than
> document state.

What does this mean, for people like me who are not familiar with either term? :-)

This is more confusing because, if I had to make up how they worked, I would have said document state was only on the root and element state was available everywhere, but the CSS changes in the path imply the opposite...

Comment 3

[:Gijs (he/him)]

Separately, while the patch looks reasonable enough, it adds a lot of descendant selectors, which we're generally told to avoid because perf. Can you explain more about why this change is necessary?

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

(In reply to :Gijs from comment #2)
> What does this mean, for people like me who are not familiar with either
> term? :-)
> 
> This is more confusing because, if I had to make up how they worked, I would
> have said document state was only on the root and element state was
> available everywhere, but the CSS changes in the path imply the opposite...

So, a pseudo-class can be used on any element.

A pseudo-class based on document state consistently matches or doesn't match elements in a document, because it is neutral / independent to element, and depends only on the document state.

Contrarily, a pseudo-class based on element state only matches an element when the element has the state bit set, so it depends on element's state rather than the document's state.

The proposed change is to convert :-moz-window-inactive from a pseudo-class based on document state (so that it matches every element and thus you can use it anywhere in a selector) to one based on the element state of only the root element (so that it would at most only match the root element).

Is it clear enough?

(In reply to :Gijs from comment #3)
> Separately, while the patch looks reasonable enough, it adds a lot of
> descendant selectors, which we're generally told to avoid because perf. Can
> you explain more about why this change is necessary?

So as I described above, we are going to change how :-moz-window-inactive is going to work. We want to change how it works because stylo has serious performance problem with pseudo-class based on document state which blocks stylo-chrome, and optimizing that would add code complexity.

Since there is no document-state-based pseudo-class in any web standard, and it is unlikely we are going to add any new ones, it doesn't seem to be worth the complexity. Thus it seems to be easier to just rewrite the selectors so that we can make it a element-state-based pseudo-class, and reuse the existing optimization.

I'll take care of not introducing performance regression with the changes. (I have had a talos try push for this patch, which doesn't show anything outstanding, but I guess I really should remeasure with the actual change from document state to element state because that is more likely to affect performance.)

Comment 5

[:Gijs (he/him)]

Comment on attachment 8941353 [details]
Bug 1429293 - Rewrite selectors using :-moz-window-inactive so that they continue working when it only matches root.

https://reviewboard.mozilla.org/r/211668/#review217450

There's a few no-op rearrangements of selectors here that should be removed, but I'm more concerned about the changes for :not(:-moz-window-inactive) based on your description of the stylo side of the change, specifically that `:-moz-window-inactive` will only ever match the root, if it matches at all, which I understand to mean that it will never match other elements, which would mean `:not(:-moz-window-inactive)` will *always* match other elements, which would change the meaning of some of the selectors you're rewriting.

FWIW, you may be able to use mozscreenshots to doublecheck you're not causing visual changes, without manually checking every platform and window configuration etc.

Also, once this lands, this should probably be noted on the firefox-dev list so people are aware of the change and don't try to rely on the old behaviour.

::: browser/base/content/browser.css:458
(Diff revision 1)
>  
>  /* The reload-button is only disabled temporarily when it becomes visible
>     to prevent users from accidentally clicking it. We don't however need
>     to show this disabled state, as the flicker that it generates is short
>     enough to be visible but not long enough to explain anything to users. */
> -#reload-button[disabled]:not(:-moz-window-inactive) > .toolbarbutton-icon {
> +:not(:-moz-window-inactive) #reload-button[disabled] > .toolbarbutton-icon {

See my comment on the windows browser.css change for the menubar for issues with this change.

::: browser/themes/windows/browser-aero.css:32
(Diff revision 1)
>        @media (-moz-windows-default-theme) {
>          :root[sizemode=normal][tabsintitlebar] {
>            border-top: 1px solid -moz-win-accentcolor;
>          }
>  
> -        :root[sizemode=normal][tabsintitlebar]:-moz-window-inactive {
> +        :root:-moz-window-inactive[sizemode=normal][tabsintitlebar] {

This is a no-op.

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

This is a no-op so should be reverted.

::: browser/themes/windows/browser-aero.css:332
(Diff revision 1)
>  
>    #toolbar-menubar:not(:-moz-lwtheme) {
>      text-shadow: 0 0 .5em white, 0 0 .5em white, 0 1px 0 rgba(255,255,255,.4);
>    }
>  
> -  #main-menubar:not(:-moz-lwtheme):not(:-moz-window-inactive) {
> +  :not(:-moz-window-inactive) #main-menubar:not(:-moz-lwtheme) {

What is the state going to be for non-root elements, and won't the `:not(:-moz-window-inactive)` end up matching non-root elements?

Again, my understanding of the internals of CSS is rudimentary, but the goal of this rule is clearly to apply only when a window is not active. If we're changing where/how this pseudoclass matches, I would expect the entirety of the selector to match any element where the :-moz-window-inactive pseudoclass doesn't match, which will presumably just match the parent of `#main-menubar` irrespective of whether the window is active or not.

If my assumptions are correct, this change is incorrect and this needs proper fixing. The simplest solution might be to add `:root` to the start of the selector, but that will change specificity slightly, so would need testing to ensure it doesn't regress anything.

::: browser/themes/windows/browser.css:165
(Diff revision 1)
>     */
>    :root[tabsintitlebar]:not([inFullscreen]):not(:-moz-lwtheme) {
>      --titlebar-text-color: CaptionText;
>    }
>  
> -  :root[tabsintitlebar]:not([inFullscreen]):not(:-moz-lwtheme):-moz-window-inactive {
> +  :root:-moz-window-inactive[tabsintitlebar]:not([inFullscreen]):not(:-moz-lwtheme) {

This change is a no-op so should be reverted.

::: browser/themes/windows/compacttheme.css:73
(Diff revision 1)
>    /* Make the menubar text readable on aero glass (copied from browser-aero.css). */
>    #toolbar-menubar {
>      text-shadow: 0 0 .5em white, 0 0 .5em white, 0 1px 0 rgba(255,255,255,.4);
>    }
>  
> -  #main-menubar:not(:-moz-window-inactive) {
> +  :not(:-moz-window-inactive) #main-menubar {

Same issue here as with the similar windows browser.css change.

::: toolkit/themes/osx/global/button.css:23
(Diff revision 1)
>    color: -moz-mac-buttonactivetext;
>  }
>  
>  /* When the window isn't focused, the default button background isn't drawn,
>   * so don't change the text color then: */
> -button[default="true"]:not([disabled="true"]):not(:-moz-window-inactive) {
> +:not(:-moz-window-inactive) button[default="true"]:not([disabled="true"]) {

Same negation issue here.

::: toolkit/themes/osx/global/tabbox.css:51
(Diff revision 1)
>  
>  tab:last-of-type {
>    padding-inline-end: 2px;
>  }
>  
> -tab[visuallyselected="true"]:not(:-moz-window-inactive) {
> +:not(:-moz-window-inactive) tab[visuallyselected="true"] {

And here.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8941353 [details]
Bug 1429293 - Rewrite selectors using :-moz-window-inactive so that they continue working when it only matches root.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211668/diff/1-2/

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8941353 [details]
Bug 1429293 - Rewrite selectors using :-moz-window-inactive so that they continue working when it only matches root.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211668/diff/2-3/

Comment 8

[:Gijs (he/him)]

Comment on attachment 8941353 [details]
Bug 1429293 - Rewrite selectors using :-moz-window-inactive so that they continue working when it only matches root.

https://reviewboard.mozilla.org/r/211668/#review217466

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Since we decided to use invalidator for document state pseudo-classes, this change is no longer needed, and we can keep using :-moz-window-inactive wherever people want.