631827 - Use CSS to set the "align" and "pack" attributes on Add-ons Manager to make easier for themes to handle items.

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Pardal Freudenthal (:ShareBird)]

This bug addresses the issues described on Bug 629438 comment #27.
Using inline attributes like we are doing now for the Addons Manager makes hard to themes to skin it in a different way. The use of CSS might make possible to override this attributes for themeing proposes.

The fix for this bug will probably also fixes Bug 569519 - "Add additional classes to make UI more easily theme-able" since classes will be necessary to select the right boxes on the CSS file.

I will start right now to work on a patch. Any suggestion or comments about the direction I need to take?

Comment 1

[Pardal Freudenthal (:ShareBird)]

Attachment: proposed patch

Comment 2

[Dave Townsend [:mossop]]

This will not block the release

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 510078 [details] [diff] [review]
proposed patch

Dave or Blair should review the new classes. description-box vs. description-container, for instance, seems questionable to me.

Comment 4

[Pardal Freudenthal (:ShareBird)]

(In reply to comment #3)
> Comment on attachment 510078 [details] [diff] [review]
> proposed patch
> 
> Dave or Blair should review the new classes. description-box vs.
> description-container, for instance, seems questionable to me.

I've tried to don't touch the existing classes, maintaining them as they are now. 
I'm not really creative to give names for classes and Ids, any suggestions would be very appreciate. ;-)

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 510078 [details] [diff] [review]
proposed patch

>+      <xul:hbox class="content-container">

Not sure about this name, but I can't think of a better one, so good enough :)


>+        <xul:vbox class="descriptions-container" flex="1">
>+          <xul:hbox class="name-box">
>             <xul:vbox flex="1">

This vbox (the one without a class) should have been removed in bug 623211. The name-container hbox would then only be used for convenience when styling (we currently set the font size and shadow using it, but that can be done other ways), so it should be able to be removed too. That should simplify this code quite a bit - though if you'd rather not do that in this bug, then that's ok.

I think "name-box" should be named something else, since it's not just the name (and there's already "name-container"). In the past it had been called "basicinfo-container" (can't remember why or when that was removed) - lets use that.

And let's rename "descriptions-container" to "content-inner-container", since it holds all the leftover content (which is most of it).


>-          <xul:hbox flex="1" align="end">
>-            <xul:vbox flex="1">
>-              <xul:hbox align="center" class="description-container">
>+          <xul:hbox flex="1" class="description-and-control-box">
>+            <xul:vbox class="description-box" flex="1">
>+              <xul:hbox class="description-container">

Nit: Swap the flex="" and class="" attributes. Having the class attribute first helps with code readability (most of the rest of this file does this).

Lets rename "description-and-control-box" to "advancedinfo-container", since it holds the all the stuff that's not basic information. And rename "description-box" to "description-outer-container" (having both XXX-box and XXX-container is confusing).


>+            <xul:vbox class="control-box">

There's already a "control-container" - lets rename this to "status-control-wrapper".


>   <!-- Addon - installing - an addon item that is currently being installed -->
...
>-      <xul:hbox align="stretch">
>-        <xul:vbox pack="start">
>-          <xul:vbox align="center" pack="center" class="icon-container">
>+      <xul:hbox class="content-container">
>+        <xul:vbox class="icon-box">
>+          <xul:vbox class="icon-container">

Again: I don't like having both icon-box and icon-container. Lets rename "icon-box" to "icon-outer-container".


>-        <xul:vbox flex="1" class="fade" align="stretch" pack="center">
>-          <xul:hbox class="name-container" align="end">
>-            <xul:label anonid="name" class="name"/>
>+        <xul:vbox flex="1" class="fade">

You can't re-purpose the fade class for this - see my comment further on.


>-        <xul:vbox align="end" pack="end">
>+        <xul:vbox class="install-box">

"install-status-container"


>diff --git a/toolkit/mozapps/extensions/content/extensions.css b/toolkit/mozapps/extensions/content/extensions.css

All these additions should be in the individual themes. And most of these classes already have existing rules in the individual themes - you should add -moz-box-align/-moz-box-pack to those instead of appending new ones to the end of the file.


>+/* Set "align" and "pack" attributes through CSS */

No need to have this comment - its self-explanitory.

>+.warning,
>+.error,
>+.pending,
>+.icon-container,
>+.description-container,
>+.update-info-box {
>+  -moz-box-align: center;
>+}

.warning, .error, and .pending already have a rule (see line 516 of winstripe), as does .icon-container (line 462 of winstripe), .description-container (line 506 of winstripe).

>+.icon-container,
>+.fade {
>+  -moz-box-pack: center;
>+}

.fade is used on various different elements, to alter the text color for disabled addons (I think only the linux theme uses it these days). You can't re-purpose it for this, as any element could have class="fade".


>\ No newline at end of file

Nit: When appending things to the end of a file, amke sure to leave an additional newline at the end.

Comment 6

[Pardal Freudenthal (:ShareBird)]

(In reply to comment #5)

Thank you Blair for taking the time for this bug.

> Comment on attachment 510078 [details] [diff] [review]
> proposed patch
> 
> >+      <xul:hbox class="content-container">
> 
> Not sure about this name, but I can't think of a better one, so good enough :)
> 
> 
> >+        <xul:vbox class="descriptions-container" flex="1">
> >+          <xul:hbox class="name-box">
> >             <xul:vbox flex="1">
> 
> This vbox (the one without a class) should have been removed in bug 623211. The
> name-container hbox would then only be used for convenience when styling (we
> currently set the font size and shadow using it, but that can be done other
> ways), so it should be able to be removed too. That should simplify this code
> quite a bit - though if you'd rather not do that in this bug, then that's ok.
> 

OK. I will remove the vbox maintaining the content as it is now. Right?

> Nit: Swap the flex="" and class="" attributes. Having the class attribute first
> helps with code readability (most of the rest of this file does this).
> 

I will also fix this for other boxes.

> .fade is used on various different elements, to alter the text color for
> disabled addons (I think only the linux theme uses it these days). You can't
> re-purpose it for this, as any element could have class="fade".

I will add an additional class="name-outer-container" for it. Is it OK?

> >diff --git a/toolkit/mozapps/extensions/content/extensions.css b/toolkit/mozapps/extensions/content/extensions.css
> 
> All these additions should be in the individual themes. And most of these
> classes already have existing rules in the individual themes - you should add
> -moz-box-align/-moz-box-pack to those instead of appending new ones to the end
> of the file.
> 
> 
> >+/* Set "align" and "pack" attributes through CSS */
> 
> No need to have this comment - its self-explanitory.
> 
> >+.warning,
> >+.error,
> >+.pending,
> >+.icon-container,
> >+.description-container,
> >+.update-info-box {
> >+  -moz-box-align: center;
> >+}
> 
> .warning, .error, and .pending already have a rule (see line 516 of winstripe),
> as does .icon-container (line 462 of winstripe), .description-container (line
> 506 of winstripe).
> 
> >+.icon-container,
> >+.fade {
> >+  -moz-box-pack: center;
> >+}

Some idea that occurred for me now... We could maybe create a couple new classes for the non-default values from "pack" and "align"; something like "center-pack" (or "pack-center"), "end-pack", "start-align", etc. and set their values globally (xul.css?). This would maybe simplify things on the future when writing xul elements by instead of writing pack="center", writing, e.g., class="myClass pack-center". What do you think? (It's just an idea...)

Comment 7

[Pardal Freudenthal (:ShareBird)]

Attachment: patch_v2

Patch following the directions from comment #5.

I've added a new line before the comment
/*** global warnings ***/
because other comments like that are preceded by two lines...

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 511615 [details] [diff] [review]
patch_v2

(Tagging myself for this review, so it doesn't get lost - will get to it when I can.)


(In reply to comment #6)
> Some idea that occurred for me now... We could maybe create a couple new
> classes for the non-default values from "pack" and "align"; something like
> "center-pack" (or "pack-center"), "end-pack", "start-align", etc. and set their
> values globally (xul.css?). This would maybe simplify things on the future when
> writing xul elements by instead of writing pack="center", writing, e.g.,
> class="myClass pack-center". What do you think? (It's just an idea...)

The trouble with that is that the class loses meaning once a theme changes it. eg, if there's 5 elements with class="blue-box", it's gonna be confusing when a theme then makes it green, or tries to make one of them red. So I think its best to stay away from classes that describe what the CSS does, and instead have the class describe what the element is.

Comment 9

[Pardal Freudenthal (:ShareBird)]

(In reply to comment #8)
> Comment on attachment 511615 [details] [diff] [review]
> patch_v2
> 
> (Tagging myself for this review, so it doesn't get lost - will get to it when I
> can.)

Thank you!

> The trouble with that is that the class loses meaning once a theme changes it.
> eg, if there's 5 elements with class="blue-box", it's gonna be confusing when a
> theme then makes it green, or tries to make one of them red. So I think its
> best to stay away from classes that describe what the CSS does, and instead
> have the class describe what the element is.

You're absolutely right! I didn't think about it...

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 511615 [details] [diff] [review]
patch_v2

Looks great - thanks!

Btw: Sorry it took me so long to get to this :(

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v2.01

Original patch had DOS line-endings - fixed it, so its easier to import/commit.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 513024 [details] [diff] [review]
Patch v2.01

Small risk - its just refactoring some CSS (and adding additional classes). No functional change. But it will make themer's lives much easier.

Comment 13

[Pardal Freudenthal (:ShareBird)]

Thank you very much Blair.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 513024 [details] [diff] [review]
Patch v2.01

a=beltzner with trepidation - we're sure this won't bust existing themers work already? Blair: you're on the hook for watching how this plays out.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #14)
> we're sure this won't bust existing themers work already?

It's not removing any classes, but it is moving the align="" and pack="" attributes into classes. Themers will need to add the respective CSS properties to the classes of those elements - until they do that, some of the layout probably won't look as good. It shouldn't break anything through (everything will still work). 

It's roughly on par (impact-wise) with some of other CSS changes we're still making in other bugs. But if its considered too risky, the changes could be moved out of the individual themes' CSS and into content CSS (so all themes get defaults they can override), but I want to avoid that if possible (code-wise, it doesn't make sense).


> Blair: you're on the hook for watching how this plays out.

Yep.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Ok, good to go after getting confirmation on IRC.

(Now the tree just needs to be receptive.)

Comment 17

[(no longer active)]

Landed <http://hg.mozilla.org/mozilla-central/rev/f4d707ab6c43> and then backed out <http://hg.mozilla.org/mozilla-central/rev/542c70cfaf47> because I needed to back out bug 625465 which conflicted with this one.

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Causality of war. Same patch needs relanded.

Comment 19

[Pardal Freudenthal (:ShareBird)]

Any chance to have this landed for beta12?

Comment 20

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/7b6be2af0613

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #19)
> Any chance to have this landed for beta12?

No, sorry - beta12 was already tagged. It'll be in tonight's nightly.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Marking as verified fixed based on the checkin and no obvious breakage except the new regression filed yesterday.