Xidorn Quan [:xidorn] UTC+11 Reporter
	Description • 8 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 1 • 8 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 2 • 8 years ago

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)

	Mike Taylor [:miketaylr]
	Comment 3 • 8 years ago

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?

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 4 • 8 years ago

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...

	Mike Taylor [:miketaylr]
	Comment 5 • 8 years ago

> 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.)

	Mats Palmgren (inactive)
	Comment 9 • 6 years ago

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.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 11 • 6 years ago
mozreview-review

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 :)

	Mats Palmgren (inactive)
	Comment 12 • 6 years ago
mozreview-review

Comment on attachment 8982840 [details]
Bug 1288572: Don't hide -moz-box / -moz-inline-box yet.

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

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 17 • 6 years ago
mozreview-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.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 18 • 6 years ago
mozreview-review

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.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 19 • 6 years ago
mozreview-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.

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 20 • 6 years ago
mozreview-review-reply

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?

	Emilio Cobos Álvarez (:emilio) Assignee
	Comment 21 • 6 years ago

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

(See comment 20)

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 22 • 6 years ago
mozreview-review-reply

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.

	Xidorn Quan [:xidorn] UTC+11 Reporter
	Comment 23 • 6 years ago
mozreview-review

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.

	Pulsebot
	Comment 24 • 6 years ago

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

	Kohei Yoshino [:kohei]
	Comment 25 • 6 years ago

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/most-of-non-standard-css-display-values-have-been-dropped/

	Bogdan Tara[:bogdan_tara | bogdant]
	Comment 26 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/dba6641c8d1a
https://hg.mozilla.org/mozilla-central/rev/cf368e565ab8
https://hg.mozilla.org/mozilla-central/rev/ab37c2da6d45
https://hg.mozilla.org/mozilla-central/rev/dd2ab05ec584

	Will Bamberg [:wbamberg]
	Comment 27 • 6 years ago

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.

	Will Bamberg [:wbamberg]
	Comment 28 • 6 years ago

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.

	ExE Boss
	Comment 29 • 6 years ago

The Browser Compatibility table fix is however still pending in https://github.com/mdn/browser-compat-data/pull/1955.