347402 - Go button is attached to the search bar

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Dave Townsend [:mossop]]

If you drag the search bar off the toolbars then it takes the go button with it, and vice versa.

Comment 1

[Régis Caspar]

The Go button is misplaced in the XUL tree(from DOMi):
<toolbaritem id="search-container" title="Search" align="center"
             class="chromeclass-toolbar-additional" flex="100">
  <toolbarbutton id="go-button" chromedir="ltr"
                 onclick="handleURLBarCommand(event);" tooltiptext="Type a location in the Location Bar, then click Go"/>
  <searchbar
             src="data:image/x-icon;base64,R0lGODlhEAAQAPfLAAATVikwdA8SnxUfgAsWpAAilholjxw4jBc7kwAlvQQ2sRMsoBUqqhMzuhY/vxw4tSgmiyM1mSUztiQ6sTE3sQ4qyxMxxRoyxiAuxR1CtBxJsBxasSJuuTFguBte0Rlf2xVc9h9W9xVjzxVr0gdj6BRh4R1o5yBcyiZbyydT1i9b2Ddb1iFY6CJg2Vpor1dzvEJu20Z0yi23QDy1REi2OUy0O1WzOVC4PU+tVUe5Sk2xQU2zRUO4UE21Ula2SmKEqWWF2HyPx2+a6X6e6Xqk1m+s78sUDs4UGdEQB9YfDdwaANEfHd0YEscjAM4mAM0qANIoD9IkGdslGswuItYgL4aP0ImP2YGZ36Opzaq2wq/S+rzX/7/e8MrS1MLO/sTb48rT8snX/83c89PZ+crq+cH1/9Dl/9Ln/93r/9fy/+Hf7P/42eDm/O7u/+T29uX2/eT2/+f4/+f5/+j/9u//8+3/9u7/9ur5/+j//+n//+v//u3//+7//e7//+////b66/T/6vX/6/f/7f/07fj/4fv/4Pj/5v/45v7/4/r+7/3/6fDw+Pfx//D/9/X/8fT/8/f/8ff/8/D///H///L8/fL///P///X7//b6/ff/+/T///b9//f///v19//w9v/09P/29v/x+f/y///z///1+v/1///2///3//j79P/58/z/8/z99/z/9v7/9P7/9vn7//v6//j9//n9//j///n///v//vv////4+v/5+//6+P/4///6/P/6/v/6///7///9+P/8+v/9+v7/+Pz////8/f/9/f79///8///9//7//////////wAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACH5BAEAAMsALAAAAAAQABAAAAj/AEn4oIFjBw8bOnrMuJGjhowZM1T8UdYJUZ5ZcNRYWjSrVK5QU0DMmtUnzRAXEy4o6FCEy6NDTkQIq1MmRgM0eZTlCXMgQJtRSE4gmgUkwh1EiZTNUiamy6NUUExcuoJgDCdDjQg9KgVL2SNFT1hwEvKglLBWuixZ+jSrlSBdRlL04bBBkTBdpZTpIqWsFaBcTEr0QaEhl6dWlswKW6poDRUPlmAUQKWMkTJLc76QMQNGUZMWgIgkCFJnlq5WXigwkFClVZQQyuRgELAlk7JBymCZGYAF0ZEPrQixgUDAihxVdPpoAZAFUZIRfThxgvPCwAILDipk+OFG2ZIVoxApERtPfvwlvZ+kQFzPvv0MJQEBADs="
             empty="true" id="searchbar" flex="1"/>
</toolbaritem>

Comment 2

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

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

Comment 3

[Jay Patel [:jay]]

I know this was probably part of the "visual refresh" for Firefox 2, but we really should detach the go button from the search bar.  

I personally think the current coupling of the two is ridiculous... why did we do that?  Nominating for blocking-firefox2 so we can reconsider this UI change that will most likely piss a lot of users off.

Comment 4

[Jackie Liu]

Yes, this definitely should be fixed.  I second Jay's nomination for this blocking  Firefox 2.

Comment 5

[Eric Foley]

Im new to all this, so what would blocking do and how would i do it?

Comment 6

[Phil Ringnalda (:philor)]

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

Comment 7

[Ed Hume]

Check out my screenshot (from the bug I filed, a dupe of this one):

https://bugzilla.mozilla.org/attachment.cgi?id=232262&action=view

This shows what happens when you move the searchbar up to the Menus toolbar to give both the urlbar and the searchbar more room.

Comment 8

[Régis Caspar]

Attachment: patch (proposal)

This patch moves the #go-button into the #urlbar-container

Comment 9

[Ria Klaassen (not reading all bugmail)]

Now they are free to go their own way :).

Comment 10

[Thorsten Peter]

Attachment: different approach, restores old behaviour with go-button as a seperate toolbar-button, attached to nothing

This patch should restore the old behaviour of the go-button before the new default theme checkins. I am pretty sure that most people want to decide by themselves if, and where they want to have the go-button. 
Pre-attaching it to any other container (searchbar, urlbar etc) doesn't make any sense to me ...

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #10)
> Created an attachment (id=232318) [edit]
> different approach, restores old behaviour with go-button as a seperate
> toolbar-button, attached to nothing

Er ... why do you review your own patch?
Looks like the indentions are wrong. And what the heck is this:

> +                    style="max-width: 20px !important; padding: 1px 3px 1px 5px !important;"

Comment 12

[Thorsten Peter]

(In reply to comment #11)

> Er ... why do you review your own patch?

sorry, my bad, edited ...
> Looks like the indentions are wrong. And what the heck is this:
> 
> > +                    style="max-width: 20px !important; padding: 1px 3px 1px 5px !important;"
> 

well this is supposed to be a proposal. if i don't set it to a fixed style, the result is a flexible width which will be a go-button of 100px width if you drag the button to an area with enough space.

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 232318 [details] [diff] [review]
different approach, restores old behaviour with go-button as a seperate toolbar-button, attached to nothing

Thorsten, one of the intentions of the new theme is to attach the go button to the end of the URL bar (after much discussion on the newsgroups) and Regis' patch fixes the bug in this way so lets let that one get reviewed without muddying the waters with multiple solutions.

Incidentally, when you request review you have to do so from a particular person. And your patch would likely have been denied review for using a style attribute. Styles should be set by the themes stylesheet so other themes can override them.

Comment 14

[Thorsten Peter]

(In reply to comment #13)
> (From update of attachment 232318 [details] [diff] [review] [edit])
> Thorsten, one of the intentions of the new theme is to attach the go button to
> the end of the URL bar (after much discussion on the newsgroups) and Regis'
> patch fixes the bug in this way so lets let that one get reviewed without
> muddying the waters with multiple solutions.
> 
> Incidentally, when you request review you have to do so from a particular
> person. And your patch would likely have been denied review for using a style
> attribute. Styles should be set by the themes stylesheet so other themes can
> override them.
> 

allright dave, no problem with me if this was a decision made on the newsgroups. though i still don't get the point of not letting the user choose the go-button position.
anyways, sorry for being a bit chaotic with this patch, but it was the first one i ever officially submitted ;-)

Comment 15

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

(In reply to comment #3)
> I personally think the current coupling of the two is ridiculous... why did we
> do that?  Nominating for blocking-firefox2 so we can reconsider this UI change
> that will most likely piss a lot of users off.

It was not an intentional change (most bugs aren't! :).

Comment 16

[Thorsten Peter]

(In reply to comment #15)
> (In reply to comment #3)
> > I personally think the current coupling of the two is ridiculous... why did we
> > do that?  Nominating for blocking-firefox2 so we can reconsider this UI change
> > that will most likely **** a lot of users off.
> 
> It was not an intentional change (most bugs aren't! :).
> 

Well, the question still is (for me at last) if it makes sense to couple the go-button with another container at all. Of course it was intended to be coupled with the urlbar and not the searchbar. Still it might be good to reconsider the reasons behind the coupling after all ...

Comment 17

[Ed Hume]

I saw the discussion on the Google Groups site. Big, messy and detailed about this twitch and that spot. The limitations of the format left the users resorting to drawing pictures with text characters - 25-year-old technology, that. 

My personal beef: when I pointed out that this button and the search-go button presupposed a fixed textbar height, there was not one response. In essence, they were so focused on the details that they gave no thought to larger questions. Further, the manner that these changes were executed, as evidenced in this bug, tells us that attention was too focused on a particular default set, and did not think about the flexibility needed to accommodate the needs and desires of our users to do other things.

So I have no respect at all for those discussions. Now it's time to fix what is broken.

Comment 18

[Jackie Liu]

Eric, if memory serves me correct, blocking, say Firefox 2, flags this bug "must be fixed" before the specified release comes out, in this case Firefox 2.  Setting (or not) this attribute is usually left up to the reporter, in this case, you.  If you decide to do this, go to the Flags: section of this bug, where it says blocking-firefox2, set the drop-down menu value to +, and commit.

P.S. It seems that this bug is already on it's way to being fixed! :)

Comment 19

[Dave Townsend [:mossop]]

This bug is not for discussion about whether the go button should or should not be attached to the url bar or any other aspect of the new theme. Anything not related to fixing this particular bug should go elsewhere, probably to the newsgroups.

Comment 20

[Peter Kasting]

Comment on attachment 232318 [details] [diff] [review]
different approach, restores old behaviour with go-button as a seperate toolbar-button, attached to nothing

Patches are worthless without review requests...

I am partly flagging this for the purpose of reviewing the patch itself (which probably has issues, see comment 13), and partly for the purpose of review of the _intent_ of the patch, which I vastly prefer to the other patch.  It's OK to make a Go button that would look odd when moved elsewhere on a toolbar (anyone moving the button will immediately know how to solve their problem or else re-theme the button), but forcing users to have the Go button or no url bar at all doesn't even give them that choice.

Comment 21

[Ria Klaassen (not reading all bugmail)]

Attachment: Screenshot comparison two patches

The first one looks best.
The second patch has extra space around the go-button.
Maybe a pref could be made for people who don't want a go- (and search) button?

Comment 22

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #21)
>
Hm, that only because of the style rule inside the xul.
I don't seen any difference at all between the two patches without it.
I don't see a 100px button either.

Comment 23

[Ria Klaassen (not reading all bugmail)]

When you do just:

</deck>
      </toolbaritem>

      <toolbaritem id="search-container" title="&searchItem.title;"
                    align="center" class="chromeclass-toolbar-additional"
                    flex="100">
        
        <searchbar id="searchbar" flex="1"/>
      </toolbaritem>
      
      <toolbarbutton id="go-button"
                       chromedir="&locale.dir;"
                       onclick="handleURLBarCommand(event);"
                       tooltiptext="&goButton.tooltip;"/>
      
      <toolbarbutton id="print-button" class="toolbarbutton-1 chromeclass-toolbar-additional"


it opens correctly with the go-button attached, but you can also drag the button into the customize window and I haven't see any problems until now but maybe I have still mud in my eyes.

Comment 24

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #23)
> it opens correctly with the go-button attached
>
that's because of the used localstore file (because of the previous dragging action in that profile) but it looks not a big problem to me.

Comment 25

[Thorsten Peter]

(In reply to comment #24)
> (In reply to comment #23)
> > it opens correctly with the go-button attached
> >
> that's because of the used localstore file (because of the previous dragging
> action in that profile) but it looks not a big problem to me.
> 

well it still depends on what user wants, if you call that a problem. take your approach without fixing a max-width. take the go-button, drag it to an area with enough space left of the wanted go-button postition and you'll see the 100px go-button :-) wanna bet?

Comment 26

[Dão Gottwald [:dao]]

(In reply to comment #25)
> take your
> approach without fixing a max-width. take the go-button, drag it to an area
> with enough space left of the wanted go-button postition and you'll see the
> 100px go-button

I don't see this on the trunk and I'm wondering why this would be different on the branch. After all it's a "toolbarbutton" without the flex attribute. If you're right, I'd guess the new theme's CSS is flawed.

Comment 27

[Thorsten Peter]

(In reply to comment #26)
> (In reply to comment #25)
> > take your
> > approach without fixing a max-width. take the go-button, drag it to an area
> > with enough space left of the wanted go-button postition and you'll see the
> > 100px go-button
> 
> I don't see this on the trunk and I'm wondering why this would be different on
> the branch. After all it's a "toolbarbutton" without the flex attribute. If
> you're right, I'd guess the new theme's CSS is flawed.

Sorry man, you were right. With the default theme, clean install the button is NOT flexible. 

Comment 28

[Mike Connor [:mconnor]]

Comment on attachment 232318 [details] [diff] [review]
different approach, restores old behaviour with go-button as a seperate toolbar-button, attached to nothing

We made the decision to keep the endcaps as part of the textfield binding.  Not looking to revisit that here.

Comment 29

[Mike Connor [:mconnor]]

Comment on attachment 232294 [details] [diff] [review]
patch (proposal)

Poor gavin, the shame, the shame.  Plussing for 1.8.1 since the new theme is only landed there.

Comment 30

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

mozilla/browser/base/content/browser.xul 1.268.2.51

Comment 31

[Tracy Walker [:tracy]]

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