1405311 - Remove :-moz-system-metric pseudo-class, replace with system metric media queries.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

After :-moz-system-metric is UA / chrome only, we can replace its current uses for system metric media queries, and remove this pseudo-class, which would be nicer to avoid duplication, and also would make them be evaluated just once.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1405311: Remove uses of :-moz-system-metric.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1405311: Remove :-moz-system-metric pseudo-class.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1405311: Not require a subtree restyle when system metrics change now that :-moz-system-metric no longer exists.

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

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8916237 [details]
Bug 1405311: Remove uses of :-moz-system-metric.

https://reviewboard.mozilla.org/r/187468/#review192972

Looks good to me, with one issue below.

Also this touches lots of toolkit and browser code, so may be worth double-check by a browser peer.

::: layout/reftests/css-mediaqueries/reftest.list
(Diff revision 1)
>  == mq_print_maxheight_updown.xhtml mq_print-ref.xhtml
>  == mq_print_minheight_updown.xhtml mq_print-ref.xhtml
>  == mq_print_minwidth_updown.xhtml mq_print-ref.xhtml
>  
>  pref(layout.css.scoped-style.enabled,true) == scoped-mq-update.html scoped-mq-update-ref.html
> -== system-metrics-1.html system-metrics-1-ref.html

The removal of test which checking this feature should probably be moved to the next patch, but it probably doesn't matter too much.

::: toolkit/content/minimal-xul.css:124
(Diff revision 1)
> -scrollbarbutton[sbattr="scrollbar-up-top"]:not(:-moz-system-metric(scrollbar-start-backward)),
> -scrollbarbutton[sbattr="scrollbar-down-top"]:not(:-moz-system-metric(scrollbar-start-forward)),
> -scrollbarbutton[sbattr="scrollbar-up-bottom"]:not(:-moz-system-metric(scrollbar-end-backward)),
> -scrollbarbutton[sbattr="scrollbar-down-bottom"]:not(:-moz-system-metric(scrollbar-end-forward)) {
> +@media not (-moz-scrollbar-start-backward) {
> +  scrollbarbutton[sbattr="scrollbar-up-top"],
> +  scrollbarbutton[sbattr="scrollbar-down-top"],
> +  scrollbarbutton[sbattr="scrollbar-up-bottom"],
> +  scrollbarbutton[sbattr="scrollbar-down-bottom"] {
> -  display: none;
> +    display: none;
> -}
> +  }
> +}

Apparently this rewrite is incorrect.

The four selectors uses different metrics. They are `scrollbar-{start,end}-{forward-backward}`.

Unless you can prove that these four are always identical, this cannot be rewritten this way.

(So, it isn't. Specifically, our GTK widget supports all kinds of combination [1])

[1] https://searchfox.org/mozilla-central/rev/b53e29293c9e9a2905f4849f4e3c415e2013f0cb/widget/gtk/nsLookAndFeel.cpp#427-441

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8916238 [details]
Bug 1405311: Remove :-moz-system-metric pseudo-class.

https://reviewboard.mozilla.org/r/187470/#review192974

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4)
> Comment on attachment 8916237 [details]
> Bug 1405311: Remove uses of :-moz-system-metric.
> 
> https://reviewboard.mozilla.org/r/187468/#review192972
> 
> Looks good to me, with one issue below.
> 
> Also this touches lots of toolkit and browser code, so may be worth
> double-check by a browser peer.
> 
> ::: layout/reftests/css-mediaqueries/reftest.list
> (Diff revision 1)
> >  == mq_print_maxheight_updown.xhtml mq_print-ref.xhtml
> >  == mq_print_minheight_updown.xhtml mq_print-ref.xhtml
> >  == mq_print_minwidth_updown.xhtml mq_print-ref.xhtml
> >  
> >  pref(layout.css.scoped-style.enabled,true) == scoped-mq-update.html scoped-mq-update-ref.html
> > -== system-metrics-1.html system-metrics-1-ref.html
> 
> The removal of test which checking this feature should probably be moved to
> the next patch, but it probably doesn't matter too much.
> 
> ::: toolkit/content/minimal-xul.css:124
> (Diff revision 1)
> > -scrollbarbutton[sbattr="scrollbar-up-top"]:not(:-moz-system-metric(scrollbar-start-backward)),
> > -scrollbarbutton[sbattr="scrollbar-down-top"]:not(:-moz-system-metric(scrollbar-start-forward)),
> > -scrollbarbutton[sbattr="scrollbar-up-bottom"]:not(:-moz-system-metric(scrollbar-end-backward)),
> > -scrollbarbutton[sbattr="scrollbar-down-bottom"]:not(:-moz-system-metric(scrollbar-end-forward)) {
> > +@media not (-moz-scrollbar-start-backward) {
> > +  scrollbarbutton[sbattr="scrollbar-up-top"],
> > +  scrollbarbutton[sbattr="scrollbar-down-top"],
> > +  scrollbarbutton[sbattr="scrollbar-up-bottom"],
> > +  scrollbarbutton[sbattr="scrollbar-down-bottom"] {
> > -  display: none;
> > +    display: none;
> > -}
> > +  }
> > +}
> 
> Apparently this rewrite is incorrect.
> 
> The four selectors uses different metrics. They are
> `scrollbar-{start,end}-{forward-backward}`.
> 
> Unless you can prove that these four are always identical, this cannot be
> rewritten this way.
> 
> (So, it isn't. Specifically, our GTK widget supports all kinds of
> combination [1])
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> b53e29293c9e9a2905f4849f4e3c415e2013f0cb/widget/gtk/nsLookAndFeel.cpp#427-441

Err, nice catch, will ammend.

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 8916237 [details]
Bug 1405311: Remove uses of :-moz-system-metric.

Moving to a browser theme owner. I'm not qualified to review css changes in toolkit.

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8916239 [details]
Bug 1405311: Not require a subtree restyle when system metrics change now that :-moz-system-metric no longer exists.

https://reviewboard.mozilla.org/r/187472/#review193856

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8916237 [details]
Bug 1405311: Remove uses of :-moz-system-metric.

https://reviewboard.mozilla.org/r/187468/#review193942

::: toolkit/content/minimal-xul.css:124
(Diff revision 1)
>  .scale-slider {
>    -moz-binding: url(chrome://global/content/bindings/scale.xml#scaleslider);
>    -moz-user-focus: normal;
>  }
>  
> -scrollbarbutton[sbattr="scrollbar-up-top"]:not(:-moz-system-metric(scrollbar-start-backward)),
> +@media not (-moz-scrollbar-start-backward) {

Apart from what Xidorn said, I don't think this syntax is correct. Media queries are weird. '@media not all and (-moz-scrollbar-start-backward)' would be the "right" way to use 'not' here, but '@media (-moz-scrollbar-start-backward: 0)' is preferable.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8916237 [details]
Bug 1405311: Remove uses of :-moz-system-metric.

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

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8916238 [details]
Bug 1405311: Remove :-moz-system-metric pseudo-class.

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

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8916239 [details]
Bug 1405311: Not require a subtree restyle when system metrics change now that :-moz-system-metric no longer exists.

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

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8916237 [details]
Bug 1405311: Remove uses of :-moz-system-metric.

https://reviewboard.mozilla.org/r/187468/#review193956

::: toolkit/content/minimal-xul.css:124
(Diff revision 1)
>  .scale-slider {
>    -moz-binding: url(chrome://global/content/bindings/scale.xml#scaleslider);
>    -moz-user-focus: normal;
>  }
>  
> -scrollbarbutton[sbattr="scrollbar-up-top"]:not(:-moz-system-metric(scrollbar-start-backward)),
> +@media not (-moz-scrollbar-start-backward) {

Yup, you're totally right. Ammended.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 8916237 [details]
Bug 1405311: Remove uses of :-moz-system-metric.

https://reviewboard.mozilla.org/r/187468/#review193964

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f758ec788914
Remove uses of :-moz-system-metric. r=xidorn,dao

Comment 16

[Emilio Cobos Álvarez (:emilio)]

x(In reply to Pulsebot from comment #15)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/autoland/rev/f758ec788914
> Remove uses of :-moz-system-metric. r=xidorn,dao

I'm landing this first so https://github.com/servo/servo/pull/18848 doesn't break the build, will land the rest after that.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e30f8c099e20
Remove :-moz-system-metric pseudo-class. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/049867300f7e
Not require a subtree restyle when system metrics change now that :-moz-system-metric no longer exists. r=heycam

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/f758ec788914
https://hg.mozilla.org/mozilla-central/rev/e30f8c099e20
https://hg.mozilla.org/mozilla-central/rev/049867300f7e