Closed Bug 631827 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0
Tracking Status
blocking2.0 --- -

People

(Reporter: ShareBird, Assigned: ShareBird)

References

Details

(Whiteboard: [has patch])

Attachments

(1 file, 2 obsolete files)

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?
Attached patch proposed patch (obsolete) — Splinter Review
Assignee: nobody → pardal
Status: NEW → ASSIGNED
Blocks: 623250
blocking2.0: --- → ?
This will not block the release
blocking2.0: ? → -
Attachment #510078 - Flags: review?(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.
Attachment #510078 - Flags: review?(dao) → review?(dtownsend)
(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. ;-)
Attachment #510078 - Flags: review?(dtownsend) → review?(bmcbride)
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.
Attachment #510078 - Flags: review?(bmcbride) → review-
OS: Windows XP → All
Hardware: x86 → All
(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...)
Attached patch patch_v2 (obsolete) — Splinter Review
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...
Attachment #510078 - Attachment is obsolete: true
Attachment #511615 - Flags: review?
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.
Attachment #511615 - Flags: review? → review?(bmcbride)
(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 on attachment 511615 [details] [diff] [review]
patch_v2

Looks great - thanks!

Btw: Sorry it took me so long to get to this :(
Attachment #511615 - Flags: review?(bmcbride) → review+
Attached patch Patch v2.01Splinter Review
Original patch had DOS line-endings - fixed it, so its easier to import/commit.
Attachment #511615 - Attachment is obsolete: true
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.
Attachment #513024 - Flags: approval2.0?
Thank you very much Blair.
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.
Attachment #513024 - Flags: approval2.0? → approval2.0+
(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.
Ok, good to go after getting confirmation on IRC.

(Now the tree just needs to be receptive.)
Keywords: checkin-needed
Whiteboard: [has patch][needs landing]
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.
Keywords: checkin-needed
Whiteboard: [has patch][needs landing] → [has patch]
Causality of war. Same patch needs relanded.
Keywords: checkin-needed
Whiteboard: [has patch] → [has patch][needs landing]
Any chance to have this landed for beta12?
http://hg.mozilla.org/mozilla-central/rev/7b6be2af0613
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][needs landing] → [has patch]
Target Milestone: --- → mozilla2.0
(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.
Flags: in-testsuite-
Flags: in-litmus-
Marking as verified fixed based on the checkin and no obvious breakage except the new regression filed yesterday.
Status: RESOLVED → VERIFIED
Duplicate of this bug: 593754
You need to log in before you can comment on or make changes to this bug.