495218 - Update all tabs button on Windows

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

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

A new drop down glyph for the windows tab strip that matches the arrows and new tab button.

Comment 1

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

Not entirely sure where we should be checking these in, perhaps /source/browser/themes/winstripe/browser/tabbrowser/

We will be using the same image on the search engine button as well.

Comment 2

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

Attachment: arrow-down.png

Comment 3

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

Attachment: arrow-down-aero.png

Comment 4

[(no longer active)]

The two images seem identical...

Comment 5

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

aero is a little darker blue on the hit state, normal state is identical

Comment 6

[(no longer active)]

Attachment: Patch (v1)

Comment 7

[Luke Iliffe (Harlequin99)]

If this lands on trunk, as opposed to 1.9.1 it will replace the icon on the 'List all tabs' button for displaying tab previews. Is this intentional & this bug is for Shiretoko only and another icon will be produced for 3.6?

Comment 8

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

I copies the wrong path in comment 1, meant to suggest that we drop these files into /source/toolkit/themes/winstripe/global/arrow/

Comment 9

[(no longer active)]

Attachment: Patch (v2)

Updated the location of arrow-down.png based on comment 8.

Comment 10

[u88484]

I don't think the all tabs button should be a down arrow.  We have a side ways arrow to scroll the tabstrip side to side and I feel users might think this icon would be to scroll the tab strip up and down.  Also, a down-wards pointing arrow really has no represetation of many tabs, more tabs, all tabs or even a tab at all.

Comment 11

[Dão Gottwald [:dao]]

I'm not convinced that toolkit/themes/winstripe/global/arrow/ is the right place for this, can we just put it in browser? Also, this needs a patch specifically for the 1.9.1 branch.

Comment 12

[(no longer active)]

(In reply to comment #11)
> I'm not convinced that toolkit/themes/winstripe/global/arrow/ is the right
> place for this, can we just put it in browser?

Alex said that this should go into toolkit.  Alex, did you have a specific reason?

> Also, this needs a patch
> specifically for the 1.9.1 branch.

How come?

Comment 13

[Dão Gottwald [:dao]]

The all-tabs button is completely different on branch.

Comment 14

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

I don't really care where it ends up, toolkit just seemed to make more sense since this was a general purpose arrow.

Comment 15

[(no longer active)]

So, what do you think Dão?  Please suggest a new place for the icon if you think toolkit is not an appropriate place for it.

Comment 16

[Dão Gottwald [:dao]]

browser/ or browser/tabbrowser/, I don't really care. What's more tricky is finding a meaningful file name... with "arrow-down.png", you need to search the whole theme to figure out where that icon is used and supposed to be used. That's probably an argument for putting it in tabbrowser/, as that's already a meaningful context.

Comment 17

[Dão Gottwald [:dao]]

Given bug 495606, I'd suggest something like browser/mainwindow-dropdown-arrow.png. I'm not good at making up file names, so feel free to propose something better.

Comment 18

[Dão Gottwald [:dao]]

Ehsan, can you prepare a 1.9.1 patch based on bug 495606?

Comment 19

[(no longer active)]

Attachment: Patch (v2.1)

With the new file name...

Comment 20

[(no longer active)]

Attachment: 1.9.1 Patch

Comment 21

[Dão Gottwald [:dao]]

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

USE_TAB_PREVIEWS is not defined. You need to touch the %else branch only.

Comment 22

[(no longer active)]

Attachment: 1.9.1 Patch

Comment 23

[Dão Gottwald [:dao]]

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

>+.tabs-alltabs-button:hover > .toolbarbutton-icon {
>+  -moz-image-region: rect(0, 26px, 11px, 13px);
> }

The images have a hit state, not a hover state.

Comment 24

[(no longer active)]

(In reply to comment #23)
> (From update of attachment 380714 [details] [diff] [review])
> >+.tabs-alltabs-button:hover > .toolbarbutton-icon {
> >+  -moz-image-region: rect(0, 26px, 11px, 13px);
> > }
> 
> The images have a hit state, not a hover state.

Hmm, so do you want me to change the trunk patch to hit state as well?

Comment 25

[(no longer active)]

Attachment: Patch (v2.2)

(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 380714 [details] [diff] [review] [details])
> > >+.tabs-alltabs-button:hover > .toolbarbutton-icon {
> > >+  -moz-image-region: rect(0, 26px, 11px, 13px);
> > > }
> > 
> > The images have a hit state, not a hover state.
> 
> Hmm, so do you want me to change the trunk patch to hit state as well?

Logically you would want that!  :-)

Comment 26

[(no longer active)]

Attachment: 1.9.1 Patch

Comment 27

[Dão Gottwald [:dao]]

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

This makes the tab bar 1px taller, I think.

>+.tabs-alltabs-button:hover:active > .toolbarbutton-icon {
>+  -moz-image-region: rect(0, 26px, 11px, 13px);
> }

:active doesn't work here, you want [open="true"].

Comment 28

[(no longer active)]

Attachment: 1.9.1 Patch

(In reply to comment #27)
> (From update of attachment 380720 [details] [diff] [review])
> This makes the tab bar 1px taller, I think.
> 
> >+.tabs-alltabs-button:hover:active > .toolbarbutton-icon {
> >+  -moz-image-region: rect(0, 26px, 11px, 13px);
> > }

I tested using DOM Inspector, and the tab bar remains at 30px height.

> :active doesn't work here, you want [open="true"].

Done.

Comment 29

[Dão Gottwald [:dao]]

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

(In reply to comment #28)
> I tested using DOM Inspector, and the tab bar remains at 30px height.

Not sure which element and which property you're looking at, but please test by comparing screenshots or something else reliable.

Comment 30

[(no longer active)]

Attachment: 1.9.1 Patch

(In reply to comment #29)
> (From update of attachment 380728 [details] [diff] [review])
> (In reply to comment #28)
> > I tested using DOM Inspector, and the tab bar remains at 30px height.
> 
> Not sure which element and which property you're looking at, but please test by
> comparing screenshots or something else reliable.

Weird, I was measuring the height of the alltabs toolbarbutton, but I guess from now on I won't trust DOM Inspector in this aspect...

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 380719 [details] [diff] [review]
Patch (v2.2)

makes the tab bar 1px taller

Comment 32

[Dão Gottwald [:dao]]

I'm also not sure that we should do this on trunk at all.

Comment 33

[(no longer active)]

(In reply to comment #32)
> I'm also not sure that we should do this on trunk at all.

Without landing it on trunk, we can't land it on 1.9.1 either, right?

Comment 34

[Mike Connor [:mconnor]]

Isn't this going to break compat with some themes?  We're adding a new image name, so themes using the old image location will break... unless I"m just reading the patch wrong.  I don't think we can take this at the last minute like this...

Comment 35

[Dão Gottwald [:dao]]

I don't see how. Themes ship all their images themselves.

Comment 36

[Dão Gottwald [:dao]]

... and their own stylesheets, too. They are independent of everything in browser/themes/.

On an unrelated note, sorry for messing up the dependency fields. Bug 495606 should land first, on both trunk and branch, and then the 1.9.1 patch in this bug can be reduced to the CSS changes (no image additions, no packaging changes).

Comment 37

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

Need to see before/after screenshots before I approve anything here.

Comment 38

[(no longer active)]

Attachment: Before/After screenshot

Comment 39

[(no longer active)]

Beltzner: posted a before/after screenshot as attachment 381007 [details].

Comment 40

[u88484]

(In reply to comment #10)
> I don't think the all tabs button should be a down arrow.  We have a side ways
> arrow to scroll the tabstrip side to side and I feel users might think this
> icon would be to scroll the tab strip up and down.  Also, a down-wards pointing
> arrow really has no represetation of many tabs, more tabs, all tabs or even a
> tab at all.

Anybody going to comment on this comment?  Why don't we just make every button image in the them an arrow and look disabled.  What is the reasoning behind this logic?

Comment 41

[Dão Gottwald [:dao]]

It's already a downwards-pointing arrow on branch. This bug isn't going to change this.

Comment 42

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

>Anybody going to comment on this comment?  Why don't we just make every button
>image in the them an arrow and look disabled.  What is the reasoning behind
>this logic?

sorry, didn't mean to ignore the comment, I'm just inundated with bugmail.  I'm fine with us coming up with a more descriptive symbol here (some browsers have had like 40 arrows in the UI), but it's something we would need to think about for the next release since we unfortunately no longer have time to create artwork or test.

Comment 43

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

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

a191=beltzner

Comment 44

[(no longer active)]

(In reply to comment #32)
> I'm also not sure that we should do this on trunk at all.

So, should I first land it on trunk, or directly on 1.9.1?

Comment 45

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

Since it has 1.9.1 approval I would assume both.

Comment 46

[(no longer active)]

(In reply to comment #45)
> Since it has 1.9.1 approval I would assume both.

Yes, but since Dão explicitly stated that he's not ok with taking this on trunk, I guess I'll wait for him to weigh in.

Comment 47

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

oh, right, forgot about the control-tab and all tabs stuff on trunk (which changes the icon), just 1.9.1 would make sense then.  We are probably going to freeze for RC pretty soon, so it would be good if we checked in soon so all of our tab strip icons are in the same visual style.

Comment 48

[(no longer active)]

Based on IRC conversation with Alex, landed only on 1.9.1: <http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0ac3ca7d67a8>.

I'm not sure what the correct convention here is, but I'm marking this as FIXED and fixed1.9.1 and also setting the TM to Firefox 3.5.

Comment 49

[(no longer active)]

The reason that this was not landed on trunk was that trunk uses a different all tabs icon, and we didn't want to overwrite that.

Comment 50

[Simon Bünzli]

Is it intentional that the arrow shifted one pixel to the right? It now feels as if 2px of the button were cut off... (in attachment 381007 [details] you'll notice that the original arrow was centered while the new one isn't).

Comment 51

[(no longer active)]

(In reply to comment #50)
> Is it intentional that the arrow shifted one pixel to the right? It now feels
> as if 2px of the button were cut off... (in attachment 381007 [details] you'll notice
> that the original arrow was centered while the new one isn't).

Alex, if that is not intentional, then I think the image needs to be updated because I simply cut it in half inside the code.

Comment 52

[Dão Gottwald [:dao]]

The image seems to be balanced, has three transparent pixels on each side.

Comment 53

[Dão Gottwald [:dao]]

The whole button should probably 17 instead of 18px wide...

Comment 54

[Aakash Desai [:aakashd]]

verified FIXED on builds:

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

and 

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