Closed Bug 1446470 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

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

Attachments

(3 files, 1 obsolete file)

Bug 1446470: Cleanup @-moz-document parsing a bit.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1446470: Allow @-moz-document url-prefix() on content.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1446470: Add another pref to control the url-prefix hack.
59 bytes, text/x-review-board-request
xidorn
: review+
smaug
: review+
Details 
All the breakage from @-moz-document removal uses url-prefix(). We should just parse this and disable the rest everywhere.
Can we have this bit behind a pref as well so we can try removing it at some point?
(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
Blocks: 1035091, 1446442, 1435385
No longer depends on: 1035091
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)
Keywords: site-compat
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 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 on attachment 8959639 [details]
Bug 1446470: Make the moz-document-in-content pref false by default.

https://reviewboard.mozilla.org/r/228452/#review234498
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)
Attachment #8959639 - Attachment is obsolete: true
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 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+
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
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
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%;
	}
}
See Also: → https://webcompat.com/issues/15956
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)
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: