Closed Bug 700856 Opened 13 years ago Closed 13 years ago

Move styles from {Media,Image,Video}Document to external stylesheets

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla11

People

(Reporter: jaws, Assigned: jaws)

References

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 file, 3 obsolete files)

Patch for bug 700856 v1.1
7.89 KB, patch
dao
: review+
jaws
: review+
Details | Diff | Splinter Review 
Before we fix bug 376997, we should move the styles currently found within MediaDocument.cpp, ImageDocument.cpp, and VideoDocument.cpp to an external stylesheet.

The external stylesheets will be introduced by bug 700854.
Attached patch Patch for bug 700856 (obsolete) — — Splinter Review 
This patch moves the styles from ImageDocument.cpp and VideoDocument.cpp to MediaDocument.css. I'm not happy with the names of the CSS classes though.

I have converted tabprompts-bgtexture.png to a data uri based on the comments in Bug 376997.

Pushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=30935a6a204d
Assignee: nobody → jwein
Status: NEW → ASSIGNED
Attachment #573057 - Flags: review?(roc)
Attachment #573057 - Flags: review?(dao)
Comment on attachment 573057 [details] [diff] [review]
Patch for bug 700856

Review of attachment 573057 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/document/src/VideoDocument.cpp
@@ +127,5 @@
> +    element->SetAttr(kNameSpaceID_None, nsGkAtoms::_class, 
> +        NS_LITERAL_STRING("nonTopLevelVideo"), PR_TRUE);
> +  else
> +    body->SetAttr(kNameSpaceID_None, nsGkAtoms::_class, 
> +        NS_LITERAL_STRING("topLevelVideo"), PR_TRUE);

Let's move this to MediaDocument so that any kind of document can use these classes.

We can simplify things by always putting the class on the "host element" (i.e. the first child of <body>). Adding attributes to the <body> doesn't seem to be allowed by HTML5. Also we can simplify things by just having a "topLevel" class when it's top-level; you can check for "not toplevel" using the :not() selector.
Can't you just avoid adding the stylesheet for non-top-level video documents?
Comment on attachment 573057 [details] [diff] [review]
Patch for bug 700856

Clearing review based on the comments I've received. I'll upload a newer patch later today.
Attachment #573057 - Flags: review?(roc)
Attachment #573057 - Flags: review?(dao)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> We can simplify things by always putting the class on the "host element"
> (i.e. the first child of <body>).

Based on the current styles of VideoDocument, we need to be able to style the <body> tag so we can set a background on it as well as height/width and margin/padding.

> Adding attributes to the <body> doesn't seem to be allowed by HTML5.

I'm not sure where you are referencing this from? Section 3.2.3 of the HTML5 spec says that "class" attribute is valid on all HTML elements, and section 4.4.1 says that the <body> tag allows the global attributes (which refers to section 3.2.3).

> Also we can simplify things by just having a
> "topLevel" class when it's top-level; you can check for "not toplevel" using
> the :not() selector.

Thanks, that's a much better idea than what I had started with.

(In reply to Dão Gottwald [:dao] from comment #3)
> Can't you just avoid adding the stylesheet for non-top-level video documents?

Non-top-level video documents previously had the video styled with |position: absolute; top: 0; left: 0; width: 100%; height: 100%|. Do you think we should remove these styles?
Attached patch Patch for bug 700856 (obsolete) — — Splinter Review 
I have moved the |topLevel| class assignment to MediaDocument and added an extra class to the body element when the synthetic document is a video for styling the background.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=c74c1dd24d5a
Attachment #573057 - Attachment is obsolete: true
Attachment #573343 - Flags: review?(roc)
Attachment #573343 - Flags: review?(dao)
(In reply to Jared Wein [:jwein and :jaws] from comment #5)
> Based on the current styles of VideoDocument, we need to be able to style
> the <body> tag so we can set a background on it as well as height/width and
> margin/padding.

OK. That means we need a class or attribute on the <html> or <body> elements I guess... Or else a custom pseudo-class, but that seems overkill.

> > Adding attributes to the <body> doesn't seem to be allowed by HTML5.
> 
> I'm not sure where you are referencing this from? Section 3.2.3 of the HTML5
> spec says that "class" attribute is valid on all HTML elements, and section
> 4.4.1 says that the <body> tag allows the global attributes (which refers to
> section 3.2.3).

http://www.whatwg.org/specs/web-apps/current-work/#read-media
"User agents may add content to the head element of the Document, or attributes to the element host element, e.g. to link to a style sheet or an XBL binding, to provide a script, to give the document a title, to make the media autoplay, etc."

Technically adding a class or attribute to <html> or <body> is not allowed. But I think adding a custom attribute to one of those elements should be allowed, so how about we add a mozTopLevel attribute to <html>? Then you can do anything you need. I'll send an email to the WHATWG list about this.

> (In reply to Dão Gottwald [:dao] from comment #3)
> > Can't you just avoid adding the stylesheet for non-top-level video documents?
> 
> Non-top-level video documents previously had the video styled with
> |position: absolute; top: 0; left: 0; width: 100%; height: 100%|. Do you
> think we should remove these styles?

We should keep them. I think Dao might have misunderstood the situation.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> http://www.whatwg.org/specs/web-apps/current-work/#read-media
> "User agents may add content to the head element of the Document, or
> attributes to the element host element, e.g. to link to a style sheet or an
> XBL binding, to provide a script, to give the document a title, to make the
> media autoplay, etc."

BTW the reason this matters is that Web content can reach into an <iframe> and see the DOM we've constructed, if the resource is same-origin with the container.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> (In reply to Jared Wein [:jwein and :jaws] from comment #5)
> > Based on the current styles of VideoDocument, we need to be able to style
> > the <body> tag so we can set a background on it as well as height/width and
> > margin/padding.
> 
> OK. That means we need a class or attribute on the <html> or <body> elements
> I guess... Or else a custom pseudo-class, but that seems overkill.

Do you really need to set the background conditionally on whether the document is toplevel or not?

Can't we always set the background, and then for non-toplevel documents, make the video (or other object) cover the body background, and set a black background color on it in case the video is transparent?
(In reply to Jared Wein [:jwein and :jaws] from comment #5)
> (In reply to Dão Gottwald [:dao] from comment #3)
> > Can't you just avoid adding the stylesheet for non-top-level video documents?
> 
> Non-top-level video documents previously had the video styled with
> |position: absolute; top: 0; left: 0; width: 100%; height: 100%|. Do you
> think we should remove these styles?

While we should keep them, I see no value in putting them in a stylesheet. If you don't do this, you don't need the topLevel class.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Do you really need to set the background conditionally on whether the
> document is toplevel or not?
> 
> Can't we always set the background, and then for non-toplevel documents,
> make the video (or other object) cover the body background, and set a black
> background color on it in case the video is transparent?

Probably we shouldn't set any background on non-toplevel documents. If we supported transparent videos, a transparent video in an <iframe> should be transparent. So I'll go ahead and email WHATWG.
(In reply to Dão Gottwald [:dao] from comment #10)
> (In reply to Jared Wein [:jwein and :jaws] from comment #5)
> > (In reply to Dão Gottwald [:dao] from comment #3)
> > > Can't you just avoid adding the stylesheet for non-top-level video documents?
> > 
> > Non-top-level video documents previously had the video styled with
> > |position: absolute; top: 0; left: 0; width: 100%; height: 100%|. Do you
> > think we should remove these styles?
> 
> While we should keep them, I see no value in putting them in a stylesheet.
> If you don't do this, you don't need the topLevel class.

Ok, I misunderstood your previous comment. Yeah, I'll keep the non-toplevel styles outside of the stylesheet. I'll change from having a MediaDocument.css to having separate ImageDocument.css and VideoDocument.css so we can style the <body> tag without needing an extra way to differentiate synthetic image and video documents.
I personally prefer the custom attribute/class but I don't mind really.
Attached patch Patch for bug 700856 (obsolete) — — Splinter Review 
Updated the patch to be based off of the new patch for bug 700854.
Attachment #573343 - Attachment is obsolete: true
Attachment #573343 - Flags: review?(roc)
Attachment #573343 - Flags: review?(dao)
Attachment #573392 - Flags: review?(roc)
Attachment #573392 - Flags: review?(dao)
Like I said in comment #11 I don't think we should add this background for non-toplevel videos.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> Like I said in comment #11 I don't think we should add this background for
> non-toplevel videos.

This stylesheet is only linked if the synthetic document is toplevel.
Comment on attachment 573392 [details] [diff] [review]
Patch for bug 700856

Review of attachment 573392 [details] [diff] [review]:
-----------------------------------------------------------------

OK. Rename the stylesheet TopLevelVideoDocument.css?
Attachment #573392 - Flags: review?(roc) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17)
> Comment on attachment 573392 [details] [diff] [review] [diff] [details] [review]
> Patch for bug 700856
> 
> Review of attachment 573392 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> OK. Rename the stylesheet TopLevelVideoDocument.css?

Sounds good to me. I'll also rename the image one to TopLevelImageDocument.css.
Comment on attachment 573392 [details] [diff] [review]
Patch for bug 700856

>+body > video {
>+  position: absolute;
>+  top: 0;
>+  right: 0;
>+  bottom: 0;
>+  left: 0;
>+  margin: auto;
>+  box-shadow: 0 0 15px #000;
>+}
>+
>+body > video:focus {
>+  outline-width: 0;
>+}

What's the point of "body >" in these selectors? Is this stylesheet loaded in differently structured documents, some of which contain videos that it shouldn't style?
Attachment #573392 - Flags: review?(dao) → feedback+
Updated patch to apply on top of changes in the patch for bug 700854.

> What's the point of "body >" in these selectors? Is this stylesheet loaded in
> differently structured documents, some of which contain videos that it 
> shouldn't style?

The "body >" in the selectors was unnecessary and was an oversight on my behalf.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1644d8438081
Attachment #573392 - Attachment is obsolete: true
Attachment #573563 - Flags: review?(dao)
Comment on attachment 573563 [details] [diff] [review]
Patch for bug 700856 v1.1

Carrying forward r+ from roc.
Attachment #573563 - Flags: review+
Attachment #573563 - Flags: review?(dao) → review+
Blocks: 693958
Blocks: 537718
https://hg.mozilla.org/mozilla-central/rev/b77ec6e8eb8c
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Comment on attachment 573563 [details] [diff] [review]
Patch for bug 700856 v1.1

>   mozilla::dom::VideoDocument* doc = new mozilla::dom::VideoDocument();
>-  if (!doc) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
>+  NS_ENSURE_TRUE(doc, NS_ERROR_OUT_OF_MEMORY);

This check should have been removed altogether instead of refactoring dead code...
Depends on: 708431
Blocks: 713487
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: