864811 - Use :-moz-any in the new PanelUI CSS where appropriate

Status: RESOLVED WONTFIX

(Firefox :: Theme, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

I came across this today:

> #PanelUI-mainView toolbarbutton:not([disabled]):-moz-any([open],[checked],:hover:active) {
>}

Bug 561154 describes the performance issues with using -moz-any as the right-most selector. In this scenario, it is equivalent to using the universal selector. We should just expand this rule so that it doesn't have the performance issues.

Also see https://developer.mozilla.org/en-US/docs/CSS/:any#Issues_with_performance_and_specificity for some examples.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 2

[Frank Yan (:fryn)]

Comment on attachment 743433 [details] [diff] [review]
Patch

Review of attachment 743433 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for flagging me for review on this. I'll ask dbaron in-person tomorrow to confirm my comment below before finalizing the review.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ -211,5 @@
>                0 0 2px hsla(210,54%,20%,.1);
>  }
>  
> -#PanelUI-subViews toolbarbutton:not([disabled]):-moz-any([open],[checked],:hover:active),
> -#PanelUI-mainView toolbarbutton:not([disabled]):-moz-any([open],[checked],:hover:active) {

I'm pretty sure that it's okay to use :-moz-any() as _part_ of the right-most selector as long as there is a faster bucket that the right-most selector also falls into. In this case, the right-most selector is considered firstly a tag selector.

Comment 3

[Frank Yan (:fryn)]

Comment on attachment 743433 [details] [diff] [review]
Patch

Review of attachment 743433 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for flagging me for review on this. I'll ask dbaron in-person tomorrow to confirm my comment below before finalizing the review.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +184,5 @@
>  #customization-palette .toolbarbutton-text {
>    display: none;
>  }
>  
> +:-moz-any(#PanelUI-mainView, #PanelUI-subViews) toolbarbutton {

One thing to watch out for is that the specificity of :-moz-any() is incorrect. It uses the specificity of a pseudo-class rather than the specificity of its arguments. This could be a problem if there are also selector rules like `.foo toolbarbutton.bar` that also apply to these elements.

See bug 561154.

@@ -211,5 @@
>                0 0 2px hsla(210,54%,20%,.1);
>  }
>  
> -#PanelUI-subViews toolbarbutton:not([disabled]):-moz-any([open],[checked],:hover:active),
> -#PanelUI-mainView toolbarbutton:not([disabled]):-moz-any([open],[checked],:hover:active) {

I'm pretty sure that it's okay to use :-moz-any() as _part_ of the right-most selector as long as there is a faster bucket that the right-most selector also falls into. In this case, the right-most selector is considered firstly a tag selector.

Comment 4

[Frank Yan (:fryn)]

Ack, stupid splinter review, dammit!

Comment 5

[Frank Yan (:fryn)]

Comment on attachment 743433 [details] [diff] [review]
Patch

Review of attachment 743433 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments addressed.

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +184,5 @@
>  #customization-palette .toolbarbutton-text {
>    display: none;
>  }
>  
> +:-moz-any(#PanelUI-mainView, #PanelUI-subViews) toolbarbutton {

Please make sure that these rules still are applied after the decreased specificity here.

@@ -211,5 @@
>                0 0 2px hsla(210,54%,20%,.1);
>  }
>  
> -#PanelUI-subViews toolbarbutton:not([disabled]):-moz-any([open],[checked],:hover:active),
> -#PanelUI-mainView toolbarbutton:not([disabled]):-moz-any([open],[checked],:hover:active) {

I verified with David Baron that the removal of :-moz-any() is not necessary here.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Nits addressed: https://hg.mozilla.org/projects/jamun/rev/71d47560397f

Thanks for the speedy review!

Comment 7

[:Gijs (out for now; he/him)]

Attachment: Screenshot

This broke the borders of the widgets in the customization panel. :-(

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 744547 [details]
Screenshot

I'm unable to reproduce this on latest Jamun using a fresh profile on Win7. Could there have been something wrong with your local build?

Comment 9

[Frank Yan (:fryn)]

(In reply to Jared Wein [:jaws] from comment #8)
> Comment on attachment 744547 [details]
> Screenshot
> 
> I'm unable to reproduce this on latest Jamun using a fresh profile on Win7.
> Could there have been something wrong with your local build?

His screenshot is of OS X, I think. Could it be that there are more specific selectors in other stylesheets on OS X?

Comment 10

[:Gijs (out for now; he/him)]

(adding an attachment on a bug should put you in the CC list by default. Grrr, bugzilla. Sorry for the late response, hello from Paris!)

(In reply to Frank Yan (:fryn) from comment #9)
> (In reply to Jared Wein [:jaws] from comment #8)
> > Comment on attachment 744547 [details]
> > Screenshot
> > 
> > I'm unable to reproduce this on latest Jamun using a fresh profile on Win7.
> > Could there have been something wrong with your local build?
> 
> His screenshot is of OS X, I think. Could it be that there are more specific
> selectors in other stylesheets on OS X?

This is indeed OS X, and I did check on a nightly with a fresh profile (and just did again, to be sure! :-) ).

Comment 11

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/projects/ux/rev/71d47560397f

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Backout of patch

I backed it out locally on OS X and it fixed the issue for me. Frank is on the right track here. Since switching to :-moz-any, the specificity of these rules was changed in a detrimental way.

The patch didn't actually make any changes that needed to happen, and so it would be easiest if we just did a straight backout while we can.

We should also merge this UX pretty promptly, as it is a highly visible regression for users of UX.

Comment 13

[:Gijs (out for now; he/him)]

Comment on attachment 744982 [details] [diff] [review]
Backout of patch

Yeah, this works.

Still, even if this doesn't fix anything, it was a good cleanup to attempt, I guess... so it might be worth doing it again but then adjusting something else for OS X so as to fix this issue? I would look myself but I'm on holiday until Sunday night. I may have time to take a peek tomorrow morning, but no promises. :-)

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Well, it's a good clean up attempt, but until bug 561154 is fixed it's not really worth the time. The change in specificity is drastic enough that it will make future modifications to the CSS have unexpected and unintended consequences.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Thanks for the review!

Backed out on Jamun, I'll close the bug now so it doesn't have to wait until the merge to UX.

https://hg.mozilla.org/projects/jamun/rev/b21b5e5e9ad7