529647 - Make Search and Go buttons widgets for Browser customizable toolbars

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Ian Neal]

At the moment the search and go buttons on the browser page are part of the URL bar widget and are controlled using browser.toolbars.showbutton.* prefs, they should be separate widgets.

Comment 1

[Ian Neal]

Attachment: Separate Go and Search buttons patch v0.1

This patch:
* Makes Go and Search buttons separate widgets to the URL bar.
* Removes browser.toolbars.showbutton.* listener and users.
* Removes prefs and preferences for go and search buttons.
* Removes some old preferences for mailnews that got missed.

Comment 2

[Ian Neal]

Attachment: Go, Search and Migrate patch v0.1a

Changes since v0.1:
* Added button to the id names for the go and search button containers.
* Removed migration code for the browser.toolbars.showbutton.* prefs.

Comment 3

[[e-mail]]

In Modern those buttons look a bit out of place "outside" the nav-bar-inner...

Comment 4

[Philip Chee]

Comment on attachment 413432 [details] [diff] [review]
Go, Search and Migrate patch v0.1a

Well the code works so I'll give a r+ but

> In Modern those buttons look a bit out of place "outside" the nav-bar-inner...

I agree with Neil about this. so I'm giving a ui-r-

Firefox places the go button inside the URLbar itself and uses only an icon with no label. I wonder how our Go button looks inside our URLbar.

I think the search button might work better as a regular toolbar button with the usual large/small icons and regular non-bold text label. But I suspect that traditionalists will throw a tantrum.

Comment 5

[Ian Neal]

(In reply to comment #4)
> (From update of attachment 413432 [details] [diff] [review])
> Well the code works so I'll give a r+ but
> 
> > In Modern those buttons look a bit out of place "outside" the nav-bar-inner...
> 
> I agree with Neil about this. so I'm giving a ui-r-
Is there an easy way of putting them inside the nav-bar-inner?

> Firefox places the go button inside the URLbar itself and uses only an icon
> with no label. I wonder how our Go button looks inside our URLbar.
Are you sure Firefox has a Go button at all?

> I think the search button might work better as a regular toolbar button with
> the usual large/small icons and regular non-bold text label. But I suspect that
> traditionalists will throw a tantrum.
I can create a patch that would do the search button like that, but there would need to be a follow-up one for getting a better icon set for both themes. I'm sure traditionalists will be fine, we're not giving a separate search field like in Firefox.

Comment 6

[Ian Neal]

Attachment: search button large/small icons in classic/modern screenshots

Icon is stretched in a slightly strange way for large icons but hopefully you get the idea.

Comment 7

[Philip Chee]

> Are you sure Firefox has a Go button at all?
browser.xul certainly has a "go-button" in m-191, m-192, an m-c. It hasn't looked like a "go" button since FX2.0, but it's there.

> Created an attachment (id=414779)
> search button large/small icons in classic/modern screenshots

Sigh. It looks like in some configurations the toolbaritem (modern, small, icons-only, text-on-right) works better and in others the toolbarbutton. I've changed my mind, and now think that both the search and go buttons should remain as part of the urlbar widget. But if you want I'll give a ui-r+ to your patch.

Comment 8

[Ian Neal]

(In reply to comment #3)
> In Modern those buttons look a bit out of place "outside" the nav-bar-inner...

TBH, it looks strange if those buttons are put anywhere but their original positions next to the url bar - so what to do?

Comment 9

[Ian Neal]

(In reply to comment #7)
> > Are you sure Firefox has a Go button at all?
> browser.xul certainly has a "go-button" in m-191, m-192, an m-c. It hasn't
> looked like a "go" button since FX2.0, but it's there.
> 
Yes, there is something with an id of "go-button" but it is of zero width on the default theme for 3.5.x (and probably all 3.x versions).

Comment 10

[Ian Neal]

(In reply to comment #9)
> (In reply to comment #7)
> > > Are you sure Firefox has a Go button at all?
> > browser.xul certainly has a "go-button" in m-191, m-192, an m-c. It hasn't
> > looked like a "go" button since FX2.0, but it's there.
> > 
> Yes, there is something with an id of "go-button" but it is of zero width on
> the default theme for 3.5.x (and probably all 3.x versions).

Ah, until you start typing in the url bar, then it turns into a green arrow on 3.5.x.

Comment 11

[Philip Chee]

> Ah, until you start typing in the url bar, then it turns into a green arrow on
> 3.5.x.
Yes. This caused more than a few support threads in Mozillazine from users upgrading from Firefox 2.0, "wha? Where did my go button disappear off to?"

After thinking about it for a while my conclusions are that the search and go buttons are thematically and structurally part of the URL bar and don't make sense detached or relocated away from it. I propose that this bug is WONTFIX and the buttons left as is. Moving them into the actual urlbar textbox like the Feeds button has other issues as well (see why it is an image rather than an actual button).

One enhancement we could make is to borrow the Firefox 2.0 right green triangle that they used for their Go button and use that when the URLbar is in Icon mode.

Comment 12

[[e-mail]]

I can think of two^H^H^Hthree other approaches:
1. Use script to tweak the position of the Go and Search buttons. Basically just before customisation, move them out of the nav-bar-inner item, and just after customisation or on window open, move them back in.
2. Create two dummy buttons. If the dummy buttons are placed to the left of the nav-bar-inner item, the Go and Search buttons appear due to the magic of CSS.
3. Split the nav-bar-inner item into a left cap, a urlbar, a Go, a Search and a right cap item. Optionally write magic CSS so that the Go and Search buttons only get their borders if they are suitably capped.
4. Make nav-bar-inner a toolbox with Go and Search in its palette ;-)

Comment 13

[Ian Neal]

Attachment: go, search, urlbar and migrate patch v0.2

Changes since v0.1a:
* Use css and js to add border in modern theme;

Requesting new review due to amount of changes.

Comment 14

[Philip Chee]

Comment on attachment 450181 [details] [diff] [review]
go, search, urlbar and migrate patch v0.2

The ui works so ui-r+, but...

> +function UpdateNavBar()
> +{
> +  var elements = getNavToolbox().getElementsByClassName("nav-bar-class");
> +  for (let i = 0; i < elements.length; i++) {
> +    let element = elements[i];
> +    let className = element.className.replace(" nav-bar-last", "")
> +                           .replace(" nav-bar-first", "");

element.classList.remove("nav-bar-first");
element.classList.remove("nav-bar-last");

> +    let next = element.nextSibling;
> +    if (!next ||
> +        (next && next.className.indexOf("nav-bar-class") < 0))
> +      className = className + " nav-bar-last";

(next && !next.classList.contains("nav-bar-class")))
element.classList.add("nav-bar-last");

> +    let previous = element.previousSibling;
> +    if (!previous ||
> +        (previous && previous.className.indexOf("nav-bar-class") < 0))
> +      className = className + " nav-bar-first";
> +    element.className = className;

(previous && !(previous.classList.contains("nav-bar-class")))
element.classList.add("nav-bar-first");

> -      <preference id="mail.toolbars.showbutton.file"
> -                  name="mail.toolbars.showbutton.file" type="bool"/>
> -      <preference id="mail.toolbars.showbutton.next"
> -                  name="mail.toolbars.showbutton.next" type="bool"/>
> -      <preference id="mail.toolbars.showbutton.print"
> -                  name="mail.toolbars.showbutton.print" type="bool"/>
> -      <preference id="mail.toolbars.showbutton.stop"
> -                  name="mail.toolbars.showbutton.stop" type="bool"/>
> -      <preference id="mail.toolbars.showbutton.junk"
> -                  name="mail.toolbars.showbutton.junk" type="bool"/>

Part of some other bug?

>  #search-button {
> -  font-weight: bold;

Any particular reason?

> +  border-left: 2px solid;
> +  padding-left: 3px;

> +  border-right: 2px solid;
> +  padding-right: 3px;

Can we use -moz-border-{start|end} -moz-padding-{start|end} to simplify things here?

I don't suppose we could use -moz-transform: scaleX(-1) for even more simplicity? Oh yeah, labels won't come out correctly.

Comment 15

[Philip Chee]

FYI:

> element.classList.remove("nav-bar-first");
> element.classList.remove("nav-bar-last");

Even if classList were not available, you should have used .split(/\s+/);  .splice(); .push(); .join(" "); etc.

See: <https://developer.mozilla.org/en/DOM/element.classList#JavaScript_shim_for_other_implementations>

Comment 16

[[e-mail]]

Comment on attachment 450181 [details] [diff] [review]
go, search, urlbar and migrate patch v0.2

>+    if (!next ||
>+        (next && next.className.indexOf("nav-bar-class") < 0))
Definitely no need to use (next &&), but if classList doesn't work then you should prefer !/nav-bar-class/.test(next.className) over indexOf.

>+toolbar[mode="text"] > .nav-bar-class,
>+toolbar[iconsize="small"] > .nav-bar-class,
>+toolbar[mode="text"] > toolbarpaletteitem > .nav-bar-class,
>+toolbar[iconsize="small"] > toolbarpaletteitem > .nav-bar-class {
>   margin: 0 !important;
>   border: none !important;
> }
Because you moved the padding you need to override it here too. (Most obviously noticed by entering/ending customisation when the border is disabled.)
sr=me with that fixed.

Comment 17

[Ian Neal]

(In reply to comment #14)
> (From update of attachment 450181 [details] [diff] [review])
> The ui works so ui-r+, but...
> 
Getting classList stuff to work was logged as a separate bug (bug 571390) and fixed by smaug - yay.

> > -      <preference id="mail.toolbars.showbutton.file"
> > -                  name="mail.toolbars.showbutton.file" type="bool"/>
> > -      <preference id="mail.toolbars.showbutton.next"
> > -                  name="mail.toolbars.showbutton.next" type="bool"/>
> > -      <preference id="mail.toolbars.showbutton.print"
> > -                  name="mail.toolbars.showbutton.print" type="bool"/>
> > -      <preference id="mail.toolbars.showbutton.stop"
> > -                  name="mail.toolbars.showbutton.stop" type="bool"/>
> > -      <preference id="mail.toolbars.showbutton.junk"
> > -                  name="mail.toolbars.showbutton.junk" type="bool"/>
> 
> Part of some other bug?
No, these were prefs that should have been removed when the rest of the toolbar was customised.
> 
> >  #search-button {
> > -  font-weight: bold;
> 
> Any particular reason?
Yes, as it is already set in the previous CSS rule.

> > +  border-left: 2px solid;
> > +  padding-left: 3px;
> 
> > +  border-right: 2px solid;
> > +  padding-right: 3px;
> 
> Can we use -moz-border-{start|end} -moz-padding-{start|end} to simplify things
> here?
Done where applicable.

(In reply to comment #16)
> (From update of attachment 450181 [details] [diff] [review])
> >+    if (!next ||
> >+        (next && next.className.indexOf("nav-bar-class") < 0))
> Definitely no need to use (next &&), but if classList doesn't work then you
> should prefer !/nav-bar-class/.test(next.className) over indexOf.
Done with classList.
> 
> >+toolbar[mode="text"] > .nav-bar-class,
> >+toolbar[iconsize="small"] > .nav-bar-class,
> >+toolbar[mode="text"] > toolbarpaletteitem > .nav-bar-class,
> >+toolbar[iconsize="small"] > toolbarpaletteitem > .nav-bar-class {
> >   margin: 0 !important;
> >   border: none !important;
> > }
> Because you moved the padding you need to override it here too. (Most obviously
> noticed by entering/ending customisation when the border is disabled.)
> sr=me with that fixed.
Fixed.

Comment 18

[Ian Neal]

Attachment: go, search, urlbar and migrate with classList patch v0.2a [Checkin: Comment 20]

Changes since v0.2:
* JS now uses fix classList method;
* Used -moz-border-{start|end} -moz-padding-{start|end} where applicable;
* Overrides padding when mode=text or iconsize=small;
* Removed unnecessary toolbar[mode="text"] .nav-bar-class rule.

I'm not sure about the unequal margins round the urlbar, search-button and go-button, but it has always been that way in modern, so perhaps for another bug?
Update of help is covered in bug 536257

Comment 19

[Philip Chee]

Comment on attachment 451179 [details] [diff] [review]
go, search, urlbar and migrate with classList patch v0.2a [Checkin: Comment 20]

r=me.

> Created an attachment (id=451179)
> go, search, urlbar and migrate with classList patch v0.2a

> I'm not sure about the unequal margins round the urlbar, search-button and
> go-button, but it has always been that way in modern, so perhaps for another
> bug?

I'm undecided if we need the groove around the go/search buttons when they are not part of the urlbar, e.g. if one is moved to the beginning of the nav-bar and the other to the end. But as you say we can always do this as part of a follow-up bug

Comment 20

[Ian Neal]

Comment on attachment 451179 [details] [diff] [review]
go, search, urlbar and migrate with classList patch v0.2a [Checkin: Comment 20]

http://hg.mozilla.org/comm-central/rev/18235de07174

Comment 21

[[e-mail]]

Comment on attachment 451179 [details] [diff] [review]
go, search, urlbar and migrate with classList patch v0.2a [Checkin: Comment 20]

>+    element.classList.remove("nav-bar-last");
>+    element.classList.remove("nav-bar-first");
>+    var next = element.nextSibling;
>+    if (!next || !next.classList.contains("nav-bar-class"))
>+      element.classList.add("nav-bar-last");
>+    var previous = element.previousSibling;
>+    if (!previous || !previous.classList.contains("nav-bar-class"))
>+      element.classList.add("nav-bar-first");
Since you don't have to manage the class string yourself any more, it would have been slightly neater to use else instead of unconditional removal i.e.
if (previous && previous.classList.contains("nav-bar-class"))
  element.classList.remove("nav-bar-first");
else
  element.classList.add("nav-bar-first");

Comment 22

[Tony Mechelynck [:tonymec]]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:2.0b2pre) Gecko/20100701 Lightning/1.1a1pre SeaMonkey/2.1a3pre - Build ID: 20100701014002

(Finally I come across this bug.)
I VERIFY that Go and Search now behave like any cutomizable toolbar buttons (and the first time they did, I almost filed a bug for missing buttons in the UI and missing entries in the prefs popup, until I thought to have a look at the buttons palette).

I notice that browser.toolbars.showbutton.go doesn't exist by default in about:config anymore, and that "resetting" it (from true to unset) doesn't make the button disappear. (I haven't yet tried to restart the browser after that, but I believe that that pref is now obsolete and without function.)

I can't find what used to be the pref for the Search button, which may mean that its default used to be <true> and that it has been removed.

=> VERIFIED.

I don't know if this bug is worthy of testing (automatic or litmus).

Comment 23

[Philip Chee]

> I can't find what used to be the pref for the Search button

Look at the patch in comment 20 . Notice that the following lines were removed:

-pref("browser.toolbars.showbutton.go",      false);
-pref("browser.toolbars.showbutton.search",  true);

Go on make a guess.

Comment 24

[Tony Mechelynck [:tonymec]]

In reply to comment #23: That's what I thought; but I was setting VERIFIED based only on what could be observed by using the software (as opposed to looking into its source). :-) I could find the showbutton.go pref because I had set it previously (years ago, maybe) and it had remained in my "user set" prefs until I reset it earlier today.

Comment 25

[Alan]

I just upgraded from 2.09 to 2.1b1 and my Go button disappeared. This seems to be the relevant place to mention it (although if I'm honest I only use the Go button instead of Reload which resubmits form information which I almost never want to do).

Comment 26

[Philip Chee]

Alan, bugzilla is not for tech support questions. Please use the proper forums to find out how to move the Go button from the customize window back on to a toolbar.