347754 - make the go button optional in the Firefox2 default theme

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[John Hanna]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060807 BonEcho/2.0b1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060807 BonEcho/2.0b1

The GO button cannot be removed as it is now attached to the URL bar..
In the previous nightly build it was attached to the Search Bar.
Before the new theme it was unattached and was optional.

Reproducible: Always

Actual Results:  
Attempting to remove the GO button removes the URL bar.

Expected Results:  
Should be unattached and optional as it was before the new theme landed

Comment 1

[Ria Klaassen (not reading all bugmail)]

Yeah, also in my theme they are Siamese twins now. Although there is a space between them like usually.

Comment 2

[Jonathan Haas]

The search button should also be removeable.

Comment 3

[Mike Connor [:mconnor]]

Why?  Other than the hyper-efficient toolbar configurations some people use, is there a win in adding additional complexity to the toolbarbuttons?  If you're in the hyper-efficiency corner case, you can just use userChrome.css

Comment 4

[Dão Gottwald [:dao]]

(In reply to comment #3)
> Why?  Other than the hyper-efficient toolbar configurations some people use, is
> there a win in adding additional complexity to the toolbarbuttons?  If you're
> in the hyper-efficiency corner case, you can just use userChrome.css

I think removing the Go button is one of the first and most common tasks that toolbar costomization is used for. I doubt that most of the users who've done this are familiar with userChrome.css.

Sure, it's an optimization, it's about about efficiency. But why "hyper"? If removing the Go button is not a valid task for the casual user, you can easily expand this argumentation to involve all toolbar items.

The next point is that most themes don't have a Go button that is styled as an endcap. And that's fine. I mean, it's styling. The new theme shouldn't dictate chrome limitations (we're not speaking about an enhancement here) where it's not absolutely necessary.

My proposal is, include two Go buttons in the new theme, e.g.:

/*pseudocode*/

#go-button {
  background-image: url(bo-button-standalone.png);
}
#address-bar + #go-button {
  background-image: url(go-button-endcap.png);
}

Comment 5

[Worcester12345]

(In reply to comment #4)
> (In reply to comment #3)
> > Why?  Other than the hyper-efficient toolbar configurations some people use, is
> > there a win in adding additional complexity to the toolbarbuttons?  If you're
> > in the hyper-efficiency corner case, you can just use userChrome.css
> 
> I think removing the Go button is one of the first and most common tasks that
> toolbar costomization is used for.

You mean ADDING it, don't you? I thought it came "stock" without the Go button. Looking for it was one of the first things I did. Maybe this was in Mozilla suite browser. Either way, your conjecture isn't 100% on.

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #5)
> You mean ADDING it, don't you?

No, I meant removing it. Firefox shows it by default.

> Either way, your conjecture isn't 100% on.

Why? I didn't conjecture that most or even all users remove it. In that case I would have argued for removing it by default. :)

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: adjacent selectors at work

Like dan said it should be pretty easy to create a style rule for the existing button to style it differently, see this attachment for a demo. The only thing that would be required is retaining the old icon, and an additional style rule.

Comment 8

[Tracy Walker [:tracy]]

*** This bug has been marked as a duplicate of 347402 ***

Comment 9

[Phil Ringnalda (:philor)]

Err, no, not a dupe of that.

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

FWIW this is not additional complexity, this is continuing to have the button conform to the framework within which it resides.

Comment 11

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: /browser/themes/winstripe/browser/Go.png

Placeholder image. Patch coming shortly.

Comment 12

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch v1

Current work in progress. 
This is currently off by one pixel in the active state when separated from the location bar and padding needs a little all around tweaking.

Comment 13

[Peter Kasting]

Thanks for taking Dao's/Ben's notes and turning them into a patch, Ryan!

As a cautionary note, have you considered how to verticallyscale this for the "url bar uses large fonts" case?  It looks like the code you're replacing has at least some hackish attempts at dealing with that, and I know that's a concern of various forum members.  I'd like to at least not make that harder/worse... I'm worried the only solution here might be "break all the images into top/side/center/bottom sections and composite as needed", which would be scary.

If you have tons of time and get all the issues on this one figured out, do you think you could do a similar patch for the Search button?

Comment 14

[Ryan Flint [:rflint] (ping via IRC for reviews)]

(In reply to comment #13)
> As a cautionary note, have you considered how to verticallyscale this for the
> "url bar uses large fonts" case?  It looks like the code you're replacing has
> at least some hackish attempts at dealing with that, and I know that's a
> concern of various forum members.  I'd like to at least not make that
> harder/worse... I'm worried the only solution here might be "break all the
> images into top/side/center/bottom sections and composite as needed", which
> would be scary.

I looked into scaling the button vertically a bit last night, but decided to replicate the current behavior simply for matters of expedience. The solution that looked most promising to me would unfortunately result in breaking the button into two separate pieces - a very large background image scaled to fit the current button size and a standalone arrow image. The only issue I see with going this route is dealing with the borders. A pure CSS border solution (a step towards resolving bug 347613) might be possible with an ugly hack or two, but I'm not entirely sure. 
I'll definitely run through a few different scaling options on the road towards fixing this.

> If you have tons of time and get all the issues on this one figured out, do you
> think you could do a similar patch for the Search button?
> 

As long as I don't go crazy trying to get things working here, that'll be my next stop. ;)

Comment 15

[Steven]

same or similar to 347714 which is the method of making go button optional drag and drop

Comment 16

[Dave Townsend [:mossop]]

(In reply to comment #15)
> same or similar to 347714 which is the method of making go button optional drag
> and drop
> 

No quite different. This is about allowing you to move the go button anywhere on the toolbars and even removing it from the toolbars. bug 347714 is about allowing you to drop links onto the go button and have it load them.

Comment 17

[Peter Kasting]

(In reply to comment #14)
> I looked into scaling the button vertically a bit last night, but decided to
> replicate the current behavior simply for matters of expedience. The solution
> that looked most promising to me would unfortunately result in breaking the
> button into two separate pieces - a very large background image scaled to fit
> the current button size and a standalone arrow image.

Hmm.  Since we only need to scale in one dimension, what about something more akin to what old table-based HTML pages used to do?

XXX
YYY
ZZZ
YYY'
XXX'

Here XXX and XXX' are the top and bottom of the button, containing the rounded corners, and ZZZ is the arrow section.  These can be made from pieces of the existing button graphic.  YYY and YYY' are thin strips of the sections of button between the rounded corners, and just repeatedly tile their background image as the button scales taller.  This way the rounded corners don't look weird as the buttons get bigger, because XXX and XXX' are never scaled.

Maybe this isn't doable in XUL, though, I don't know.

Comment 18

[Dão Gottwald [:dao]]

Attachment: image for prototype (bg.png)

Comment 19

[Dão Gottwald [:dao]]

Attachment: image for prototype (top.png)

Comment 20

[Dão Gottwald [:dao]]

Attachment: image for prototype (bottom.png)

Comment 21

[Dão Gottwald [:dao]]

Attachment: prototype: scale button vertically

This should be possible with XUL, right?

Comment 22

[Dão Gottwald [:dao]]

Attachment: image for prototype v2 (top.png)

Attachment 233663 [details] had a problem with transparent images (bg.png visible through top.png and bottom.png). Uploading updated files.

Comment 23

[Dão Gottwald [:dao]]

Attachment: image for prototype v2 (bottom.png)

Comment 24

[Dão Gottwald [:dao]]

Attachment: prototype v2: scale button vertically

Hope this helps.

Comment 25

[Dão Gottwald [:dao]]

(In reply to comment #24)
> Created an attachment (id=233677) [edit]

Ignore background-color:inherit. I just tried something and forgot to remove this. It's not needed at all.

That's my last comment for now. :)

Comment 26

[Mike Connor [:mconnor]]

Comment on attachment 232878 [details] [diff] [review]
Patch v1

doing this halfway (Go but not Search) is weird, and breaking the Search binding to facilitate this is going to be ugly, and add extra complexity for sure.

Going to push this until after b2 so we can figure out how much this really matters, and if/how we can do it cleanly and consistently.

Comment 27

[Pam Greene]

(In reply to comment #14)

> I looked into scaling the button vertically a bit last night, but decided to
> replicate the current behavior simply for matters of expedience.

I'll be fixing all three static buttons (Go, Search, and the search-engine selection button) to scale vertically; see bug 348138.  I won't be addressing this bug (making the button optional), though.

Comment 28

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Alright, thanks Pam :)
I'll hold off here until you're done with bug 348138 to avoid making conflicting changes.

(In reply to comment #27)
> 
> I'll be fixing all three static buttons (Go, Search, and the search-engine
> selection button) to scale vertically; see bug 348138.  I won't be addressing
> this bug (making the button optional), though.
> 

Comment 29

[Peter Kasting]

If, when this bug is fixed, the Go button can be placed somewhere on the toolbar other than at the end of the urlbar, bug 349150 should almost certainly be un-WONTFIXED.

Comment 30

[Reed Loden [:reed]]

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

Comment 31

[Uri Bernstein]

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

Comment 32

[Peter Kasting]

mconnor/beltzner have been discussing the idea of the Go button being an "attribute" of the urlbar (so you could toggle it on and off) rather than a standalone button.  This would let us make the Go button vertically scalable without preventing users from easily removing it.

I don't know if this plan is final or what the UI for toggling the button on and off would be (context menu on the urlbar?).  It might be nice to get some input from them here...

Comment 33

[Peter Kasting]

A suggestion from the Mozillazine forums:

"Another alternative coming to mind is to have two Go buttons - one inside the urlbar-container and another independent one - and use Flock's technique of applying different styling to the address bar when the Go button has been placed right behind it (using a rule for toolbar[currentset*="urlbar-container,go-button"]) to hide the independent one and show the embedded one (default is to hide the embedded one and only show the independent one). That'd solve both the customizability and the correct height issue."

In other words, something similar to what's already been suggested on this bug, except instead of using a different image for the button when next to the bar, you'd make it invisible and make the go button inside the bar (which uses the patch on bug 348138 to be scalable) visible.

Presumably in toolbar customization mode you always show the independent button and always hide the embedded one.

Comment 34

[Ria Klaassen (not reading all bugmail)]

Why not just untie them in browser.xul? Then you can drag the end-cap into the customize window. Maybe I under-estimate the problem (must be), but I had no problem bringing the end-cap over to a trunk theme (just a little change in the margin of the button): http://img457.imageshack.us/img457/1382/endcapec3.png

Comment 35

[Peter Kasting]

(In reply to comment #34)
> Why not just untie them in browser.xul?

The problems are:
(1) Go button looks silly if it uses the endcap images when moved elsewhere -- so the suggestion on this bug was to use two sets of images, one for "next to urlbar" styling, and one for "elsewhere".  (Two sets of padding would be needed, too.)
(2) Untying them means the Go button can't properly scale vertically for taller urlbars.  Hence the proposal to use two buttons -- one on the bar and one not.  This might help solve the issues with Go buttons having/not having text in "Text only/text + icons" mode as well, which is an issue with the proposal to do the Go button as an attribute of the urlbar.

Comment 36

[Radek 'sysKin' Czyz]

(In reply to comment #32)
> mconnor/beltzner have been discussing the idea of the Go button being an
> "attribute" of the urlbar (so you could toggle it on and off) rather than a
> standalone button.

Although a shameless ripoff from IE, this idea perfectly satisfies *my* humble need. I also see no reason why anyone else would want to move this button around.

(In reply to comment #35)
> (1) Go button looks silly if it uses the endcap images when moved elsewhere

Question, does any person actually saw this "endcap" without being told it's supposed to be an endcap?
For me, it just looks like a "play" button in a media player. This rings a good bell, and is an interesting innovation (so far, this symbol was restricted to players). I see no endcap though...

Comment 37

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #35)
> 
Are there really people who want the Go-button elsewhere? If they really exist they could use an extension that replaces the button by another image. Or a pref that replaces the image if it is more than 5% of the users who want the button elsewhere.
Hm, I didn't think about people who use larger URL bars. 
Why not just make a pref that toggles on and off a simple style rule that makes the button disappear? I suspect that half of the people never use the go-button and that justifies a place in the Options window. 

Comment 38

[Radek 'sysKin' Czyz]

(In reply to comment #37)
> and that justifies a place in the Options window. 
 
Or customize window, next to "use small icons".

Comment 39

[Bill Gianopoulos [:WG9s]]

(In reply to comment #37)
> (In reply to comment #35)
> > 
> Are there really people who want the Go-button elsewhere? 
Why yes.  There are those who want it immediately to the left of the url bar.

Comment 40

[Peter Kasting]

(In reply to comment #37)
> Why not just make a pref that toggles on and off a simple style rule that makes
> the button disappear? I suspect that half of the people never use the go-button
> and that justifies a place in the Options window. 

Because a patch that makes the Go button movable/removable using the normal mechanism of dragging it off the toolbars is easier to discover and understand than something that works specially for the Go button.  From a user perspective, why should I move other buttons using dragging but move this one by toggling a checkbox?  It makes no sense, and UI shouldn't have elements that make no sense.

I really like the suggestion in comment 33; I think it satisfies every circumstance very nicely.

Comment 41

[Mike Connor [:mconnor]]

The problem with the proposed solution is that we'd have to audit the hell out of any of the go button-related event listeners/handlers in browser.js, and implement some shim to use whichever one is actually visible.

Given how many problems we've had with go button click/drag/etc when we've touched it, I think that idea scares me.  Its really not that different from the "have two URL bars in the palette" concept that came up a lot time ago, except its more subtle.  I'm worried about extensions as well, since we'd have two potential go button targets, so we'd break anyone who was attaching to those.  It'd be fixable breakage, but this seems like a lot of risk for a 50% solution (since its only removing one of the endcaps)

Comment 42

[Peter Kasting]

(In reply to comment #41)
> The problem with the proposed solution is that we'd have to audit the hell out
> of any of the go button-related event listeners/handlers in browser.js, and
> implement some shim to use whichever one is actually visible.

This may be ignorant, but couldn't we just set things up so people automatically listened to both buttons?  Since at most one should ever be visible at once, it's not as if we'll get conflicting messages that way.

Comment 43

[Simon Bünzli]

What if we refined my solution from comment #33 to applying a different binding to the whole toolbar and have that binding insert address bar and Go button inside their own box to achieve what we want? Then there'd be only one Go button after all...

What I hadn't considered for the original proposition was that IIRC themes can't contain JS code. Otherwise the solution would have been to just pass all (relevant) events from our fake Go button to the (hidden) real one.

In any way, this hack should be contained within the theme and not leak into other code - so that browser devs and extension authors won't have to make special modifications for the Go button mess (which they really shouldn't).

Comment 44

[Mike Connor [:mconnor]]

That would be via a new XBL binding, as I understand it, and thus we'd have anonymous content, with the same auditing requirements.  Replacing a binding with a theme specific binding is ok, replacing content with a binding is just as fragile.

Comment 45

[Simon Bünzli]

I've been experimenting somewhat with different bindings and haven't found a clean solution yet. One idea was to have a binding which moves the Go button into the address bar container in the binding's constructor and back out (into the then hidden Go button container) in the destructor. Unfortunately the destructor isn't called until the window is closed. This could be worked around with a second binding which is applied when the first one isn't. This worked quite well - the hacky binding would have to live outside the theme though and just be optional for other themes.

A less hacky alternative which works without a binding would be to keep the Go button inside the address bar's container, but add a fake Go button which is hidden except when the Customize Toolbar window is opened. Users could then add/remove the button as all other buttons, but can only place it adjacent to the address bar. This would then only require some additional CSS rules for hiding the fake button most of the time and for hiding the real one whenever the fake button isn't part of the toolbar.

The draw-back in this case is that the Go button can't be placed anywhere any longer - and that the behavior when the fake Go button is moved elsewhere isn't really definable (it might just disappear - or it might jump right back to the address bar).

In both cases, we could additionally show the native drop-down button whenever the Go button isn't shown through the same CSS rules...

Comment 46

[Simon Bünzli]

Attachment: WIP (winstripe only)

Alright, this looks like a more promising solution: this patch adds a new binding for the end cap Go button which includes a hidden autocomplete textarea to force the button to the address bar's height. No JS involved, so everything remains within the theme.

TBD: (1) Adapt the patch for pinstripe; (2) get new images and adapt the margins for when the Go button isn't directly adjacent to the address bar; (3) figure out where the surplus 1px margin comes from (see the XXX).

(1) is a job for somebody with a Mac, (2) I could do as soon as the image is provided, and (3) is bonus for any CSS guru (unless the patch breaks somewhere).

Before going on, I'd however like to get some feedback on the offered solution.

Comment 47

[Simon Bünzli]

Attachment: WIP v2

The same as above with most modifications ported to pinstripe (to make it easier for whoever does the final touches) and with different margin settings for the Go button depending on whether it functions as end cap or not. This patch also adds a slight margin between Go button and address bar when customizing the toolbar so that the former can easily be moved on its own.

Comment 48

[Peter Kasting]

If this is going to make it, we need a solidly working patch ASAP, certainly before the end of tomorrow

Comment 49

[Simon Bünzli]

(In reply to comment #48)
> If this is going to make it, we need a solidly working patch ASAP, certainly
> before the end of tomorrow

As I said: somebody will have to do the pinstripe testing (Ryan? Asaf? Seth?) and we'd need an image like the one from comment #11 with transparent background, including an rtl version (Jay?).

TBD: (4) The patch currently lacks rtl code (depends on the missing image); and (5) there's been some bitrot due to bug 350742.

Comment 50

[Peter Kasting]

(In reply to comment #49)
> As I said: somebody will have to do the pinstripe testing (Ryan? Asaf? Seth?)
> and we'd need an image like the one from comment #11 with transparent
> background, including an rtl version (Jay?).

_If_ she has a lot of time, Pam might be able to help with both of these.

We do have a lot of existing images in the tree for both ltr and rtl Go buttons, and the patch on bug 348138 should contain more.  The only real change needed is to add a closing border around the fourth side, which should be pretty doable.

> TBD: (4) The patch currently lacks rtl code (depends on the missing image); and
> (5) there's been some bitrot due to bug 350742.

If/when bug 348138 lands, that's going to significantly bitrot this patch.

Comment 51

[Pam Greene]

I'm hoping to get to this (refreshing, merging with bug 348138, putting together the images, and making any remaining pinstripe tweaks), but it's not the top item on my list, so I'm not making any promises.

Comment 52

[Pam Greene]

Now working on this.  I think it will affect more people than bug 348138 will, so I'm going to do this first and merge bug 348138 in, rather than the other way around.

Comment 53

[Pam Greene]

Attachment: WIP v3

More progress, but there's still some left to do.  This de-bitrotted patch, with the following images, fixes all the spacing and display issues in the Customize dialog, as well as in the three toolbar modes (text, icons, and both) in both locations (next to the urlbar and elsewhere).  The standalone Go button has been turned into a toolbarbutton, like Stop and Reload, since it looked odd otherwise.

What's left:
(1) Creation of a small-icons version of the Go button, for when it's a toolbarbutton
(2) Any needed tweaks to the CSS for the two small-icon cases (icons and icons+text)
(3) Testing and any needed adjustments for pinstripe
(4) A quick test on Linux, since this has only been run in Windows

Comment 54

[Pam Greene]

Attachment: Go buttons

Comment 55

[Pam Greene]

Attachment: RTL Go buttons

Comment 56

[Myk Melez [:myk] [@mykmelez]]

> (4) A quick test on Linux, since this has only been run in Windows

I tested on Linux and noticed these issues:

1. When next to the location bar, the search icon is stretched horizontally.

3. When next to the location bar in text-only mode, the button has no hover or active state at all (no change in color, no addition of bevel, etc.).

2. When not next to the location bar, the search icon retains a border, which looks funky since the other toolbar buttons don't have borders, and since the button itself gains a beveled border when you hover over it.

Comment 57

[Mike Connor [:mconnor]]

I'm sure this is going to get me hate mail, but we don't have a finished patch, and we're already past freeze.  This would be nice to have, but based on volume of feedback from Hendrix/Mozillazine skimming and searching, its a small and very vocal minority who care about this.  If we need a UI solution for this, we need a UI solution for closebutton modes, and a whack of other things we've changed (less fine-grained controls in prefs, search without the searchbar showing via the dialog, etc)

I am going to write a patch tomorrow to allow hiding Go/Search buttons via about:config, but we're probably not going to block on that either (should be low-risk enough to take anyway).

I'm still interested in finding a fix for this, but not at the risk of causing an additional delay for users.  That said, none of the patches along the current axis will be safe to take without any bake time, since we'll be starting builds tomorrow evening.

Comment 58

[Worcester12345]

You are talking about a freeze for the next beta or maybe RC1, right? Meaning, this will still be on the slate for 2.0 final. Or do you mean it is off for final?

Comment 59

[Dão Gottwald [:dao]]

(In reply to comment #57)
> I'm still interested in finding a fix for this, but not at the risk of causing
> an additional delay for users.

Is it possible to include such a fix in a minor update?

(In reply to comment #58)
> You are talking about a freeze for the next beta or maybe RC1, right? Meaning,
> this will still be on the slate for 2.0 final. Or do you mean it is off for
> final?

Missing the release candidate means to miss the release. (That's the theory, we'll see how it works.)

Comment 60

[Peter Kasting]

Attachment: Band-aid branch patch v1

This band-aid patch adds a "browser.urlbar.hideGoButton" pref which will toggle the button on and off.

I had to move the 5px of margin/padding on the button to the urlbar-container so the bar would not suddenly butt up against things when the button was removed.

The pref is live since:
(a) The toolbar customizations and the tab close button location pref are all live, so this is consistent
(b) There were already several other live prefs in browser.js whose code I could copy and paste with minimal effort & risk

This patch has only been tested on Linux; please test on other OSes, especially Mac.

Comment 61

[Mike Connor [:mconnor]]

Comment on attachment 238546 [details] [diff] [review]
Band-aid branch patch v1

as a branch-only patch, we should be aiming at minimal impact, and this isn't it

Comment 62

[Mike Connor [:mconnor]]

Comment on attachment 238546 [details] [diff] [review]
Band-aid branch patch v1

after much thought on this patch, going to take it.

If this causes any problems whatsoever, I'm going to back it out, and ship without it.

Comment 63

[Peter Kasting]

Band-aid patch checked in on branch:

/mozilla/browser/app/profile/firefox.js 1.71.2.75
/mozilla/browser/base/content/browser.js 1.479.2.203
/mozilla/browser/themes/winstripe/browser/browser.css 1.17.2.63
/mozilla/browser/themes/pinstripe/browser/browser.css 1.11.4.49

Leaving open for the real fix, which should be what gets used on trunk.

Comment 64

[Peter Kasting]

One more tweak (accidentally w/o a log message) to make the pref be respected after we finish toolbar customization.  Thanks to Gavin Sharp for noticing.

/mozilla/browser/base/content/browser.js 1.479.2.204

Comment 65

[Mike Connor [:mconnor]]

marking fixed-as-its-gonna-get1.8.1 so it falls off queries.  Might be worth splitting off into a new bug to discuss a more general approach that solves this for endcaps in general for Fx3

Comment 66

[Marcia Knous [:marcia]]

Looks good on Mac using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1) Gecko/20060918 BonEcho/2.0, using Peter's Comment 60 as a benchmark for testing the pref.

Comment 67

[Daniel Schroeter]

(In reply to comment #60)
> This band-aid patch adds a "browser.urlbar.hideGoButton" pref

On Linux you will see at the start of the browser a few microseconds the go button. I think the pref will be checked after everything is started up.
I think this can not be changed easy. So it's time for a faster computer ;-)

Comment 68

[Anders Conbere]

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

Comment 69

[Bruno Tenório Ávila]

I agree with this bug in order to make a simpler user interface for Firefox. The severity of the bug, however, should be enhancement.

Others ideas for simplifying the firefox user interface should be welcome, such as, removing or at least changing the google search toolbar.

Comment 70

[Nick Thomas [:nthomas] (UTC+12)]

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

Comment 71

[Michael Ventnor]

Attachment: Patch

So is the reason there has been no progress is because no-one has an idea on how to separate the button without it remaining vertically scalable? I've managed to do this by using getComputedStyle().

While I can't offer any images for the stand-alone go button, I did try to make it as easy as possible for the Future Fx3 themer to do so.

Comment 72

[Simon Bünzli]

(In reply to comment #71)
> So is the reason there has been no progress is because no-one has an idea on
> how to separate the button without it remaining vertically scalable?

I guess the reason is rather that nobody with access to both Windows and OS X has had the time for finalizing the patches from comment #47 resp. comment #53 which do solve the issue in the least-hacky way available so far.

Your patch breaks whenever the appearance of the address bar or the Go button is changed through system settings or through any other means than toolbar customization.

If you are interested to work on this, please continue where Pam was held off three months ago.

Comment 73

[Michael Ventnor]

Attachment: Patch v2

(In reply to comment #72)
> Your patch breaks whenever the appearance of the address bar or the Go button
> is changed through system settings or through any other means than toolbar
> customization.
> 

In that case adding a listener should also take care of this. What system settings do you mean? I have tried to find them before I created the previous patch and couldn't find any.

Comment 74

[Simon Bünzli]

Comment on attachment 249842 [details] [diff] [review]
Patch v2

With system settings I meant OS settings (among others DPI settings and global font sizes) which are propagated exclusively through CSS.

After further reflection, your patch does have further serious design issues:
* themes which don't want their Go button stretched have to resort to ugly hacks in order to get around this hack (since they can't contain JavaScript code)
* the Go button will look quite out-of-place when placed somewhere else than to the right of the address bar
* the issue at hand is purely a theme issue (with the exception of some XUL code which has to be reverted) and as such it should be handled within the theme itself (the fact that this wasn't done in the first place is what led us here, let's not make it any worse)

Comment 75

[Michael Ventnor]

(In reply to comment #74)
> (From update of attachment 249842 [details] [diff] [review])
> With system settings I meant OS settings (among others DPI settings and global
> font sizes) which are propagated exclusively through CSS.
> 
> After further reflection, your patch does have further serious design issues:
> * themes which don't want their Go button stretched have to resort to ugly
> hacks in order to get around this hack (since they can't contain JavaScript
> code)

I was thinking of a way to provide themers a way around this. One of my thoughts was to let them declare a constant height of the go button stack with the !important flag, and if the function saw the important flag, it would respect it and not stretch the button.

> * the Go button will look quite out-of-place when placed somewhere else than to
> the right of the address bar

That's what the isEndcap flag is for. Not only will the dynamic height attribute be removed, but it will also allow themers to easily tell whether its an endcap or a standalone button.

> * the issue at hand is purely a theme issue (with the exception of some XUL
> code which has to be reverted) and as such it should be handled within the
> theme itself (the fact that this wasn't done in the first place is what led us
> here, let's not make it any worse)
> 

The only way to fix this without some JS AFAICT is to just get rid of the endcap idea and make the go button back into a stand-alone style permanently.

Comment 76

[Simon Bünzli]

(In reply to comment #75)
> I was thinking of a way to provide themers a way around this.

They shouldn't have to work around anything in the first place. You describe yet another hack although a quite hack-free solution is already available (cf. comment #53).

> That's what the isEndcap flag is for.

I wasn't referring to other themes. Applying your patch will make the Go button look wrong when it's not where intended (i.e. your patch is incomplete and wasn't ready for review). OTOH as has been demonstrated throughout this bug, such an additional flag wouldn't be needed anyway...

> The only way to fix this without some JS AFAICT is to just get rid of the
> endcap idea and make the go button back into a stand-alone style permanently.

If you don't feel like bothering to read and understand the patch from comment #53, please consider investing your time more productively in other bugs. We really don't need a new solution here but just a completed update to the WIP patch.

Comment 77

[Michael Ventnor]

I know, I apologize. I only read comment #53 after I posted that comment. I did try to port it last night but the furthest I got was the button was still just a bit too tall (the top and bottom parts were surrounding the textbox, not aligned with it). I need to research about custom bindings...

Comment 78

[Michael Ventnor]

Attachment: Ported WIP 1

OK, I've finally managed to get a theme-centric WIP going. The only thing missing from the Winstripe portion is images to use for the standalone go button. Could someone please make some suitable images for me?

Pinstripe needs work. Not only do I not have a Mac but even after Fx2 shipped, the button wasn't designed to expand at all, based on reading the CSS file. Does changing the text size/resolution on OS X not affect the textboxes?

I'd like a review from Simon first, and if needed a follow-up review from someone else.

Comment 79

[Simon Bünzli]

Comment on attachment 249983 [details] [diff] [review]
Ported WIP 1

This looks better, thanks.

Now, I'd prefer to do away completely with #go-container since it's only purpose is to hold #go-button-stack which is theme-specific and rather belongs into browser.xml than browser.xul as well. To achieve that, make the endcap-go-button binding inherit from toolbarbutton.xml#toolbarbutton and move the stack into that binding so that you stack three things: the autocomplete textbox for the correct height, the vbox containing the different background image bits and the arrow image.

Then there's something not quite correct in your Winstripe CSS since when the patch is applied the button does not expand to the full height of the address bar (interestingly it does so precisely when it's _not_ in the endcap position).

Finally, for now you could style the Go button as it was in Firefox 1.5 for when it's not functioning as endcap by always displaying the text "Go" to the side of the arrow image (use the one you're using anyway, at least for the time being).

Comment 80

[Michael Ventnor]

Attachment: Standalone Go Buttons (.zip)

I borrowed the concept buttons from this bug and used those.

Comment 81

[Michael Ventnor]

Attachment: Winstripe-complete Patch

For some strange reason, making the go button inherit from the toolbarbutton makes it unclickable, so I had to keep toolbar-base. Everything works in Winstripe, but I haven't started Pinstripe yet and only included a few lines to make sure I don't cause damage with my other changes.

I don't have CVS access so if this gets r+ could you also check everything in?

Comment 82

[Simon Bünzli]

Comment on attachment 250428 [details] [diff] [review]
Winstripe-complete Patch

> For some strange reason, making the go button inherit from the toolbarbutton
> makes it unclickable

Make sure not to confound #go-button and #go-container. I'd like the whole now pointless #go-container to go. For that, you'll have to make your binding extend chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton and replace

<children/>

with

<xul:image class="toolbarbutton-icon" xbl:inherits="src=image"/>

(see the toolbarbutton-image binding).

Further nits:

>Index: browser/themes/winstripe/browser/browser.xml
>===================================================================

>+            <xul:image id="go-button-top"
>+                   class="go-button-background"
>+                   chromedir="&locale.dir;" 
>+                   xbl:inherits="src=image"/>

* Align the attributes so that id, class, etc. all start at the same column.

* Set chromedir on the button in browser.xul and inherit it (so you don't need to include global.dtd for this binding).

* Make sure not to include any tabs in your patches (there are some in the browser.xul bits).

> I don't have CVS access so if this gets r+ could you also check everything in?

Please, don't worry about the check-in until you get r+ by a browser peer (note: I'm not).

Comment 84

[Michael Ventnor]

Attachment: Patch 2

Don't you have a Mac, Mano? Hopefully you can tell me if anything is broken in Pinstripe.

This will need the standalone button images I uploaded before.

Calling the last patch Winstripe-only was a misconception, the purpose of this bug is to make the Go button detachable and that will happen on a Mac too.

Comment 85

[Michael Ventnor]

I guess Asaf is too busy right now...
Is there anyone else capable of reviewing this?

Comment 86

[Asaf Romano (gone)]

Index: browser/themes/winstripe/browser/browser.css
===================================================================

+toolbar:not([mode="text"]) #go-button,
+#palette-box #go-button {
...
+  margin-right: -2px; /* Center this button more when torn off url bar */
+}
+

+toolbar:not([mode="text"]) #urlbar-container + #go-button {
+  -moz-binding: url(chrome://browser/skin/browser.xml#endcap-go-button);
+  margin-right: 1px;  /* Retain gap between go button and adjacent item */
+  margin-left: -1px;
+}

Did you test these rules in RTL UI? -moz-margin-start/end should be used here, I think.

Comment 87

[Asaf Romano (gone)]

Comment on attachment 251364 [details] [diff] [review]
Patch 2

Other than that:
  * We need a standalone-image for pinstripe.
  * I see different image/border when customizing the toolbar in my hacked-up mac-winstripe, is this intended, a "mac-winstripe" issue on my side, or something else?
  * With this patch applied, removing the location bar from the toolbar does not remove the go button.

Comment 88

[Peter Kasting]

(In reply to comment #87)
>   * With this patch applied, removing the location bar from the toolbar does
> not remove the go button.

It didn't in <=1.5 either, regardless of Go button location.  And if you've separated the two, it probably shouldn't now either.  That leaves the case of removing the bar when the two are joined.  How many people remove their location bar?  Is it important to try and detect this case?

Comment 89

[Asaf Romano (gone)]

Even if they're separated, what's the purpose of the go button when the location bar isn't visible?

Comment 90

[Peter Kasting]

I don't know, but I was suggesting that getting back to the functionality of 1.5 is perhaps a reasonable goal on its own.  Why not file a separate bug on auto-removing the Go button when the location bar is removed?

Comment 91

[Asaf Romano (gone)]

Well, we already shipped 2.0 with the usability improvement of removing and adding them together.

Comment 92

[Adam Kowalczyk]

(In reply to comment #91)
> Well, we already shipped 2.0 with the usability improvement of removing and
> adding them together.

... and a usability regression of not being able to remove the Go button while leaving the Location Bar. I don't believe it is a problem for a user to remove the Go button manually after removing the Location Bar. After all, this is how it had worked for a long time and I don't remember hearing many complaints about it. The new behavior, on the other hand, provoked plenty of them. Therefore, it seems clear which use case is more important. Plus, it's a choice between making one the use cases impossible and making one o them require performing an additional simple step.

It'd be certainly nice for the Go button to be removed automatically when removing the Location Bar but it shouldn't block the resolution of this bug.

Comment 93

[Reijo]

What I think is the most important thing, is that you could place the Go-button where ever you want in the toolbar. The ability to edit easily the toolbars was the one thing I really liked in Firefox. Currently this isn't true anymore, only because of the change in the Go-button. I think it would be better to return it to what is used to be in 1.5, if there is no other way to allow users to change the location of the button. 

Comment 94

[Mike Connor [:mconnor]]

Before we decide this is something we're going to do, we should figure out what's going on with design and overall functionality for the Location Bar in Fx3.  I know a lot of people care about this very much, but I think that depending on how the location bar as an item shapes up, this might start to make less and less sense as something that can be broken up.

Comment 95

[Michael Ventnor]

Mike, just because the location bar isn't there doesn't make the go button useless. For example, you can drag an image to the go button and it will view the image by itself.

The point of customizing the toolbar is to put whatever you want wherever you want, and the go button should be no exception. IMO, if there are one or two people out there who do want the go button but no location bar then let them be, don't introduce more complex code to cripple this case.

Asaf, the border problem would most likely be due to your implementation since the only change I made in Pinstripe was to remove the gap that would have occurred from separating the buttons. The Pinstripe button was never designed to expand at all when Fx2 was released.

Also, I don't have the image skills to make standalone buttons. Could someone else please do that?

Comment 96

[Nathan Egge]

Just to add another opinion to this thread, I liked the original Firefox 1.5 theme and was happy to find that someone had ported it to Firefox 2.0 under the name Winestripe:

https://addons.mozilla.org/firefox/3479/

I had no problem installing the theme and to its credit I was very pleased that it restored the native tab rendering.

However, when I enabled the Go button it ended up stretching the URL text field vertically.  I'll attach two screenshots that show this.  I realize that its not the job of the Firefox developers to debug a third party theme, but I takled to the author and he indicated that it was out of his control.

Is this something that can be fixed in Firefox trivially?

Comment 97

[Nathan Egge]

Attachment: Firefox 2.0 with the Winestripe them, showing go button

Comment 98

[Nathan Egge]

Attachment: Firefox 2.0 with the Winestripe them, no go button

Comment 99

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

This bug is for Firefox 2. Bug 378983 is for Firefox 3.

Comment 100

[Worcester12345]

(In reply to comment #99)
> This bug is for Firefox 2. Bug 378983 is for Firefox 3.
> 

Didn't even see that. Shouldn't this one just be marked as WONTFIX then, since it was denied blocking and it looks like it will be attempted on the trunk instead?

Comment 101

[Liam K.]

This can be changed in about:config.

Comment 102

[Liam K.]

You go to about:config and change browser.urlbar.HideGoButton to true.

Comment 103

[Peter Kasting]

(In reply to comment #101)
> This can be changed in about:config.

Yes, that band-aid happened on this very bug, in comment 63.

Comment 104

[Nathan Egge]

Its good to see there is some activity on this thread.  Can someone please tell me what to do about the issue I mentioned in #96-#98.  Do I need to submit this as a separate bug?  Can someone confirm that this is something that can be fixed?

Comment 105

[Dão Gottwald [:dao]]

Nathan, the theme author does have control over that. It's actually quite simple:

.urlbar-button-box {
  -moz-box-align: center;
}


Let's close this bug, as there won't be another fix for Firefox 2. Bug 378983 is about a better one for Firefox 3.

Comment 106

[Nathan Egge]

Dão,

Thanks for that, but I'm afraid that is not the problem.  Below is the entire set of CSS related to the go button.  The author's comments seem to indicate that this is a known problem.  I wonder if this is related to GTK.

Nathan


/* ::::: go button ::::: */

/* In text icon mode, the Go button scales independently of the location bar,
 * and the button can be much taller, so we have to center align the elements
 * (instead of stretching them) to prevent the location bar from stretching
 * vertically to match the button's height and growing too tall. Despite this
 * center alignment, the history dropmarker still stretches to the location
 * bar's height, as it's inside the location bar's textbox, which is stretchy.
 */
toolbar[mode="text"] #urlbar-button-box {
  -moz-box-align: center;
}

toolbar[mode="text"] #go-button {
  -moz-margin-start: 5px;
}

toolbar[mode="text"] #go-button,
toolbar[mode="text"] #go-button-stack .go-button-background {
  list-style-image: none;
  background-image: none;
}

toolbar[mode="text"] #go-button-stack {
  padding: 0;
}

#go-button-stack {
  padding: 2px 0px 2px 0px;
}

toolbar:not([mode="text"]) #go-button {
  list-style-image: url("chrome://browser/skin/Go-arrow.png");
  -moz-image-region: rect(0px 25px 22px 0px);
  padding: 0px 1px 0px 0;
  margin: 0px;
}

toolbar:not([mode="text"]) #go-button[chromedir="rtl"] {
  list-style-image: url("chrome://browser/skin/Go-arrow-rtl.png");
}

toolbar:not([mode="text"]) #go-button-top {
  height: 10px;
}

toolbar:not([mode="text"]) #go-button-bottom {
  height: 10px;
}

#go-button-stack:hover #go-button-top {
  -moz-image-region: rect(0px, 50px, 10px, 25px);
}

#go-button-stack:hover #go-button-mid-top,
#go-button-stack:hover #go-button-mid-bottom {
  background-position: -25px 0px;
}


/* Disabled images are not used. */
#go-button-stack[disabled="true"] #go-button-top {
  -moz-image-region: rect(0px, 75px, 10px, 50px) !important;
}

#go-button-stack[disabled="true"] #go-button-mid-top,
#go-button-stack[disabled="true"] #go-button-mid-bottom {
  background-position: -50px 0px;
}

#go-button-stack[disabled="true"] #go-button-bottom {
  -moz-image-region: rect(12px, 75px, 22px, 50px) !important;
}


#go-button-stack:hover:active #go-button-top {
  -moz-image-region: rect(0px, 100px, 10px, 75px);
}

#go-button-stack:hover:active #go-button-mid-top,
#go-button-stack:hover:active #go-button-mid-bottom {
  background-position: -75px;
}

#go-button-stack:hover:active #go-button-bottom {
  -moz-image-region: rect(12px, 100px, 22px, 75px);
}


toolbar[mode="text"] #go-button > .toolbarbutton-text {
  display: -moz-box !important;
  -moz-margin-start: 4px !important;
}

/* Not used. */
#go-button:not([disabled="true"]):hover {
  -moz-image-region: rect(0px 50px 22px 25px);
}

#go-button[disabled="true"] {
  -moz-image-region: rect(0px 75px 22px 50px);
}

#go-button:not([disabled="true"]):hover:active {
  -moz-image-region: rect(0px 100px 22px 75px);
}

#go-button > .toolbarbutton-icon {
  margin: 0;
}

toolbar:not([mode="text"]) #go-button,
toolbar:not([mode="text"]) .search-go-button,
toolbarpaletteitem:not([place="toolbar"]) #go-button,
toolbarpaletteitem:not([place="toolbar"]) .search-go-button {
  -moz-binding: url(chrome://global/content/bindings/toolbarbutton.xml#toolbarbutton-image);
}

#sidebar {
  background-color: Window;
}

Comment 107

[Worcester12345]

FIXED? Isn't WORKSFORME or WONTFIX more appropriate since there wasn't really a fix, but a workaround?