Details

Reviewers

heycam
emilio

Commits

rMOZILLACENTRAL8c74a8452ad5: Bug 1305732 - part5 : set CSS properties directly on '::cue'. r=heycam,emilio…
rMOZILLACENTRAL809f38c8859a: Bug 1305732 - part5 : set CSS properties directly on '::cue'. r=heycam,emilio

Bugzilla Bug ID

1305732

Summary

According to the spec [1], we have to set those CSS properties on the root node, and then this root node would have a child node, background box [2], which would contain all other child nodes.

In our case, the background box is cueDiv [3].

In theory, all those properties set on the root node should be inherited by the background box. However, when the background box is a pseudo element ::cue, they won't be directly inherit from the the background box's parent, inherited styles would acutally come from video instead.

Therefore, we have to directly set these properties on the pseudo element and mark them as !important to avoid being overrided by user style script.

[1] https://www.w3.org/TR/webvtt1/#ref-for-list-of-webvtt-node-objects-9
[2] https://www.w3.org/TR/webvtt1/#webvtt-cue-background-box
[3] https://searchfox.org/mozilla-central/rev/11712bd3ce7454923e5931fa92eaf9c01ef35a0a/dom/media/webvtt/vtt.jsm#533-534

Diff Detail

Repository

rMOZILLACENTRAL mozilla-central

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

alwu created this revision.Jun 24 2019, 6:34 PM

Herald added a project: Restricted Project. · View Herald TranscriptJun 24 2019, 6:34 PM

Harbormaster completed remote builds in B85388: Diff 122198.Jun 24 2019, 6:34 PM

alwu added a parent revision: D35693: Bug 1305732 - part1 : don't show the box with zero width or height..Jun 24 2019, 6:35 PM

alwu added a child revision: D35695: Bug 1305732 - part6 : modify wpts..

phab-bot requested review of this revision.Jun 24 2019, 6:35 PM

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".

phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".

phab-bot removed a project: Restricted Project.

Herald added a subscriber: emilio. · View Herald TranscriptJun 24 2019, 6:35 PM

Can you add a test for this?

This revision is now accepted and ready to land.Jul 2 2019, 6:12 AM

Becasue of bug 1321488, now we acutally are not allowed to add overflow-wrap on the pseudo element according to the spec.
So now this patch should find a way to make ::cue inherit the overflow-wrapproperty because it seems not work on the ::cue.

You can just allow it and set it as important. But probably the spec should be fixed to allow overflow-wrap anyhow, so maybe add the whitelist entry and file a spec issue?

Hi, @emilio, the spec actually has defined overflow-wrap should be break-word for the root node.

However, the problem I got is that, even if I have set it on the cue box, this property seems not working on its child div element which is ::cue. Is it correct that the pseudo element won't inherit this property from its parent div?

As this property doesn't take effect on the pseudo element, I was thinking about adding it directly to the pseudo element, and found it works.

So if the pseudo element should inherit this property, I think maybe we should figure out why it doesn't work on ::cue. If it's an expected behavior, then I will allow overflow-wrap setting on ::cue and set it break-word !important on html.css to avoid user override it.

Thank you!

I am confused, overflow-wrap is inherited, if you set it on an ancestor of the cue the cue should have it as well.

alwu updated this revision to Diff 126905.Jul 3 2019, 10:57 PM

alwu retitled this revision from Bug 1305732 - part2 : set 'break-word' style in cue pseudo element. to Bug 1305732 - part5 : set CSS properties directly on '::cue'..

alwu edited the summary of this revision. (Show Details)

alwu removed a reviewer: heycam.

This revision now requires review to proceed.Jul 3 2019, 10:57 PM

Herald added a subscriber: jya. · View Herald TranscriptJul 3 2019, 10:57 PM

Harbormaster completed remote builds in B88907: Diff 126905.Jul 3 2019, 10:57 PM

alwu removed a parent revision: D35693: Bug 1305732 - part1 : don't show the box with zero width or height..Jul 3 2019, 11:06 PM

alwu added a parent revision: D36409: Bug 1562021 - part4 : wrap channel and listener clean-up to a function..

alwu edited parent revisions, added: D36849: Bug 1305732 - part4 : background box should be a inline node.; removed: D36409: Bug 1562021 - part4 : wrap channel and listener clean-up to a function..

alwu added reviewers: heycam, emilio.Jul 5 2019, 7:50 PM

heycam accepted this revision.Jul 8 2019, 2:56 AM

This revision is now accepted and ready to land.Jul 8 2019, 2:56 AM

Ah, I see, so this is because the cue div is not inheriting from the parent but from the <video>, that makes sense. Could we add a comment to html.css about why these are needed?

dom/media/webvtt/vtt.jsm
608	This needs to be `!important` too, otherwise author code can override it setting `--cue-writing-mode` to whatever value.
layout/style/res/html.css
787	May be worth to make this `var(--cue-writing-mode, inherit)` so that when using getComputedStyle you get the inherited writing-mode.

This revision now requires changes to proceed.Jul 8 2019, 9:03 AM

alwu updated this revision to Diff 128697.Jul 8 2019, 6:16 PM

alwu marked 2 inline comments as done.

Harbormaster completed remote builds in B90253: Diff 128697.Jul 8 2019, 6:16 PM

emilio added inline comments.Jul 8 2019, 11:25 PM

dom/media/webvtt/vtt.jsm
608	This doesn't seem done? `setProperty` has an important flag as a third argument.

alwu added inline comments.Jul 8 2019, 11:50 PM

dom/media/webvtt/vtt.jsm
608	@emilio do you mean adding `!important` in `html.css` is not enough?

emilio added inline comments.Jul 8 2019, 11:52 PM

dom/media/webvtt/vtt.jsm
608	No, the custom property needs to be `!important` as well, otherwise I can change the custom property value to change the writing mode.

alwu updated this revision to Diff 128918.Jul 8 2019, 11:59 PM

alwu marked 2 inline comments as done.

Harbormaster completed remote builds in B90408: Diff 128918.Jul 8 2019, 11:59 PM

alwu added inline comments.Jul 8 2019, 11:59 PM

dom/media/webvtt/vtt.jsm
608	Ah, got it. This `!important` is added for the value of variable itself.

emilio accepted this revision.Jul 9 2019, 12:12 AM

emilio added inline comments.

dom/media/webvtt/vtt.jsm
608	Right, thanks, this looks good. Probably we should do the same for `--cue-font-size`, though that's less of an issue since it's meant to be user-overridable.

This revision is now accepted and ready to land.Jul 9 2019, 12:12 AM

alwu updated this revision to Diff 128939.Jul 9 2019, 1:42 AM

Harbormaster completed remote builds in B90418: Diff 128939.Jul 9 2019, 1:42 AM

Closed by commit rMOZILLACENTRAL809f38c8859a: Bug 1305732 - part5 : set CSS properties directly on '::cue'. r=heycam,emilio (authored by alwu). · Explain WhyJul 9 2019, 1:46 AM

This revision was automatically updated to reflect the committed changes.

alwu added a commit: rMOZILLACENTRAL809f38c8859a: Bug 1305732 - part5 : set CSS properties directly on '::cue'. r=heycam,emilio.

alwu added a commit: rMOZILLACENTRAL8c74a8452ad5: Bug 1305732 - part5 : set CSS properties directly on '::cue'. r=heycam,emilio….Jul 11 2019, 11:47 AM