1417200 - Make -moz-border-*-colors chrome only

Status: RESOLVED FIXED

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

Description

[Jeff Muizelaar [:jrmuizel]]

Supporting this is a pain for webrender. It would be nice not to expose them to webcontent and then eventually remove from chrome.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

We end up hitting a fallback on Gmail because of this. Gmail is using them on 0 width borders so removing the properties wont have a visual effect.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1417200: Make -moz-border-colors chrome only.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1417200: Stop using -moz-border-*-colors in forms.css.

This removes one px of "padding" in:

 <progress style="-moz-appearance: none"></progress>

But it'd be worse to not be able as an author to override those somehow.

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8932558 [details]
Bug 1417200: Make -moz-border-colors chrome only.

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

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Please send an intent to unship to dev-platform.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #0)
> Supporting this is a pain for webrender. It would be nice not to expose them
> to webcontent and then eventually remove from chrome.

FWIW, the CSS working group is actually considering adding something similar to this to the spec, see https://github.com/w3c/csswg-drafts/issues/1172#issuecomment-295565255 and https://drafts.csswg.org/css-backgrounds-4/#propdef-border-color

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Oh, it's not considering adding. It has been added.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC-6 (less responsive Nov 5 ~ Dec 16) from comment #5)
> Please send an intent to unship to dev-platform.

Yup, I planned to do that once I got the patches reviewed, but I guess I can also do it now.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC-6 (less responsive Nov 5 ~ Dec 16) from comment #6)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #0)
> > Supporting this is a pain for webrender. It would be nice not to expose them
> > to webcontent and then eventually remove from chrome.
> 
> FWIW, the CSS working group is actually considering adding something similar
> to this to the spec, see
> https://github.com/w3c/csswg-drafts/issues/1172#issuecomment-295565255 and
> https://drafts.csswg.org/css-backgrounds-4/#propdef-border-color

What's the point of doing this in border-color? It's effectively superseded by border-image I'd say... I guess for simple cases is less annoying than writing a gradient?

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8932558 [details]
Bug 1417200: Make -moz-border-colors chrome only.

https://reviewboard.mozilla.org/r/203616/#review209418

r=me with the comments addressed.

::: commit-message-c7eec:1
(Diff revision 2)
> +Bug 1417200: Make -moz-border-colors chrome only. r?xidorn

It seems that this also makes `-moz-border-*-colors` no longer subproperties of some border shorthands.

This should be mentioned in the commit message as well, since it isn't immediately clear from a brief look.

::: layout/style/test/property_database.js
(Diff revision 2)
>      "-moz-linear-gradient(10 10px -45deg, red, blue) repeat",
>      "-moz-linear-gradient(10px 10 -45deg, red, blue) repeat",
>    );
>  }
>  
> -if (IsCSSPropertyPrefEnabled("layout.css.unset-value.enabled")) {

Why do you move this? It doesn't seem to me this is related to this change.

::: widget/reftests/progressbar-fallback-default-style-ref.html
(Diff revision 2)
> +  <link rel='stylesheet' href='chrome://reftest/content/progress.css'>
>    <style>
> -    div.progress-element {
> -      /**
> -       * The purpose of this test is to not show the native style.
> -       * -moz-appearance: progressbar;

`progress/style.css` is using `-moz-appearance: progressbar`, so you probably shouldn't use that in this reference file.

Maybe create a separate css file instead, I guess...

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8932558 [details]
Bug 1417200: Make -moz-border-colors chrome only.

https://reviewboard.mozilla.org/r/203616/#review209428

::: layout/style/test/property_database.js
(Diff revision 2)
>      "-moz-linear-gradient(10 10px -45deg, red, blue) repeat",
>      "-moz-linear-gradient(10px 10 -45deg, red, blue) repeat",
>    );
>  }
>  
> -if (IsCSSPropertyPrefEnabled("layout.css.unset-value.enabled")) {

It's not removed, it's moved under the block where the chrome-only properties are defined.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8932558 [details]
Bug 1417200: Make -moz-border-colors chrome only.

https://reviewboard.mozilla.org/r/203616/#review209428

> It's not removed, it's moved under the block where the chrome-only properties are defined.

Err, sorry, I misread. It's moved because it uses `gCSSProperties["-moz-border-*-colors"]`. Thus, I reordered the block so it came after the potential definition of it.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8932558 [details]
Bug 1417200: Make -moz-border-colors chrome only.

https://reviewboard.mozilla.org/r/203616/#review209418

> `progress/style.css` is using `-moz-appearance: progressbar`, so you probably shouldn't use that in this reference file.
> 
> Maybe create a separate css file instead, I guess...

Note that `progress/style.css` adds `-moz-appearance: none` right under.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #12)
> Comment on attachment 8932558 [details]
> Bug 1417200: Make -moz-border-colors chrome only.
> 
> https://reviewboard.mozilla.org/r/203616/#review209428
> 
> > It's not removed, it's moved under the block where the chrome-only properties are defined.
> 
> Err, sorry, I misread. It's moved because it uses
> `gCSSProperties["-moz-border-*-colors"]`. Thus, I reordered the block so it
> came after the potential definition of it.

-moz-border-*-colors are moving into a "if (false)" block, so those mentions it should be removed directly, otherwise you would see JS errors I suppose.

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8932934 [details]
Bug 1417200: Stop using -moz-border-*-colors in forms.css.

https://reviewboard.mozilla.org/r/203938/#review209442

::: commit-message-640ef:5
(Diff revision 1)
> +Bug 1417200: Stop using -moz-border-*-colors in forms.css. r?xidorn
> +
> +This removes one px of "padding" in:
> +
> + <progress style="-moz-appearance: none"></progress>

So it doesn't change anything when `-moz-appearance` is `progressbar` right?

Comment 16

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC-6 (less responsive Nov 5 ~ Dec 16) from comment #14)
> -moz-border-*-colors are moving into a "if (false)" block, so those mentions
> it should be removed directly, otherwise you would see JS errors I suppose.

Right, that's why I added a couple if (gCSSProperties["-moz-border-*-colors"]) around those. But happy to just remove it I guess.

Comment 17

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

These patches look like they would break the FinderHighlighter.jsm outline styling. While that's currently preffed off, it'd be nice to just fix it here or in a dep? ( https://dxr.mozilla.org/mozilla-central/rev/c2248f85346939d3e0b01f57276c440ccb2d16a1/toolkit/modules/FinderHighlighter.jsm#51 and later )

There also seem to be a number of shared styles in toolkit/themes/shared/in-content/common.inc.css used to override other styling that will be applied to non-system-privileged documents that use this stylesheet for HTML (e.g. about:studies, but I think there's other places as well). Are we sure that doesn't need updating, either?

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8932558 [details]
Bug 1417200: Make -moz-border-colors chrome only.

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

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8932934 [details]
Bug 1417200: Stop using -moz-border-*-colors in forms.css.

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

Comment 20

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 2 changesets with 35 changes to 35 files
remote: (33607ab330e6 modifies servo/components/style/properties/longhand/border.mako.rs from Servo; not enforcing peer review)
remote: (33607ab330e6 modifies servo/components/style/properties/shorthand/border.mako.rs from Servo; not enforcing peer review)
remote: (1 changesets contain changes to protected servo/ directory: 33607ab330e6)
remote: ************************************************************************
remote: you do not have permissions to modify files under servo/
remote: 
remote: the servo/ directory is kept in sync with the canonical upstream
remote: repository at https://github.com/servo/servo
remote: 
remote: changes to servo/ are only allowed by the syncing tool and by sheriffs
remote: performing cross-repository "merges"
remote: 
remote: to make changes to servo/, submit a Pull Request against the servo/servo
remote: GitHub project
remote: ************************************************************************
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.e_prevent_vendored hook failed
abort: push failed on remote

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8932558 [details]
Bug 1417200: Make -moz-border-colors chrome only.

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

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8932934 [details]
Bug 1417200: Stop using -moz-border-*-colors in forms.css.

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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d483a0d9ceb1
Make -moz-border-colors chrome only. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/e6af6f8ae63e
Stop using -moz-border-*-colors in forms.css. r=xidorn

Comment 24

[Mozilla Autoland]

We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.

Comment 25

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/d483a0d9ceb1
https://hg.mozilla.org/mozilla-central/rev/e6af6f8ae63e

Comment 26

[Richard Marti (:Paenglab)]

On Thunderbird which uses menulists and menupopups, there both are -moz-appearance: none;, in chrome it's not possible to set a border-color, the -moz-border-*-colors are always used. Is this expected? Or should all this -moz-border-*-colors be changed to border-color like in the second patch of this bug?

Comment 27

[Emilio Cobos Álvarez (:emilio)]

(In reply to Richard Marti (:Paenglab) from comment #26)
> On Thunderbird which uses menulists and menupopups, there both are
> -moz-appearance: none;, in chrome it's not possible to set a border-color,
> the -moz-border-*-colors are always used. Is this expected? Or should all
> this -moz-border-*-colors be changed to border-color like in the second
> patch of this bug?

From chrome code you should be able to reset -moz-border-*-colors to none, and then set border-color. It's not pretty... We could make a shorthand to reset them I assume, but that's not great if we eventually want to get rid of them.

Comment 28

[Richard Marti (:Paenglab)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #27)
> From chrome code you should be able to reset -moz-border-*-colors to none,
> and then set border-color. It's not pretty... We could make a shorthand to
> reset them I assume, but that's not great if we eventually want to get rid
> of them.

This looks for me as regression as this wasn't needed before this bug. Now this needs also fixing in different places for bug 1423453. Maybe it's now time to get rid of this -moz-border-*-colors.

Comment 29

[Kohei Yoshino]

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/moz-border-colors-properties-have-been-removed/

Comment 31

[Chris Mills [:cmills]]

I've documented this by:

Adding a note to the Fx 59 rel notes: https://developer.mozilla.org/en-US/Firefox/Releases/59#CSS_2
Adding entries for these properties into the browser compat data project: https://github.com/mdn/browser-compat-data/pull/881

Let me know if this looks OK. Thanks!

Comment 32

[Emilio Cobos Álvarez (:emilio)]

Looks good to me!

I've noticed another thing though. @-moz-document hasn't been removed from the web (only on Nightly and early Beta for now).

Comment 33

[Chris Mills [:cmills]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #32)
> Looks good to me!
> 
> I've noticed another thing though. @-moz-document hasn't been removed from
> the web (only on Nightly and early Beta for now).

Ah, OK. I've removed the rel note for now, and updated the note I added to the @document page to say it is only Nightly and beta for now. Will update again when it is enabled in release.

Comment 34

[Kohei Yoshino]

I don't see any `ifdef EARLY_BETA_OR_EARLIER` in the patch. Just chrome vs content, right?

Comment 35

[Emilio Cobos Álvarez (:emilio)]

(In reply to Kohei Yoshino [:kohei] from comment #34)
> I don't see any `ifdef EARLY_BETA_OR_EARLIER` in the patch. Just chrome vs
> content, right?

This patch yes, it's only chrome vs. content, so these properties are effectively inaccessible. But the patch in bug 1035091 definitely has those.

Comment 36

[Kohei Yoshino]

Okay, so my site compat notes are correct, I think.

https://www.fxsitecompat.com/en-CA/docs/2015/moz-document-support-will-be-dropped/
https://www.fxsitecompat.com/en-CA/docs/2017/moz-border-colors-properties-have-been-removed/

Comment 37

[Emilio Cobos Álvarez (:emilio)]

(In reply to Kohei Yoshino [:kohei] from comment #36)
> Okay, so my site compat notes are correct, I think.
> 
> https://www.fxsitecompat.com/en-CA/docs/2015/moz-document-support-will-be-
> dropped/
> https://www.fxsitecompat.com/en-CA/docs/2017/moz-border-colors-properties-
> have-been-removed/

Yup, that looks fine, thanks!