658431 - Move generic page styling into separate stylesheet for reuse in other in-content UIs

Status: VERIFIED FIXED

(Toolkit :: Themes, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Existing and new in-content UI wants to use the styling that the addons manager has. To help facilitate that, the generic styling in the addons manager needs to be moved into a stylesheet that can be shared by all in-content UI pages.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

I used about:permissions as a guinea pig (see bug 658530), but I suspect we'll find other things that should be shared as more UI is moved to be in-content.

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 536207 [details] [diff] [review]
Patch v1

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

Couple of things I'd like to see here but this is basically there. Most of my comments to the gnomestripe css apply everywhere. Can we use in-content instead of incontent? I keep reading it wrong.

::: toolkit/mozapps/extensions/content/extensions.xml
@@ +900,5 @@
>                              label="&cmd.showPreferencesUnix.label;"
>                              tooltiptext="&cmd.showPreferencesUnix.tooltip;"
>  #endif
>                              oncommand="document.getBindingParent(this).showPreferences();"/>
> +                <xul:button anonid="enable-btn"  class="incontent-button enable"

should just get rid of the extra space here, seems unnecessary

::: toolkit/themes/gnomestripe/global/incontent-ui.css
@@ +1,1 @@
> +/* ***** BEGIN LICENSE BLOCK *****

I think the naming of this should match the naming of the dtd.

@@ +38,5 @@
> +@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> +
> +
> +/* Page background */
> +page {

I think you can use :root as a selector to match the document root, might be better than requiring a fixed element.

@@ +107,5 @@
> +}
> +
> +
> +/* Inner content */
> +.inner-content {

This doesn't seem to be used anywhere, what's it for?

@@ +122,5 @@
> +}
> +
> +
> +/* Buttons */
> +.incontent-button[disabled="true"] {

I don't think a general rule of hiding disabled buttons is right. It makes sense for the add-ons manager but probably not in a lot of other cases. Arguably we should be using hidden there too tbh.

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 536207 [details] [diff] [review]
Patch v1

I see some about:addons centric stuff here that I'm not sure should be shared like this. For now I'd like to see the search, inconent-button and header-related classes go. We can let the stylesheet grow as we add more consumers.

> <?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
> <?xml-stylesheet href="chrome://mozapps/content/extensions/extensions.css"?>
>+<?xml-stylesheet href="chrome://global/skin/incontent-ui.css"?>
> <?xml-stylesheet href="chrome://mozapps/skin/extensions/extensions.css"?>

Can chrome://mozapps/skin/extensions/extensions.css @include chrome://global/skin/incontent-ui.css?

>+page {

Use :root here

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #3)
> Can chrome://mozapps/skin/extensions/extensions.css @include
> chrome://global/skin/incontent-ui.css?

Do you mean @import? Or %include via the preprocessor? 

I considered %include, but I want this to be usable by extensions that provide in-content UI. 

I hadn't considered @import - out of curiosity, what benefits would that have over using xml-stylesheet?

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #2)
> Can we use in-content instead of incontent? I keep reading it wrong.

Heh...

> > +/* Inner content */
> > +.inner-content {
> 
> This doesn't seem to be used anywhere, what's it for?

Used in about:permissions (bug 658530) - I'll move it there, since it's a new addition.

Comment 6

[Dão Gottwald [:dao]]

I meant @import, sorry. The idea is that extensions.css can pull whatever styles it wants, extensions.xul doesn't need to know about them.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 537960 [details] [diff] [review]
Patch v2

nit: I'd prefer in-content_UI...

Comment 9

[Dão Gottwald [:dao]]

or just inContentUI

Comment 10

[Robert Kaiser]

As a note, I still think that "in-content UI" is a misnomer, as it's chrome, including chrome privileges, and doesn't even run in content processes when we turn them on. "In-tab UI" would be so much clearer about what it is... ;-)

Comment 11

[Dão Gottwald [:dao]]

It's the content area. in-tab doesn't really make much sense... it's attached to and represented by a tab, not in it.

Comment 12

[Robert Kaiser]

It's also not inside content, but shown instead of content. And we actually refer to what panorama manages as "tabs" and what Fennec has as "tabs", so we clearly refer to the content area as "tab" even if the correct definition might be different. Not sure if that discussion in the end belong into this bug, though ;-)

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 537960 [details] [diff] [review]
Patch v2

These parts look ok, what happened to the button stylings though?

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #13)
> These parts look ok, what happened to the button stylings though?

Moved them into bug 658530, as per comment 3.

Whats your take on the naming, Dave? in-contentUI, in-content_UI, or inContentUI? (This type of UI has always been referred to as "in-content", so I don't like changing it to "in-tab".)

Comment 15

[Dave Townsend [:mossop]]

I concur with inContentUI.css, we don't use underscores anywhere else in the theme files that I can see.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

inContentUI it is. 

https://hg.mozilla.org/integration/mozilla-inbound/rev/aeed034b74d0

Comment 17

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/aeed034b74d0

Comment 18

[Vlad [QA]]

How can i verify this?
Thanks

Comment 19

[Paul [pwd]]

Go to the AOM and view source (ctrl+U).

Comment 20

[Ioana (away)]

Verified with Addons Manager and about:permissions on Firefox 8.0 Beta 6, Firefox 9.0 Aurora and Firefox 10.0 Central.