Closed Bug 1466715 Opened 2 years ago Closed Last year

Root XUL window/dialog elements -moz-appearance values are inconsistent

Categories

(Toolkit :: Themes, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

Details

Attachments

(1 file)

On macOS, we currently use `-moz-appearance: dialog;`[1] (mapping to NS_THEME_DIALOG) in global.css for XUL root elements while on Windows[2] and Linux[3] we use `-moz-appearance: window;` (mapping to NS_THEME_WINDOW).

From what I can tell NS_THEME_WINDOW behaves the same as NS_THEME_DIALOG on Linux[4], NS_THEME_WINDOW doesn't do anything useful on macOS[5], and neither are implemented on Windows (bug 119737).

[1] https://dxr.mozilla.org/mozilla-central/rev/0ee6b755ab2ee6d2ab79b17cc97bd4e83424cbfc/toolkit/themes/osx/global/global.css#35
[2] https://dxr.mozilla.org/mozilla-central/rev/0ee6b755ab2ee6d2ab79b17cc97bd4e83424cbfc/toolkit/themes/windows/global/global.css#41
[3] https://dxr.mozilla.org/mozilla-central/rev/0ee6b755ab2ee6d2ab79b17cc97bd4e83424cbfc/toolkit/themes/linux/global/global.css#45
[4] All references to NS_THEME_WINDOW and NS_THEME_DIALOG seem to handle the other in the same way.
[5] https://dxr.mozilla.org/mozilla-central/search?q=regexp%3ANS_THEME_WINDOW%5Cb+path%3Acocoa&redirect=false

If we've gotten this far without the distinction being needed then it's probably safe to remove the distinction (we can easily add it back) and simplify global.css to use the same -moz-appearance values across platforms.
The alternative to removing NS_THEME_WINDOW would be to fix the cocoa code to treat NS_THEME_DIALOG and NS_THEME_WINDOW the same in all places and then change https://dxr.mozilla.org/mozilla-central/rev/0ee6b755ab2ee6d2ab79b17cc97bd4e83424cbfc/toolkit/themes/osx/global/global.css#35 to `-moz-appearance: window;` but as I said in comment 0:

(Quoting Matthew N. [:MattN] from comment #0)
> If we've gotten this far without the distinction being needed then it's
> probably safe to remove the distinction (we can easily add it back)…
Comment on attachment 8983258 [details]
Bug 1466715 - Remove unimplemented -moz-appearance:window from XUL document elements on Windows and align on 'dialog' elsewhere.

https://reviewboard.mozilla.org/r/249146/#review255322

Looks like these go back to bug 188254, so I suspect we're not going to miss them... Thanks for the cleanup :)

I wonder if it'd make sense to keep NS_THEME_WINDOW instead of NS_THEME_DIALOG, given we do have a bunch of `NS_THEME_WINDOW_*`. Though I haven't dug whether they're supposed to be related...

In any case r=me, thanks again!

::: toolkit/themes/linux/global/global.css:45
(Diff revision 1)
>  
>  window,
>  page,
>  dialog,
>  wizard {
> -  -moz-appearance: window;
> +  -moz-appearance: dialog;

nit: Can we unify these CSS rules now? If so, probably worth doing in a separate patch and getting dao to review it :)
Attachment #8983258 - Flags: review?(emilio) → review+
Comment on attachment 8983258 [details]
Bug 1466715 - Remove unimplemented -moz-appearance:window from XUL document elements on Windows and align on 'dialog' elsewhere.

https://reviewboard.mozilla.org/r/249146/#review255322

> I wonder if it'd make sense to keep NS_THEME_WINDOW instead of NS_THEME_DIALOG, given we do have a bunch of NS_THEME_WINDOW_*. Though I haven't dug whether they're supposed to be related...

Yeah, I wondered the same thing for both the `NS_THEME_*` variable and the `-moz-appearance` property value but started with the minimal approach. It would be easy to rename in a follow-up if someone has strong feelings.

> nit: Can we unify these CSS rules now? If so, probably worth doing in a separate patch and getting dao to review it :)

Trying to unify this file was how I stumbled across this mess (as I did a three way merge of global.css for the three themes)…  it looks like there is enough divergence in the three global.css files that de-duping may make things harder to understand at this time. As we remove more XUL it should be easier to unify.
Unfortunately though, these values are exposed to web content so simply
removing them without having any telemetry on their use is a bit risky.
For example,
data:text/html,<body style="background:black"><div style="-moz-appearance:window">Hello
Could we perhaps keep the CSS 'window' value for now,
but map it to NS_THEME_DIALOG internally?
(In reply to Mats Palmgren (:mats) from comment #6)
> Unfortunately though, these values are exposed to web content so simply
> removing them without having any telemetry on their use is a bit risky.

Even given that it's Linux-only (see below / comment 0)?

(Quoting Matthew N. [:MattN] from comment #0)
> NS_THEME_WINDOW doesn't do anything useful on macOS[5], and
> neither are implemented on Windows (bug 119737).
> …
> [5]
> https://dxr.mozilla.org/mozilla-central/
> search?q=regexp%3ANS_THEME_WINDOW%5Cb+path%3Acocoa&redirect=false

(In reply to Mats Palmgren (:mats) from comment #7)
> Could we perhaps keep the CSS 'window' value for now,
> but map it to NS_THEME_DIALOG internally?

I think that would be worse for web-compat since the behaviour on macOS would change since they weren't aliases there.

Either way I guess I'll send an Intent To Unship.
Flags: needinfo?(mats)
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #8)
> Even given that it's Linux-only (see below / comment 0)?

Oh, I guess it's not much of a web-compat concern then.
I mistakenly thought it did something on all platforms,
just that it was incompatible.

> Either way I guess I'll send an Intent To Unship.

That sounds good, thanks.
Flags: needinfo?(mats)
So… after doing a GitHub search[1] and then a Google search while writing my intent t0 unship, it seems there's a pattern of using `-moz-appearance:window` on `select`[2][3][4] to workaround bug 649849 so I guess this can't land as-is :(

[1] https://github.com/search?p=1&q=%22moz-appearance%3A+window%22&type=Code
[2] https://css-tricks.com/almanac/properties/a/appearance/#comment-1584201
[3] https://stackoverflow.com/a/12594685
[4] https://stackoverflow.com/a/13959685
Comment on attachment 8983258 [details]
Bug 1466715 - Remove unimplemented -moz-appearance:window from XUL document elements on Windows and align on 'dialog' elsewhere.

Emilio, I reverted the changes to the -moz-appearance:window implementation but left a comment so someone else doesn't waste their time trying to clean up -moz-appearance:window in the future so I'm requesting another review.
Attachment #8983258 - Flags: review+ → review?(emilio)
Component: Widget → Themes
Product: Core → Toolkit
Comment on attachment 8983258 [details]
Bug 1466715 - Remove unimplemented -moz-appearance:window from XUL document elements on Windows and align on 'dialog' elsewhere.

https://reviewboard.mozilla.org/r/249146/#review255714

::: layout/style/nsCSSProps.cpp:395
(Diff revision 3)
>    { eCSSKeyword_radio_label,            NS_THEME_RADIO_LABEL },
>    { eCSSKeyword_button_focus,           NS_THEME_BUTTON_FOCUS },
> +  // `window` is unused in mozilla-central but web developers use it on <select>
> +  // to remove the dropmarker. Care should be taken when changing
> +  // -moz-appearance:window to avoid web compatibility regressions.
> +  // See https://bugzilla.mozilla.org/show_bug.cgi?id=1466715#c10

I think we should clean up all these, but yeah, probably should be a more deliberate thing.

Thanks for adding the comment.
Attachment #8983258 - Flags: review?(emilio) → review+
Comment on attachment 8983258 [details]
Bug 1466715 - Remove unimplemented -moz-appearance:window from XUL document elements on Windows and align on 'dialog' elsewhere.

https://reviewboard.mozilla.org/r/249146/#review255738

::: toolkit/themes/windows/global/global.css:41
(Diff revision 3)
>  
>  window,
>  page,
>  dialog,
>  wizard {
> -  -moz-appearance: window;
> +  -moz-appearance: dialog;

Why set this at all if it doesn't do anything useful?
Priority: -- → P2
Comment on attachment 8983258 [details]
Bug 1466715 - Remove unimplemented -moz-appearance:window from XUL document elements on Windows and align on 'dialog' elsewhere.

https://reviewboard.mozilla.org/r/249146/#review255738

> Why set this at all if it doesn't do anything useful?

For consistency with the hope that we can dedupe the identical rules in the future. Also because it's the -moz-appearance value that makes sense for these document elements… the same question could be asked about the old value before my change too.
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #15)
> Comment on attachment 8983258 [details]
> Bug 1466715 - Change XUL document elements to use -moz-appearance:dialog.
> 
> https://reviewboard.mozilla.org/r/249146/#review255738
> 
> > Why set this at all if it doesn't do anything useful?
> 
> For consistency with the hope that we can dedupe the identical rules in the
> future. Also because it's the -moz-appearance value that makes sense for
> these document elements… the same question could be asked about the old
> value before my change too.

Generally, we don't set -moz-appearance because it makes sense but because we want that appearance. I don't foresee a future where bug 119737 gets fixed, so we should just drop this, as it is a distraction for anyone trying to figure out our window styling on Windows. Keeping it around in order to make rules identical and dedupe them seems backwards.
Comment on attachment 8983258 [details]
Bug 1466715 - Remove unimplemented -moz-appearance:window from XUL document elements on Windows and align on 'dialog' elsewhere.

https://reviewboard.mozilla.org/r/249146/#review256536

::: layout/style/nsCSSProps.cpp:395
(Diff revision 4)
>    { eCSSKeyword_radio_label,            NS_THEME_RADIO_LABEL },
>    { eCSSKeyword_button_focus,           NS_THEME_BUTTON_FOCUS },
> +  // Even though window is meant for OS window styling, web developers use
> +  // `window` on <select> to remove the dropmarker. Care should be taken when
> +  // changing -moz-appearance:window to avoid web compatibility regressions.
> +  // See https://bugzilla.mozilla.org/show_bug.cgi?id=1466715#c10

FWIW, I think we can go ahead and remove this since this hack never worked on Windows. Doesn't need to be in this bug, though.

::: toolkit/themes/osx/global/global.css
(Diff revision 4)
>  
>  window,
>  page,
>  dialog,
>  wizard {
> -  -moz-appearance: dialog;

I'm confused; I thought -moz-appearance: dialog did something on Mac?
Attachment #8983258 - Flags: review?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #18)
> > +  // Even though window is meant for OS window styling, web developers use
> > +  // `window` on <select> to remove the dropmarker. Care should be taken when
> > +  // changing -moz-appearance:window to avoid web compatibility regressions.
> > +  // See https://bugzilla.mozilla.org/show_bug.cgi?id=1466715#c10
> 
> FWIW, I think we can go ahead and remove this since this hack never worked
> on Windows. Doesn't need to be in this bug, though.

I believe you're mistaken.  All valid values for -moz-appearance other
than 'menulist' inhibits the dropdown arrow, for example:
data:text/html,<select style="-moz-appearance:window"><option>hello

Here's the code for that:
https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/layout/forms/nsComboboxControlFrame.cpp#761
Hmm, I thought I read on Stack Overflow that the hack wouldn't work on Windows, but I just tried it on Windows 10 and you're right, it does work.
Comment on attachment 8983258 [details]
Bug 1466715 - Remove unimplemented -moz-appearance:window from XUL document elements on Windows and align on 'dialog' elsewhere.

https://reviewboard.mozilla.org/r/249146/#review256536

> I'm confused; I thought -moz-appearance: dialog did something on Mac?

Yeah, sorry, I got confused with `window` being unimplemented.
Comment on attachment 8983258 [details]
Bug 1466715 - Remove unimplemented -moz-appearance:window from XUL document elements on Windows and align on 'dialog' elsewhere.

https://reviewboard.mozilla.org/r/249146/#review256834

It may still be worth switching from window to dialog on Linux, so we're consistent with Mac and can one day get rid of window?
Attachment #8983258 - Flags: review?(dao+bmo) → review+
Comment on attachment 8983258 [details]
Bug 1466715 - Remove unimplemented -moz-appearance:window from XUL document elements on Windows and align on 'dialog' elsewhere.

https://reviewboard.mozilla.org/r/249146/#review256834

I agree but wasn't sure if you would prefer to leave the blame as-is. I'll change this.
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/2091c74ecd29
Remove unimplemented -moz-appearance:window from XUL document elements on Windows and align on 'dialog' elsewhere. r=dao,emilio
https://hg.mozilla.org/mozilla-central/rev/2091c74ecd29
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.