431057 - RTL icon needed for the new tree twisty icons for the Aero theme

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[(no longer active)]

We need RTL versions of the new tree twisty icons, otherwise RTL trees look ridiculous.  Alex said he's going to provide the new icons.

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: New set of tree icons

Here are all if the tree icons, including RTL states for the 4 aero icons.

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

I'm requesting blocking since having expansion arrows pointing in the wrong direction is pretty obviously broken.

Comment 3

[Dão Gottwald [:dao]]

How are we going to use the icons in CSS? chromedir isn't globally available.

Comment 4

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

(In reply to comment #3)
> How are we going to use the icons in CSS? chromedir isn't globally available.

You mean unavailable from /toolkit? That's, uh, hrm. Can we override that by adding our own tree.css in /browser? I guess that would require moving the images into /browser as well and restoring the old non-RTL ones here.

Comment 5

[Kai Liu]

(In reply to comment #4)
> (In reply to comment #3)
> > How are we going to use the icons in CSS? chromedir isn't globally available.
> 
> You mean unavailable from /toolkit? That's, uh, hrm. Can we override that by
> adding our own tree.css in /browser? I guess that would require moving the
> images into /browser as well and restoring the old non-RTL ones here.
> 

I think moving that out of toolkit would be a good idea, especially if the suggestion in bug 430852 comment 25 is ever implemented...

Comment 6

[:Gavin Sharp [email: [e-mail]]]

global.dtd is in dom/locales, so we can use it from toolkit without a problem. You can set chromedir on trees by setting it on the binding's <content>, too.

Comment 7

[(no longer active)]

Thanks for the tip Gavin!  Patch coming up shortly.

Comment 8

[(no longer active)]

Actually, this bug seems Vista specific, unless we want to add the code for all of the platforms and use identical RTL images for the themes which use symmetric (RTL-agnostic) tree twisty icons.

Comment 9

[(no longer active)]

Attachment: Patch (v1)

This patch should land with the icons in attachment 318328 [details].

Comment 10

[Damon Sicore (:damons)]

Comment on attachment 318328 [details]
New set of tree icons

a1.9+=damons

Comment 11

[Alex Faaborg [:faaborg] (Firefox UX)]

We are changing the file names from aero-rtl to rtl-aero for consistency, sorry about getting it wrong in the earlier icon set.

Comment 12

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Updated tree widget images

Here is an updated set of images.  Note that the file names have been changed for rtl to appear before -aero instead of after.  If bug 431633 is resolved then these icons will already be checked in, otherwise you can check them in with the patch on this bug.

Comment 13

[(no longer active)]

Attachment: Patch (v1.1)

Here's the updated patch considering the file name change.

Comment 14

[(no longer active)]

Attachment: Patch (v1.2)

Updated patch to handle the hover RTL icons as well.  I also included the fix to bug 430895 in this patch (in tree-aero.css).

Comment 15

[(no longer active)]

Comment on attachment 318757 [details]
Updated tree widget images

Now that bug 431633 has landed, this should no longer be necessary here.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 318763 [details] [diff] [review]
Patch (v1.2)

>Index: toolkit/themes/winstripe/global/jar.mn

>+        skin/classic/aero/global/tree/twisty-clsd-rtl.png                (tree/twisty-clsd-rtl-aero.png)
>+        skin/classic/aero/global/tree/twisty-clsd-hover-rtl.png          (tree/twisty-clsd-hover-rtl-aero.png)
>+        skin/classic/aero/global/tree/twisty-open-rtl.png                (tree/twisty-open-rtl-aero.png)
>+        skin/classic/aero/global/tree/twisty-open-hover-rtl.png          (tree/twisty-open-hover-rtl-aero.png)

Looks like these changes have already landed.

>Index: toolkit/themes/winstripe/global/tree-aero.css

>+  Please note that the following RTL icons are only available in Aero themes:

The hover icons are also only in aero themes, so this comment should probably be at the top of the file and apply to all images referred to in this file.

Comment 17

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

Comment on attachment 318763 [details] [diff] [review]
Patch (v1.2)

a1.9=beltzner

Comment 18

[(no longer active)]

Attachment: Patch (v1.3) (for check-in)

This patch addresses Gavin's review comments, so I'm carrying forward r+ and a+ on this patch.  This is the patch to land for this bug.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch I checked in

For some reason I couldn't apply that previous patch cleanly (I got: patch: **** malformed patch at line 119), so I applied it manually.

mozilla/toolkit/content/widgets/tree.xml 	1.57
mozilla/toolkit/themes/winstripe/global/jar.mn 	1.58
mozilla/toolkit/themes/winstripe/global/tree-aero.css 	1.1