Closed Bug 1417200 Opened 7 years ago Closed 6 years ago

Make -moz-border-*-colors chrome only

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jrmuizel, Assigned: emilio)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(2 files)

Supporting this is a pain for webrender. It would be nice not to expose them to webcontent and then eventually remove from chrome.
Flags: needinfo?(emilio)
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.
Summary: Unship or make Chrome only -moz-border-*-colors → Unship or make chrome only -moz-border-*-colors
Blocks: 1415203
Assignee: nobody → emilio
Depends on: 1417563
Flags: needinfo?(emilio)
Priority: -- → P3
Please send an intent to unship to dev-platform.
(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
Oh, it's not considering adding. It has been added.
(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.
(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 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...
Attachment #8932558 - Flags: review?(xidorn+moz) → review+
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 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 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.
(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 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?
Attachment #8932934 - Flags: review?(xidorn+moz) → review+
(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.
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?
Depends on: 1422257
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
Pushed by ecoal95@gmail.com:
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
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.
https://hg.mozilla.org/mozilla-central/rev/d483a0d9ceb1
https://hg.mozilla.org/mozilla-central/rev/e6af6f8ae63e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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?
Flags: needinfo?(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.
Flags: needinfo?(emilio)
Depends on: 1423453
(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.
Depends on: 1424251
Summary: Unship or make chrome only -moz-border-*-colors → Make -moz-border-*-colors chrome only
Blocks: 1429723
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!
Flags: needinfo?(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).
Flags: needinfo?(emilio)
(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.
I don't see any `ifdef EARLY_BETA_OR_EARLIER` in the patch. Just chrome vs content, right?
(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.
(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!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: