Closed Bug 347402 Opened 13 years ago Closed 13 years ago

Go button is attached to the search bar

Categories

(Firefox :: Toolbars and Customization, defect)

2.0 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 2 beta2

People

(Reporter: mossop, Assigned: regis.caspar+bz)

References

Details

(Keywords: fixed1.8.1)

Attachments

(3 files)

If you drag the search bar off the toolbars then it takes the go button with it, and vice versa.
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>


*** Bug 347459 has been marked as a duplicate of this bug. ***
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.
Flags: blocking-firefox2?
Yes, this definitely should be fixed.  I second Jay's nomination for this blocking  Firefox 2.
Im new to all this, so what would blocking do and how would i do it?
*** Bug 347486 has been marked as a duplicate of this bug. ***
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.
Attached patch patch (proposal)Splinter Review
This patch moves the #go-button into the #urlbar-container
Attachment #232294 - Flags: review?
Attachment #232294 - Flags: review? → review?(mconnor)
Now they are free to go their own way :).
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 ...
Attachment #232318 - Flags: review+
(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;"
Attachment #232318 - Flags: review+ → review?(boba)
(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 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.
Attachment #232318 - Flags: review?(boba)
(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 ;-)
(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! :).
(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 ...
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.
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! :)
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 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.
Attachment #232318 - Flags: review?(mconnor)
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?
(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.
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.
(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.

(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?
(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.
(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 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.
Attachment #232318 - Flags: review?(mconnor)
Flags: blocking-firefox2? → blocking-firefox2+
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.
Attachment #232294 - Flags: review?(mconnor)
Attachment #232294 - Flags: review+
Attachment #232294 - Flags: approval1.8.1+
Assignee: nobody → regis.caspar+bz
mozilla/browser/base/content/browser.xul 1.268.2.51
Status: NEW → RESOLVED
Closed: 13 years ago
Keywords: fixed1.8.1
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → Firefox 2 beta2
*** Bug 347754 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.