Closed Bug 1475511 Opened Last year Closed Last year

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: gingerbread_man, Assigned: emilio)

Details

(Keywords: dev-doc-complete, Whiteboard: [lang=c++][lang=rust])

Attachments

(6 files)

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.
Emilio, what's the current status of @-moz-document bits?
Flags: needinfo?(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.. :)
Mentor: emilio
Flags: needinfo?(emilio)
Keywords: good-first-bug
Whiteboard: [lang=c++][lang=rust]
Yeah, doesn't sound very hard, although I'm not sure whether it's going to be a good first bug :)
Priority: -- → P4
Is this a good first bug to work on
Hello,

I'm an absolute beginner and would like to work on this.
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.
Keywords: good-first-bug
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.
Attachment #8993436 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8993437 [details]
Bug 1475511: Simplify MozdocumentRule::Match.

https://reviewboard.mozilla.org/r/258186/#review265372
Attachment #8993437 - Flags: review?(xidorn+moz) → review+
Attachment #8993435 - Flags: review?(xidorn+moz) → review?(bzbarsky)
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?
Attachment #8993434 - Flags: review?(xidorn+moz) → review+
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.
Attachment #8993438 - Flags: review?(xidorn+moz)
(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.
Attachment #8993435 - Flags: review?(xidorn+moz) → review?(bzbarsky)
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.
Attachment #8993677 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8993438 [details]
Bug 1475511: Add @-moz-document media-document(image|video|plugin).

https://reviewboard.mozilla.org/r/258188/#review265428
Attachment #8993438 - Flags: review?(xidorn+moz) → review+
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.
Attachment #8993435 - Flags: review?(bzbarsky) → review+
Assignee: nobody → emilio
Status: NEW → ASSIGNED
Pushed by emilio@crisal.io:
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
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).
Keywords: dev-doc-needed
Mentor: emilio
You need to log in before you can comment on or make changes to this bug.