1763667 - Consider merging image & video document CSS from toolkit/themes/shared/media into their stylesheets in layout/style

Status: RESOLVED FIXED

(Toolkit :: Themes, task)

Description

[Daniel Holbert [:dholbert]]

Emilio noticed in https://phabricator.services.mozilla.com/D143099#4678171 that we have this file:
https://searchfox.org/mozilla-central/rev/3269d4c928ef0d8310c2f57634e9b6057aa636e9/toolkit/themes/shared/media/TopLevelVideoDocument.css
...which only has a single CSS rule (to set a black background on top-level video documents).

I'm not sure if there's a reason that this file exists as a distinct thing from the identically-named stylesheet in layout/style at this point -- that other longer stylesheet is:
https://searchfox.org/mozilla-central/rev/3269d4c928ef0d8310c2f57634e9b6057aa636e9/layout/style/TopLevelVideoDocument.css

Perhaps they can be merged together?

Dao or jaws, perhaps you know why these files exist as distinct things, & whether there's any reason we should keep them that way?

The same question/situation applies to this similar pair of image-doc stylesheets -- I'm not sure if there's a reason they are distinct?
https://searchfox.org/mozilla-central/rev/3269d4c928ef0d8310c2f57634e9b6057aa636e9/layout/style/TopLevelImageDocument.css
https://searchfox.org/mozilla-central/rev/3269d4c928ef0d8310c2f57634e9b6057aa636e9/toolkit/themes/shared/media/TopLevelImageDocument.css

Comment 1

[Daniel Holbert [:dholbert]]

It sort of looks like the versions in layout/style are focused on layout-and-functionality-related CSS, vs. the version in themes/shared/media exists for more presentation & potentially-theme-related CSS.

This separation makes some sense, but I'm not how much it's useful vs. historical at this point; and it feels a little silly for the video one which only has a single rule to set a plain black background.

Maybe at some point in the past, the theme-related styles needed to be separated so that the /shared/ one could be overridden on a per-platform basis -- but (if so) it looks like we don't actually do that anymore. I do see in e.g. https://hg.mozilla.org/mozilla-central/rev/b965a94cd13e / Bug 1291268, we removed some platform-specific additional versions of these /themes stylesheets. So I wonder if this separation was for the benefit of that no-longer-present platform-specific theming, and hence isn't useful anymore?

Comment 2

[Daniel Holbert [:dholbert]]

(jaws, I suspect you can add some clarity here on whether this separation still makes sense & is useful going forward. If it is useful, that's fine & we can close this bug out as INVALID/WONTFIX. But if there's not a strong reason to keep the two distinct-but-identically-named stylesheets as separate files, then perhaps we could simplify things a bit by merging them.)

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

I'm okay with merging them. At the time of creation we had at least one more rule, but also that was Firefox 25 and I think XUL addons still existed then to allow for overriding themes. As this is no longer the case, we should move towards a simpler setup. Thanks for the ping.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Bug 1763667 part 1: Merge our two copies of TopLevelImageDocument.css and TopLevelVideoDocument.css. r?emilio!,jaws!

For both files, we've been keeping around an extra copy in
toolkit/themes/shared/media/ with 1-2 CSS rules related to presentation.

This was probably to allow XUL-style extensions and themes to override the
presentation, back when we supported that form of extension. But there's not a
good reason to have this separation anymore, so let's simplify and combine the
files of the same name.

This patch also removes one no-longer-esepcially-useful "N.B." comment (which
refers to a file that's now alongside the styles in question). This patch also
adds an explanatory comment to explain why we bother to set color for images
(which was initially not clear to me).

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: Bug 1763667 part 2: Use :root instead of html/body, on selectors in TopLevelImageDocument.css and TopLevelVideoDocument.css. r?emilio

This patch shouldn't change behavior. It just gets us a bit more consistency
between these two stylesheets, and it makes these rules more explicit about the
fact that they're intending to style the root.

Depends on D143395

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c4c4292e412
part 1: Merge our two copies of TopLevelImageDocument.css and TopLevelVideoDocument.css. r=emilio,jaws

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e395c42667c
part 2: Use :root instead of html/body, on selectors in TopLevelImageDocument.css and TopLevelVideoDocument.css. r=emilio,jaws

Comment 8

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/8c4c4292e412
https://hg.mozilla.org/mozilla-central/rev/5e395c42667c