1446470 - Parse @-moz-document url-prefix() on content.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

All the breakage from @-moz-document removal uses url-prefix(). We should just parse this and disable the rest everywhere.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1446470: Cleanup @-moz-document parsing a bit.

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1446470: Allow @-moz-document url-prefix() on content.

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

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1446470: Make the moz-document-in-content pref false by default.

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

Can we have this bit behind a pref as well so we can try removing it at some point?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> Can we have this bit behind a pref as well so we can try removing it at some
> point?

Sure

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1446470: Add another pref to control the url-prefix hack.

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8959637 [details]
Bug 1446470: Cleanup @-moz-document parsing a bit.

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

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8959638 [details]
Bug 1446470: Allow @-moz-document url-prefix() on content.

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

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8959639 [details]
Bug 1446470: Make the moz-document-in-content pref false by default.

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

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8959639 [details]
Bug 1446470: Make the moz-document-in-content pref false by default.

The pref-flip will land a bit earlier because Tom happened to write a patch for it already in bug 1422245. I think we should land the rest of the patches in this bug too though.

If you think it's not necessary, please let me know. I thought YouTube had not fixed their stuff yet, but they apparently have, and the other two breakages seem a bit less risky. My fear is that there's tons of other web pages out there relying on this, so it seems safer to let 61 ride the trains with only url-prefix, then try to toggle the hack pref for a later release.

Maybe we should keep empty-url-prefix disabled in nightly / early beta, wdyt?

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8959637 [details]
Bug 1446470: Cleanup @-moz-document parsing a bit.

https://reviewboard.mozilla.org/r/228448/#review234494

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8959638 [details]
Bug 1446470: Allow @-moz-document url-prefix() on content.

https://reviewboard.mozilla.org/r/228450/#review234496

::: servo/components/style/stylesheets/document_rule.rs:192
(Diff revision 2)
> +
> +        let condition = DocumentCondition(conditions);
> +
> +        if !condition.allowed_in(context) {
> +            return Err(input.new_custom_error(
> +                StyleParseErrorKind::UnsupportedAtRule("-moz-document".into())
> +            ))
> +        }
> +

It's probably just personal favor, but I don't understand how it makes sense to add this many blank lines. It seems to me this function is simple and short enough that it doesn't really need any further separation in its code.

::: servo/components/style/stylesheets/document_rule.rs:219
(Diff revision 2)
> +
> +        if context.stylesheet_origin != Origin::Author {
> +            return true;
> +        }
> +
> +        if unsafe { structs::StylePrefs_sMozDocumentEnabledInContent } {
> +            return true;
> +        }
> +
> +        // Allow a single url-prefix() for compatibility.
> +        //
> +        // See bug 1446470 and dependencies.
> +        if self.0.len() != 1 {
> +            return false;
> +        }
> +

This function as well...

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8959639 [details]
Bug 1446470: Make the moz-document-in-content pref false by default.

https://reviewboard.mozilla.org/r/228452/#review234498

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8959718 [details]
Bug 1446470: Add another pref to control the url-prefix hack.

https://reviewboard.mozilla.org/r/228538/#review234500

::: dom/ipc/ContentPrefs.cpp:170
(Diff revision 1)
>    "layout.css.individual-transform.enabled",
>    "layout.css.initial-letter.enabled",
>    "layout.css.isolation.enabled",
>    "layout.css.mix-blend-mode.enabled",
>    "layout.css.moz-document.content.enabled",
> +  "layout.css.moz-document.url-prefix-hack.enabled",

You need a DOM peer to review this.

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

I think we should land this, and also we should post an update to the original intent to unship thread to explain the current plan.

Also I think it makes sense to keep empty-url-prefix disabled in nightly to see if we can have it removed completely at some point. Not making something work for a short term then stop working again helps avoiding unnecessary confusion (e.g. when people try to bisect for some issue).

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8959718 [details]
Bug 1446470: Add another pref to control the url-prefix hack.

Olli, can you review the ContentPrefs.cpp change? It's just adding a new pref as requested in comment 4.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Looks like similar stuff was tried to be fixed in bug 1341414, but got backed out.
So, fine, the pref isn't need that early, but since we do the same thing for other prefs...

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8959718 [details]
Bug 1446470: Add another pref to control the url-prefix hack.

https://reviewboard.mozilla.org/r/228538/#review234552

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43a3fc3416d2
Allow @-moz-document url-prefix() on content. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/dd8ea031beae
Add another pref to control the url-prefix hack. r=xidorn,smaug

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6bec5f4329ac
followup: Enable the right pref on a reftest on a CLOSED TREE. r=me

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/038bee98aa0a
Another followup, fix the expectation for stylo-disabled. r=me

Comment 22

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/43a3fc3416d2
https://hg.mozilla.org/mozilla-central/rev/dd8ea031beae
https://hg.mozilla.org/mozilla-central/rev/6bec5f4329ac
https://hg.mozilla.org/mozilla-central/rev/038bee98aa0a

Comment 23

[Karl Dubost💡 :karlcow]

And a first breakage due to the release :)
https://webcompat.com/issues/15956

@-moz-document url-prefix()
{
	table.plans tr > *, table.comparison tr > *
	{
		height: 100%;
	}
}

Comment 24

[Emilio Cobos Álvarez (:emilio)]

So, this has been behaving this way on pre-release since bug 1035091. That kind of code is the only reason for this.

This bug will make that _not_ break on release. Does that make sense? Or am I missing something else? If you toggle layout.css.moz-document.url-prefix-hack.enabled to true (which will be true on release and late beta) then the page should render correctly.

Comment 25

[Masatoshi Kimura [:emk]]

The latest Nightly <https://hg.mozilla.org/mozilla-central/rev/c8dbb4ed05f38f40ef3607a6e36545bd95b8a287> does not contain this fix yet. This bug fix will *fix* the webcompat regression.

Comment 26

[Karl Dubost💡 :karlcow]

yup. sorry for the noise.

Comment 27

[Will Bamberg [:wbamberg]]

I've updated the compat data to capture this: https://developer.mozilla.org/en-US/docs/Web/CSS/@document#Browser_compatibility

Marking dev-doc-complete, but please comment if we need anything else.