495214 - Refresh tab strip arrows on Windows

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

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

Theme bug for refreshed tab strip arrows on XP and Vista.

Comment 1

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

Attachment: tab-arrow-end-aero.png

Comment 2

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

Attachment: tab-arrow-start-aero.png

Comment 3

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

Attachment: tab-arrow-end.png

Comment 4

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

Attachment: tab-arrow-start.png

Comment 5

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

Image size had to be increased to 15x19 for each arrow to accommodate the glow on the hit state.  Order is:

normal / hover / disabled / hit

in all images.

Comment 6

[Dão Gottwald [:dao]]

The hit state in tab-arrow-end-aero.png is significantly darker than in tab-arrow-end.png, whereas tab-arrow-start.png and tab-arrow-start-aero.png are almost identical.

Comment 7

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

Attachment: tab-arrow-end.png with less fail

sorry, hit layer had opacity problem, fixed

Comment 8

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

the -aero are both a darker blue, and the xp icons use a brighter blue, they should all match their counterparts now.

Comment 9

[(no longer active)]

Attachment: Patch (v1)

Comment 10

[(no longer active)]

Attachment: Patch (v2)

Compensate the height difference of the new images to make sure the tab strip doesn't get taller with this patch (the toolbar buttons' heights remain 23px).

Comment 11

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

(this has my pre-approval to land on mozilla-central once reviewed, please nominate for a191 when that happens)

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #10)
> Created an attachment (id=380154) [details]
> Patch (v2)
> 
> Compensate the height difference of the new images to make sure the tab strip
> doesn't get taller with this patch (the toolbar buttons' heights remain 23px).

So that patch cuts off the top and bottom lines of the images.

Alex, can we get images with a height of 17px instead?

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 380154 [details] [diff] [review]
Patch (v2)

> .tabbrowser-arrowscrollbox > .scrollbutton-down > .toolbarbutton-icon {
>-  margin-top: 5px;
>-  margin-bottom: 1px;
>   -moz-margin-end: 2px;
> }

Looks like you need to remove the remaining margin too; makes the button overly wide.

Comment 14

[(no longer active)]

(In reply to comment #12)
> (In reply to comment #10)
> > Created an attachment (id=380154) [details] [details]
> > Patch (v2)
> > 
> > Compensate the height difference of the new images to make sure the tab strip
> > doesn't get taller with this patch (the toolbar buttons' heights remain 23px).
> 
> So that patch cuts off the top and bottom lines of the images.
> 
> Alex, can we get images with a height of 17px instead?

Yes, FWIW the top and bottom lines were empty pixels.

Comment 15

[(no longer active)]

Attachment: Patch (v2.1)

Addressed comment 13 for now...

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 380452 [details] [diff] [review]
Patch (v2.1)

needs fixed image files

Comment 17

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

Attachment: Tab strip arrows, all 17 pixels tall

Comment 18

[Dão Gottwald [:dao]]

Ehsan, while you're at it, care to rename the files from arrow-start and arrow-end to arrow-left and arrow-right?

Comment 19

[(no longer active)]

Attachment: Patch (v3)

Patch with new icons.

(In reply to comment #18)
> Ehsan, while you're at it, care to rename the files from arrow-start and
> arrow-end to arrow-left and arrow-right?

Done.

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 380605 [details] [diff] [review]
Patch (v3)

>+.tabbrowser-arrowscrollbox > .scrollbutton-up:not([disabled="true"]),
>+.tabbrowser-arrowscrollbox > .scrollbutton-down:not([disabled="true"]) {
>+  -moz-image-region: rect(0, 15px, 17px, 0);
>+}

:not([disabled="true"]) is not needed here, AFAICT.

Otherwise looks good, code-wise. I haven't tested this again, I hope you have!

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 380605 [details] [diff] [review]
Patch (v3)

>+        skin/classic/browser/tabbrowser/tab-arrow-right.png                     (tabbrowser/tab-arrow-right.png)
>+        skin/classic/browser/tabbrowser/tab-arrow-left.png                      (tabbrowser/tab-arrow-left.png)

nit: "left" precedes "right" alphabetically.

Comment 22

[(no longer active)]

(In reply to comment #20)
> (From update of attachment 380605 [details] [diff] [review])
> >+.tabbrowser-arrowscrollbox > .scrollbutton-up:not([disabled="true"]),
> >+.tabbrowser-arrowscrollbox > .scrollbutton-down:not([disabled="true"]) {
> >+  -moz-image-region: rect(0, 15px, 17px, 0);
> >+}
> 
> :not([disabled="true"]) is not needed here, AFAICT.

Yes, I was just imitating the :hover rules below it.  Will fix upon landing.

> Otherwise looks good, code-wise. I haven't tested this again, I hope you have!

Yes, I have!  :-)

(In reply to comment #21)
> (From update of attachment 380605 [details] [diff] [review])
> >+        skin/classic/browser/tabbrowser/tab-arrow-right.png                     (tabbrowser/tab-arrow-right.png)
> >+        skin/classic/browser/tabbrowser/tab-arrow-left.png                      (tabbrowser/tab-arrow-left.png)
> 
> nit: "left" precedes "right" alphabetically.

Will fix upon landing.

BTW, I assume that we don't want to rename the existing images on branch, right?

Comment 23

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/543dfafa466a

Comment 24

[(no longer active)]

hg did some crazy stuff because the binary image files were both changed and renamed, which caused the change not to take affect.  Pushed icons in a separate round: <http://hg.mozilla.org/mozilla-central/rev/625ef204187e>

Comment 25

[(no longer active)]

Attachment: 1.9.1 Patch

Branch patch without renaming the images.

Comment 26

[Dão Gottwald [:dao]]

(In reply to comment #22)
> BTW, I assume that we don't want to rename the existing images on branch,
> right?

If someone is using them, we're breaking that anyway, as the dimensions change.

I missed that you removed opacity:0.4 from the disabled state. Why have you done that?

Comment 27

[(no longer active)]

(In reply to comment #26)
> (In reply to comment #22)
> > BTW, I assume that we don't want to rename the existing images on branch,
> > right?
> 
> If someone is using them, we're breaking that anyway, as the dimensions change.

OK, I'll provide a new 1.9.1 patch then.

> I missed that you removed opacity:0.4 from the disabled state. Why have you
> done that?

Because previously we turned down the opacity in disabled mode using the normal image.  Now, we're using a dedicated disabled state, so I assumed that it's no longer needed.  Am I wrong?

Comment 28

[Dão Gottwald [:dao]]

Yes. We used to fade the button texture, too, and we want to continue to do so. So we should just keep doing what the old code did. I'll attach new images with the disabled state removed.

Comment 29

[(no longer active)]

(In reply to comment #28)
> Yes. We used to fade the button texture, too, and we want to continue to do so.
> So we should just keep doing what the old code did. I'll attach new images with
> the disabled state removed.

We can do so with the same images, just ignore the disabled state.  IINM this was the exact thing that was being done in the past.  Do you want me to submit a new patch to restore the opacity rule and ditch -moz-image-region?

Comment 30

[Dão Gottwald [:dao]]

Attachment: Tab strip arrows, disabled state removed

I think it's better to actually remove the disabled state from the images, so that we don't regress this again the future.

Comment 31

[Dão Gottwald [:dao]]

(In reply to comment #24)
> hg did some crazy stuff because the binary image files were both changed and
> renamed, which caused the change not to take affect.

I don't think we care about the file history in this case. For 1.9.1 just remove the old images and add the new ones, fwiw.

Comment 32

[(no longer active)]

Attachment: Followup patch

Followup patch to use the three-state icons.

I think this looks much better now, FWIW.  Thanks for catching my mistake, Dao!

Comment 33

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/59c3caaaddeb

Comment 34

[(no longer active)]

Attachment: 1.9.1 Patch

Comment 35

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

Comment on attachment 380623 [details] [diff] [review]
1.9.1 Patch

a191=beltzner

Comment 36

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4df4434bc10a

Comment 37

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090601 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090601041706

and

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1pre) Gecko/20090601 Shiretoko/3.5pre (.NET CLR 3.5.30729) ID:20090601044045

and checked on Windows XP builds:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090601 Shiretoko/3.5pre ID:20090601044045

and 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090601 Minefield/3.6a1pre ID:20090601041706