Hide -moz-prefixed display values from web content

RESOLVED FIXED in Firefox 62

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: xidorn, Assigned: emilio)

Tracking

(Blocks 2 bugs, {dev-doc-complete, site-compat})

Trunk
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(4 attachments)

There are several -moz-prefixed XUL-specific display values exposed to web content, which are not speced properly, and we do not have enough test coverage on them with other features either. Exposing them could cause compatibility issues e.g. bug 1288472. We should exclude them from display ktable when parsing CSS for web content.
I guess we should make `display` use parsing function, rather than keeping it in the normal keyword path and hacking in the normal path like what we are currently doing.
Thanks for filing this.

(Along with display:-moz-box, I expect XUL's "display:-moz-grid" to inadvertantly cause compat problems similar to bug 1288472, as modern CSS grid gets enabled & it gains mindshare, and as authors ask for it in the wrong way, & with declarations in the wrong order)
I wonder how much we might break, for old sites using display: -moz-box, but without a display: flex. But maybe our webkit flexy display aliases let us get away with it?
Is -moz-box ever expected to work in web content? I don't believe so... I think they are used just because people see there is something with that name, and they think it is the Mozilla version of -webkit-box...
> Is -moz-box ever expected to work in web content? I don't believe so... I think they are used just because people see there is something with that name, and they think it is the Mozilla version of -webkit-box...

Yeah, I agree that's probably the reasoning so many people included it. 

But I suspect there's enough content like the following (crappy first example I found on GitHub search): https://github.com/obashtovoj/5course/blob/a958f8d34e1dd9b2e514ca142722418f77dd8a59/Web-programming/HTML5/HTML5/PropertiesOfFlexibleBoxModel/2_46.css

Now, if we removed -moz-box support, the -webkit-box alias stuff would kick in and nothing would break. But I wouldn't be surprised by sites doing UA sniffing to only serve old -moz-box. 

(Of course, this is just paranoid speculation. We could implement it behind a pref and surf the web and see what happens.)
Blocks: unprefix
Priority: -- → P3
See Also: → 1255315
Depends on: 879275, 914360
Assignee: nobody → emilio
Duplicate of this bug: 914360
As I said in bug 914360, I think we can unexpose the following values:
-moz-grid -moz-inline-grid -moz-grid-group -moz-grid-line
-moz-stack -moz-inline-stack
-moz-deck -moz-popup -moz-groupbox

I'm less sure about -moz-box / -moz-inline-box though.
I suggest we wait with those and gather some telemetry about their use.
Blocks: 1466170
Comment on attachment 8982840 [details]
Bug 1288572: Don't hide -moz-box / -moz-inline-box yet.

https://reviewboard.mozilla.org/r/248702/#review254898

::: commit-message-440c3:3
(Diff revision 1)
> +Bug 1288572: Don't hide -moz-box / -moz-inline-box yet. r?mats
> +
> +I'd really prefer to not land this patch, but...

Well, I guess it's the right call, and reduces the risk of unshipping the other values.

Looks fine to do this for now :)
Comment on attachment 8982840 [details]
Bug 1288572: Don't hide -moz-box / -moz-inline-box yet.

https://reviewboard.mozilla.org/r/248702/#review254900
Attachment #8982840 - Flags: review?(mats) → review+
Comment on attachment 8982833 [details]
Bug 1288572: Introduce css(parse_condition).

https://reviewboard.mozilla.org/r/248698/#review254938

::: servo/components/style_derive/to_css.rs:238
(Diff revision 1)
>      pub function: Option<Override<String>>,
>      pub comma: bool,
>      pub dimension: bool,
>      pub keyword: Option<String>,
>      pub aliases: Option<String>,
> +    pub parse_condition: Option<Path>,

Maybe we should add a struct for `Parse` deriving. `parse_condition` doesn't seem to be related to `ToCss` at all.

Also, we should probably have comment on `parser::Parse` stating that the trait can be derived for some types, and it uses css attributes and it has extra attribute to control the condition.
Attachment #8982833 - Flags: review?(xidorn+moz)
Comment on attachment 8982834 [details]
Bug 1288572: Hide -moz- display values from content behind a pref.

https://reviewboard.mozilla.org/r/248700/#review254940

::: servo/components/style/values/specified/box.rs:22
(Diff revision 2)
>  use values::generics::box_::VerticalAlign as GenericVerticalAlign;
>  use values::specified::{AllowQuirks, Number};
>  use values::specified::length::{LengthOrPercentage, NonNegativeLength};
>  
> +#[cfg(feature = "gecko")]
> +fn moz_display_values_enabled_on_content(context: &ParserContext) -> bool {

`moz_display_values_enabled` should be enough... Having `on_content` makes it more confusing to me...

::: servo/components/style/values/specified/box.rs:26
(Diff revision 2)
> +    context.chrome_rules_enabled() ||
> +    unsafe {
> +        structs::StaticPrefs_sVarCache_layout_css_xul_display_values_content_enabled
> +    }

Indentations? I bet rustfmt doesn't like this.
Attachment #8982834 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8982867 [details]
Bug 1288572: Update test expectations.

https://reviewboard.mozilla.org/r/248722/#review254942

::: layout/base/crashtests/crashtests.list:362
(Diff revision 2)
>  load 560441-1.xhtml
>  load 560447-1.html
>  load 564063-1.html
>  load 567292-1.xhtml
>  load 569018-1.html
> -load 570038-1.html
> +asserts(4) load 570038-1.html

Maybe just pref on the preference for this test as well... When we eventually remove that pref and unship the values completely, this test can probably be removed as well.
Attachment #8982867 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8982833 [details]
Bug 1288572: Introduce css(parse_condition).

https://reviewboard.mozilla.org/r/248698/#review254938

> Maybe we should add a struct for `Parse` deriving. `parse_condition` doesn't seem to be related to `ToCss` at all.
> 
> Also, we should probably have comment on `parser::Parse` stating that the trait can be derived for some types, and it uses css attributes and it has extra attribute to control the condition.

Per that reasoning, we should probably also move `aliases` to this new attribute. If you agree on moving both, mind if I do it as a followup bug?
Comment on attachment 8982833 [details]
Bug 1288572: Introduce css(parse_condition).

(See comment 20)
Attachment #8982833 - Flags: review?(xidorn+moz)
Comment on attachment 8982833 [details]
Bug 1288572: Introduce css(parse_condition).

https://reviewboard.mozilla.org/r/248698/#review254938

> Per that reasoning, we should probably also move `aliases` to this new attribute. If you agree on moving both, mind if I do it as a followup bug?

Okay.
Comment on attachment 8982833 [details]
Bug 1288572: Introduce css(parse_condition).

https://reviewboard.mozilla.org/r/248698/#review254990

OK, as far as you're splitting the attributes in a followup bug, and adding the document for this, r=me.
Attachment #8982833 - Flags: review?(xidorn+moz) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dba6641c8d1a
Introduce css(parse_condition). r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf368e565ab8
Hide -moz- display values from content behind a pref. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab37c2da6d45
Don't hide -moz-box / -moz-inline-box yet. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd2ab05ec584
Update test expectations. r=xidorn
Browser compat data is updated for this: https://developer.mozilla.org/en-US/docs/Web/CSS/::selection#Browser_compatibility and I think that's all the doc update needed here, but please let me know if we need anything else.
Sorry, wrong bug :/.

Anyway, ExE-Boss updated the docs for this: https://developer.mozilla.org/en-US/docs/Web/CSS/display#XUL_values so I think this one is also dev-doc-complete.
The Browser Compatibility table fix is however still pending in https://github.com/mdn/browser-compat-data/pull/1955.
Blocks: 879275
No longer depends on: 879275
Depends on: 1477553
You need to log in before you can comment on or make changes to this bug.