187096 - Go button should not be container

Status: RESOLVED WONTFIX

(Firefox :: Toolbars and Customization, defect)

Description

[alanjstr]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3a) Gecko/20021227 Phoenix/0.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3a) Gecko/20021227 Phoenix/0.5

This will probably get marked as Won't Fix, but I'm going to try anyways.

The XUL for the Go button is different than most of the other toolbar buttons. 
Instead of a button, it is a container.
      <toolbaritem id="go-container" align="center" class="chromeclass-location">
        <button id="go-button" class="button-toolbar"
                label="&goButton.label;"
                oncommand="addToUrlbarHistory(); BrowserLoadURL();"
                ondragover="nsDragAndDrop.dragOver(event, goButtonObserver);"
                ondragdrop="nsDragAndDrop.drop(event, goButtonObserver);"
                ondragexit="nsDragAndDrop.dragExit(event, goButtonObserver);"
                tooltiptext="&goButton.tooltip;"/> 
      </toolbaritem>

This makes theming the Go button much harder, especially when it comes to Text
Only mode.  

I suggest making it have the class toolbarbutton-1, just like Back, Forward,
Print, etc.

      <toolbarbutton id="print-button" class="toolbarbutton-1"
                      label="&printButton.label;"
                      oncommand="BrowserPrint();"
                      tooltiptext="&printButton.tooltip;"/>


Reproducible: Always

Steps to Reproduce:

Comment 1

[David Tenser [:djst]]

Confirming.

Comment 2

[Aaron Peterson]

I'm not verifying this, but I love the drop down arrow on the buttons, that give
extra choices... and I'm adding myself as a cc to see if this has anything to do
with the behavior of the go button.

Comment 3

[David Tenser [:djst]]

Is this bug related to the fact that if you right-click on the Go button (which
will bring up the "Customize..." menu item), and then click outside the menu,
the next time you hover the mouse over the button it will be get a different
hover state?

Comment 4

[Walter K.]

This results in default theme not having text-only version of Go button.

Comment 5

[alanjstr]

It doesn't prohibit it, but it makes it much harder to code.

Comment 6

[Walter K.]

I do not argue about complexity of coding text-only version of the Go button. I
was referring to a current bug which inhibits text-only version of the Go button
using default skin. In my mind, there is no need to open a new bug for the issue
because fixing this bug will make the issue go away.

Tested on "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b)
Gecko/20030504 Mozilla Firebird/0.6"

Also, summary of this bug should be expanded to include "- prevents text-only Go
button mode".

Comment 7

[alanjstr]

We were able to create a text-only mode for Pinball, but trying to get the text
aligned properly for Go has been elusive. 

Comment 8

[Simon Paquet [:sipaq]]

Taking QA Contact

Comment 9

[Mike Connor [:mconnor]]

*** Bug 225088 has been marked as a duplicate of this bug. ***

Comment 10

[Ben]

The toolbar can be customized such that the buttons (back, forward, etc) only
show the icons without the text. This works for all buttons, except the Go
button that keeps showing "Go" to the right of the button.

Comment 11

[alanjstr]

That's a theming problem.  Pinball does it right.

Comment 12

[Simon Paquet [:sipaq]]

*** Bug 225088 has been marked as a duplicate of this bug. ***

Comment 13

[alanjstr]

Attachment: browser.xul patch by cdn

Comment 14

[alanjstr]

Comment on attachment 148041 [details] [diff] [review]
browser.xul patch by cdn

class="button-toolbar" should probably be class="toolbarbutton-1" right?

Comment 15

[Chris Neale]

(In reply to comment #14)
> (From update of attachment 148041 [details] [diff] [review])
> class="button-toolbar" should probably be class="toolbarbutton-1" right?
> 

That would place 'Go' below the button

Comment 16

[Mike Connor [:mconnor]]

Comment on attachment 148041 [details] [diff] [review]
browser.xul patch by cdn

try this, without CSS changes the button doesn't even show up.	The problem
with this bug is that it really is an implementation detail.  Plus, please
provide patches as diffs, not entire files :)

Looking at this bug, I don't know that fixing this is necessarily better than
letting themers decide how they want to present this.

Comment 17

[alanjstr]

Damn, I attached the wrong file.  Stupid yahoo and its attachments.  

As for Go being below the container ... that's how all the other buttons are

The reason for "fixing" this is that trying to theme the button is a major pain.
 You wind up theming several containers instead of a simple button.

cdn, can you please either attach the patch or email me a zip of the patch?

Comment 18

[Chris Neale]

Attachment: patch changing go-button into toolbarbutton with class="toolbarbutton-1"

Comment 19

[Mike Connor [:mconnor]]

Comment on attachment 148045 [details] [diff] [review]
patch changing go-button into toolbarbutton with class="toolbarbutton-1"

same problem still exists that if you change this by itself the toolbar button
disappears.  I'm not going to review the change without the css changes
required to make Qute behave the same as it always has (and for that matter,
Pinstripe, assuming this causes problems there).

Making the go button into just another button is not a solution that we're
going to accept, simply because we want this text label for the button.  If
that makes it more difficult for theme authors, so be it.

Comment 20

[Chris Neale]

(In reply to comment #19)
> (From update of attachment 148045 [details] [diff] [review])
> same problem still exists that if you change this by itself the toolbar button
> disappears.  I'm not going to review the change without the css changes
> required to make Qute behave the same as it always has (and for that matter,
> Pinstripe, assuming this causes problems there).
> 

It only disappears because the entry for "go-button" isn't in localstore.rdf,
since the id has changed from the original "go-container" - it's not an absolute
disappearance, just a disappearance from the toolbar.

I think the whole point of the change is that it doesn't perform as it always
has, alanjstr correct me if I'm wrong.

Comment 21

[alanjstr]

So the go button is back on the palette?

Comment 22

[Chris Neale]

(In reply to comment #21)
> So the go button is back on the palette?

That's what happened to my trunk cvs build, yes.

Comment 23

[alanjstr]

(In reply to comment #22)
> (In reply to comment #21)
> > So the go button is back on the palette?
> 
> That's what happened to my trunk cvs build, yes.

mconnor-
Can you live with that?

cdn-
So this requires no CSS changes for Qute?  Should Qute be cleaned up anyways?

Comment 24

[Chris Neale]

Just adjusted gtk2/xft Firefox 0.8 (since Qute awol on cvs trunk - not that I
care, I prefer Pinball for Fx).  Since go-button ( a placeable toolbarbutton,
rather than a placeable toolbaritem ) now has a class of 'toolbarbutton-1' for
Qute etc it no longer has a text label in 'Icons' mode [which would fix bug
225088   ], for 'Icons and Text' the label is still to the right (as I believe
it was before)

Comment 25

[alanjstr]

Comment on attachment 148045 [details] [diff] [review]
patch changing go-button into toolbarbutton with class="toolbarbutton-1"

Now that cdn has explained...

Comment 26

[David Tenser [:djst]]

Cc'ing Arvid since this involves Qute.

Comment 27

[Mike Connor [:mconnor]]

the entire point of having it as a container was to allow it to have a text
label even when text labels were turned off (ideally, we could have selective
text labels, but that's much more involved).  I don't think that's a bad thing,
and if we're going to change something deliberately implemented, I want a way of
forcing the text label on if desired.  As it currently stands, this can be
worked around by theme authors, as it is in Pinball.  If we change this
behaviour, we need a way of ensuring that we don't force a visual change for
every theme that uses the current behaviour.

considering that this is, effectively, a bug to make a certain type of theming
easier, we should absolutely not be making a change that will affect all users
who have the go button on their toolbars.  (This change would potentially affect
hundreds of thousands of users, all to make it easier for theme authors to
achieve a certain look.  That seems like a net loss to me)

Comment 28

[alanjstr]

Having a text label in all modes has absolutely nothing to do with XUL and
everything to do with CSS.  If your complaint is that you want text in all
modes, I'm sure cdn can fix that.

Comment 29

[Aaron Peterson]

Here's the problem as i see it: 
 
We want easy themeing 
We want multiple language capability 
Go is a short word, and is almost like a picture itself. 
 
we have a large amount of themes already made that deal with this. 
***** 
Possible solutions: 
1. just keep on making funkey themes 
 
2. make a script or tool that makes a "standard" theme work with this text 
container 
 
3. change the browser, which would break existing themes, unless we write a 
wrapper to deal with broken themes 
 
3.b the extra behavior of the go button is desireable, extra behaviors are 
really cool.. 
 
3.c ... we could make all of the buttons like the go button.. and then make it 
possible to individually select button lables  (I think this is cool) 
 
3.d If we make the go button like all of the other buttons, here is how we can 
do it: 
 
3.d.1  have a script that creates the go button, (text+image) whenever the 
local changes or such... or when the user turns it off.    There is no reason 
to have the go button not be the word "Go" itself in fancy text... except for 
alternate languages. 
 
 

Comment 30

[David P James]

Actually, this change won't break existing themes - I did nothing to Pinball 
and it worked just fine with the change (but it would allow me to pull out a 
whole whack of pointless, needless code in Pinball). 
 
As to the issue at hand, why does the Go button need text at all? What's so 
special about it that it needs to be treated differently than all the rest? 
The only thing I can think of is that IE does it and that doesn't convince me 
one iota. Konqueror doesn't use a Go label and there's no good reason why we 
should either. Heck, Netscape 4 didn't even have a Go button! If a user needs 
a text label for Go then (1) the design of the Go icon should be revisited or 
(2) they're probably using text labels on all icons. If (2) is the case then 
this change needs to go in since the Go button is inconsistent with the rest 
in both text+icons and text-only - if you ask me, it looks unprofessional 
since the alignment is off. Sure, we could fix this in the default theme with 
some excess CSS, but fixing it in the XUL is quicker and cleaner. We will 
probably need a large sized version of the Go button icon along with a 
simplification of the go-button CSS to match that of the rest of the buttons 
ought to fix the remaining alignment issues (so only the back, forward and 
mail buttons will still be messed up) 
 
If someone can come up with a good reason why Go is fundamentally different 
than all the other icons and needs special treatment that doesn't rely on "We 
want to look like IE" then let's hear it. Also, let's not forget that there is 
a rather long and descriptive tooltip for the Go button as well... 

Comment 31

[alanjstr]

> We want easy themeing 

This helps with that

> We want multiple language capability 

That has absolutely nothing to do with this

> Go is a short word, and is almost like a picture itself. 

That's a theming issue.

> we have a large amount of themes already made that deal with this. 

How many of those are broken when it comes to the Go button?  

> Possible solutions: 
> 1. just keep on making funkey themes 

This would make theming easier

> 2. make a script or tool that makes a "standard" theme work with this text 
> container 

Most themes will probably still work.  It really depends on how they were created.

> 3.b the extra behavior of the go button is desireable, extra behaviors are 
> really cool.. 

Where did "extra behavior" come from?

> 3.c ... we could make all of the buttons like the go button.. and then make it 
> possible to individually select button lables  (I think this is cool) 

That has absolutely nothing to do with this.  If you want certain labels off,
use userChrome.css

This bug has nothing to do with localization or image choice.  I filed it in
December 2002, way back when this would have had a much smaller impact.  There
is still enough time between now and 0.9 to get this in and have themes change
if they really need to.   

A potential impact that hasn't been mentioned are extensions that might overlay
the go button.  

Comment 32

[Mike Connor [:mconnor]]

wrt the "without relying on we want to look like IE" comment.  This was done 
because of that convention (this isn't the place to argue those merits).  We 
don't have selective text labels implemented, so this was done deliberately.

Again, the problem with this bug is that it will result in a UI change.  We 
want the Go label exposed in icons-only mode.  (Text-only is a bug IMO).  If 
you as a theme author want to work around this, fine, but don't come to me with 
a patch that breaks the desired behaviour to make third-party themes easier to 
make, without at least providing a means to preserve the desired behaviour in 
the default themes.

Comment 33

[Chris Neale]

(In reply to comment #32)
> Again, the problem with this bug is that it will result in a UI change.  We 
> want the Go label exposed in icons-only mode.  (Text-only is a bug IMO).  If 
> you as a theme author want to work around this, fine, but don't come to me with 
> a patch that breaks the desired behaviour to make third-party themes easier to 
> make, without at least providing a means to preserve the desired behaviour in 
> the default themes.

The only thing that determines whether the label displays at any given time, in
any given mode is the class of 'toolbarbutton-1'.  Extra xul is extraneous.

The initial patch (which was never attached here) left the toolbaritem class
[button-toolbar] on the toolbarbutton, but the result was as now; the
toolbarbutton had text in 'icons only' mode, and in 'icons and text' mode, it
was to the right, not underneath.

Comment 34

[David P James]

Attachment: Corresponding CSS changes to retain 'pretend to be IE' Go button

I still fail to see why the Go button is treated differently; pretending to be
IE is insufficient grounds in my book but then pretending to be IE seems to be
a determinant of behaviour for some reason I cannot fathom. Whatever. <sigh>
(might as well invalidate/wontfix bug 225088 then).

Anyway, what this patch does is retain the current IE-like Go button text to
the right of Go button in icons-only mode. In text+icons mode the text drops
below the icon like all the rest so it doesn't look dumb and unprofessional.
And in text mode it fixes the problem since that is what this bug was all about
and the raison-d'etre of switching away from a container. I've also moved the
Go-button code to the end of the rest of the toolbarbutton code to make theming
easier.

A small caveat - you have to close the palette for changes to actually take -
you can't rely on what you see on the toolbars when the palette is open and
conclude something. The palette must be closed. I'm not sure why this is.

Pinball shows no deleterious effects from this change; haven't tested with
other themes.

What we will need is a bigger icon for large-icon mode, and therefore a bit
more CSS to go in after the small button code of the rest of the buttons.
Unless the current undersized Go icon is fine since that's pretty much what we
have right now... but a large Go button would just make the entire theme fit
together better (imho, we need it anyway, regardless of this bug).

Comment 35

[miahz]

Somewhere along the way the Go Button was changed to a proper <toolbarbutton>. 
Which is a big improvement and does simplify theming a bit.  Though it still has
the container and is not toolbarbutton-1 class.

Comment 36

[alanjstr]

The only other item that is only chromeclass-toolbar-additional is the search
box.    Theo others are also toolbarbutton-1.  What's the diff?

Comment 37

[Mike Connor [:mconnor]]

Not going to fix this in this form.  Some reworking of how toolbars are done, in general, will be necessary to fix this, but until that happens we're not going to churn the way this is implemented