1475511 - Extend @-moz-document syntax to let users style standalone images and videos

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P4)

Description

[Gingerbread Man]

There's currently no proper way to style standalone image and video documents with userContent.css. The closest is to use regexp to check if the current URL contains certain file extensions. This results in false positives and false negatives, as documents of those types don't necessarily have the extensions in the URL, and those that do aren't necessarily images or videos.

(In reply to Asa Dotzler [:asa] from bug 626776, comment 0)
> I believe we create a synthetic document to hold videos when they're opened
> "stand alone". It would be really awesome if there was a way for third
> parties (user styles, scripts, extensions, etc) to modify the "standalone"
> video.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from bug 626776, comment 3)
> I'd rather support this with a new function for @-moz-document.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Emilio, what's the current status of @-moz-document bits?

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Shouldn't be hard to add another url matching function in:

  https://searchfox.org/mozilla-central/rev/448f792f9652d29daebdad21bf50b03405e40a45/servo/components/style/stylesheets/document_rule.rs#121

and evaluate it as we wish from the evaluate function.

Do we have a strong preference for names or something like that? I'd be happy to mentor the patch if someone is interested in giving this a shot.

I can also do it my self if it's prioritary, but I have a whole other bunch of stuff to get to so.. :)

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Yeah, doesn't sound very hard, although I'm not sure whether it's going to be a good first bug :)

Comment 4

[Gowry Sugathan]

Is this a good first bug to work on

Comment 5

[Devika Sugathan]

Hello,

I'm an absolute beginner and would like to work on this.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Hi! For people wanting to work on this, this would be the way to go. You need to get a build first. For that, see:

  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

Once you have a build, you need to add a new case in URLMatchingFunction::parse:

  https://searchfox.org/mozilla-central/rev/88199de427d3c5762b7f3c2a4860c10734abd867/servo/components/style/stylesheets/document_rule.rs#126

What about calling it "media-document", so that people could do: media-document(video) or media-document(image) or media-document(all)? I don't have a strong opinion on this.

In any case, we'd need something like a:

  #[derive(Copy, Clone, Parse, PartialEq)]
  enum MediaDocumentKind {
      All,
      Image,
      Video,
  }

and in URLMatchingFunction, we'd have:

  MediaDocument(MediaDocumentKind)

Then in URLMatchingFunction::Evaluate, you'd need a case MediaDocument(..), and you need to extend nsIDocument to give you the document kind, via a virtual function, probably...

Maybe this is not so much of a good first bug after all, but if you still want to tackle this I'd be happy to help.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1475511: UrlMatchingFunction -> DocumentMatchingFunction.

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

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1475511: Introduce nsIDocument::MediaDocumentKind.

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

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1475511: Make document condition parsing a bit nicer.

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

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1475511: Simplify MozdocumentRule::Match.

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

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1475511: Add @-moz-document media-document(image|video|plugin).

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

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8993436 [details]
Bug 1475511: Make document condition parsing a bit nicer.

https://reviewboard.mozilla.org/r/258184/#review265370

::: servo/components/style/stylesheets/document_rule.rs:144
(Diff revision 1)
>                      input.expect_string()?.as_ref().to_owned(),
>                  ))
> -            });
> -        }
> +            }
> -
> -        let url = CssUrl::parse(context, input)?;
> +            _ => {
> +                return Err(location.new_custom_error(

You don't need `return` here.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8993437 [details]
Bug 1475511: Simplify MozdocumentRule::Match.

https://reviewboard.mozilla.org/r/258186/#review265372

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8993434 [details]
Bug 1475511: UrlMatchingFunction -> DocumentMatchingFunction.

https://reviewboard.mozilla.org/r/258180/#review265374

It would be better if you can use file moving... but that's probably not working well with git?

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8993438 [details]
Bug 1475511: Add @-moz-document media-document(image|video|plugin).

https://reviewboard.mozilla.org/r/258188/#review265376

::: layout/style/DocumentMatchingFunction.h:22
(Diff revision 1)
> +  AllMediaDocuments,
> +  ImageMediaDocuments,
> +  PluginMediaDocuments,
> +  VideoMediaDocuments,

I don't really like adding so many variants into this enum...

What do you think about just having a single `Media` here, and have pattern includes static string "all", "image", "plugin", "video" correspondingly, and use `aPattern.EqualLiteral("...")` in `Match` function?

"video" and "image" are different on their first letter, so I don't think there is going to be any performance issue, but the code should look nicer.

Comment 16

[Emilio Cobos Álvarez [:emilio]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #15)
> Comment on attachment 8993438 [details]
> Bug 1475511: Add @-moz-document media-document(image|video|plugin).
> 
> https://reviewboard.mozilla.org/r/258188/#review265376
> 
> ::: layout/style/DocumentMatchingFunction.h:22
> (Diff revision 1)
> > +  AllMediaDocuments,
> > +  ImageMediaDocuments,
> > +  PluginMediaDocuments,
> > +  VideoMediaDocuments,
> 
> I don't really like adding so many variants into this enum...
> 
> What do you think about just having a single `Media` here, and have pattern
> includes static string "all", "image", "plugin", "video" correspondingly,
> and use `aPattern.EqualLiteral("...")` in `Match` function?
> 
> "video" and "image" are different on their first letter, so I don't think
> there is going to be any performance issue, but the code should look nicer.

I'd be fine with that though I don't see why would it be better off-hand... It looks slightly nicer on the C++ side, and slightly less nice on the Rust side I guess.

I'll update, but again not ultra-fan of it.

Comment 17

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1475511: Use strings except of enum variants for media-document.

I'm not 100% sure this is a progression, but happy to land this instead of the
previous patch.

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

Comment 18

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8993434 [details]
Bug 1475511: UrlMatchingFunction -> DocumentMatchingFunction.

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

Comment 19

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8993435 [details]
Bug 1475511: Introduce nsIDocument::MediaDocumentKind.

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

Comment 20

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8993436 [details]
Bug 1475511: Make document condition parsing a bit nicer.

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

Comment 21

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8993437 [details]
Bug 1475511: Simplify MozdocumentRule::Match.

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

Comment 22

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8993438 [details]
Bug 1475511: Add @-moz-document media-document(image|video|plugin).

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

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8993677 [details]
Bug 1475511: Use strings except of enum variants for media-document.

https://reviewboard.mozilla.org/r/258360/#review265426

Probably better have it merged with the previous patch.

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8993438 [details]
Bug 1475511: Add @-moz-document media-document(image|video|plugin).

https://reviewboard.mozilla.org/r/258188/#review265428

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8993435 [details]
Bug 1475511: Introduce nsIDocument::MediaDocumentKind.

https://reviewboard.mozilla.org/r/258182/#review265462

::: dom/base/nsIDocument.h:2805
(Diff revision 2)
>    virtual DocumentTheme ThreadSafeGetDocumentLWTheme() const { return Doc_Theme_None; }
>  
> +  // Whether we're a media document or not.
> +  enum class MediaDocumentKind
> +  {
> +    None,

Maybe NotMedia?

::: dom/base/nsIDocument.h:2806
(Diff revision 2)
>  
> +  // Whether we're a media document or not.
> +  enum class MediaDocumentKind
> +  {
> +    None,
> +    Video,

This could really be video or audio... not sure how best to communicate that.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dae2d07dd5c6
UrlMatchingFunction -> DocumentMatchingFunction. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/04005126d036
Introduce nsIDocument::MediaDocumentKind. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8c253425478
Make document condition parsing a bit nicer. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/e70dcbc0e5da
Simplify MozdocumentRule::Match. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/51f57944e6f0
Add @-moz-document media-document(image|video|plugin). r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/245cd4d3a38b
Add a few parsing tests for this. r=me

Comment 27

[Emilio Cobos Álvarez [:emilio]]

Flagging as dev-doc-needed to update the documentation in https://developer.mozilla.org/en-US/docs/Web/CSS/@document (note that this is not exposed by default to content, so can only be used from UA and user stylesheets).

Comment 28

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/dae2d07dd5c6
https://hg.mozilla.org/mozilla-central/rev/04005126d036
https://hg.mozilla.org/mozilla-central/rev/d8c253425478
https://hg.mozilla.org/mozilla-central/rev/e70dcbc0e5da
https://hg.mozilla.org/mozilla-central/rev/51f57944e6f0
https://hg.mozilla.org/mozilla-central/rev/245cd4d3a38b