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

RESOLVED FIXED in Firefox 61

Status

()

defect
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

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

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

All the breakage from @-moz-document removal uses url-prefix(). We should just parse this and disable the rest everywhere.
Comment hidden (mozreview-request)
Can we have this bit behind a pref as well so we can try removing it at some point?
(Assignee)

Comment 5

a year ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
No longer depends on: 1035091
(Assignee)

Comment 10

a year ago
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?
Flags: needinfo?(xidorn+moz)
Attachment #8959639 - Flags: review?(xidorn+moz)

Comment 11

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

https://reviewboard.mozilla.org/r/228448/#review234494
Attachment #8959637 - Flags: review?(xidorn+moz) → review+

Comment 12

a year ago
mozreview-review
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...
Attachment #8959638 - Flags: review?(xidorn+moz) → review+

Comment 13

a year ago
mozreview-review
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

a year ago
mozreview-review
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.
Attachment #8959718 - Flags: review?(xidorn+moz) → review+
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).
Flags: needinfo?(xidorn+moz)
(Assignee)

Updated

a year ago
Attachment #8959639 - Attachment is obsolete: true
(Assignee)

Comment 16

a year ago
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.
Attachment #8959718 - Flags: review?(bugs)
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

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

https://reviewboard.mozilla.org/r/228538/#review234552
Attachment #8959718 - Flags: review?(bugs) → review+

Comment 19

a year ago
Pushed by ecoal95@gmail.com:
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

a year ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6bec5f4329ac
followup: Enable the right pref on a reftest on a CLOSED TREE. r=me

Comment 21

a year ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/038bee98aa0a
Another followup, fix the expectation for stylo-disabled. r=me
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%;
	}
}
(Assignee)

Comment 24

a year ago
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.
Flags: needinfo?(kdubost)
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.
yup. sorry for the noise.
Flags: needinfo?(kdubost)
(Assignee)

Updated

a year ago
Blocks: 1449753
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.
You need to log in before you can comment on or make changes to this bug.