400327 - automatically add splitter whenever location bar and search bar are adjacent

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P2)

Description

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

Bug 267831 added a splitter to the customize toolbar palette to allow resizing of the search bar, but that ended up:

a) not being discoverable to existing users,
b) resulting in strangeness when it was moved to be between, say, two buttons

Upon reflection and based on feedback in that bug, we should

 - remove the splitter item from the customize toolbar palette
 - do something more like attachment 262241 [details] [diff] [review] where we automatically add the splitter between search and location bars allowing them to be resized

Comment 1

[Jesse Ruderman]

I prefer the solution in bug 393733: Allow resizing the search bar in Customize Toolbars mode (without adding a splitter).

Comment 2

[Neil Deakin]

Attachment: use a different mechanism for the search reszier

Removes the splitter from the customize toolbar window, and just inserts the splitter between the search and urlbar items when they are next to each other.

The nsSplitterFrame is actually the fix for bug 391121. It adds support for resizebefore/after="flex" which is equivalent to "closest" except that it only resizes flexible elements.

Comment 3

[Dão Gottwald [:dao]]

It would be nice to read some reasoning as to why this solution is better than bug 393733. That is, why add a permanent UI element for something that most users will use never or once? Showing the splitter while customizing (which is the opposite of what your doing) is the way to go, IMHO.

Comment 4

[Neil Deakin]

I think the reasoning is to make it more discoverable. In bug 393733, not only does one have to figure out how to customize toolbars, one also has to notice the little dot that appears between the two fields.
 

Comment 5

[Dão Gottwald [:dao]]

(In reply to comment #4)
> the little dot that appears between the two fields.

Note that the dot is Mac-specific. On Linux and Windows it's a rather ugly vertical bar.

In any case, if the splitter would only be visible while customizing, it could be more eye-catching and use a stronger visual metaphor like <http://www.lo-motion.de/t/images/resize.gif>.

Comment 6

[[e-mail]]

Comment on attachment 285466 [details] [diff] [review]
use a different mechanism for the search reszier

I'm reduced to making really obscure style nits ;-)

>-  enum ResizeType { Closest, Farthest, Grow };
>+  enum ResizeType { Closest, Farthest, Grow, Flex };
I think this would be neater with Grow last, since only resizeafter uses Grow.

>     case 0: return Farthest;
>     case 1: return Grow;
>+    case 2: return Flex;
(... in which case I would reorder uses such as this into the same order.)

>   // if the resizebefore is closest we must reverse the list because the first child in the list
>   // is the Farthest we want the first child to be the closest.
>-  if (resizeBefore == Closest)
>+  if (resizeBefore == Closest || resizeBefore == Flex)
>      Reverse(mChildInfosBefore, mChildInfosBeforeCount);
Since resizeBefore can't be Grow, you could use resizeBefore != Farthest, or if you prefer, reorder Flex before Farthest and use resizeBefore <= Farthest.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 285466 [details] [diff] [review]
use a different mechanism for the search reszier

>Index: browser/base/content/browser.js

>+    splitter = document.createElement("splitter");
>+    splitter.id = "urlbar-search-splitter";
>+    splitter.setAttribute("resizebefore", "flex");
>+    splitter.setAttribute("resizeafter", "flex");

The splitter should have the "chromeclass-toolbar-additional" class name.

Comment 8

[Dão Gottwald [:dao]]

Also, maybe the element should be created lazily to minimize the cost if the location bar and the search bar aren't siblings?

Comment 9

[Simon Bünzli]

What about a slightly more general implementation: adding a splitter after every flex'ed toolbar item if there's another one on the same toolbar following (resp. adding one before every flex'ed item if we've already passed one while searching)? That would allow to (1) have e.g. Stop and Reload between address and search bar or (2) move the search bar down to the Bookmarks toolbar - while still being able to resize it. Furthermore it's nicer for extension authors (who'd otherwise have to implement the same feature for their own extensions - as do many search toolbar extensions, etc.).

Comment 10

[Neil Deakin]

Attachment: address comments from Neil and Dão

Comment 11

[Asaf Romano (gone)]

Comment on attachment 286562 [details] [diff] [review]
address comments from Neil and Dão

>Index: browser/base/content/browser.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser.js,v
>retrieving revision 1.870
>diff -u -p -8 -r1.870 browser.js
>--- browser/base/content/browser.js	13 Oct 2007 02:43:48 -0000	1.870
>+++ browser/base/content/browser.js	29 Oct 2007 15:47:49 -0000
>@@ -1065,16 +1065,18 @@ function delayedStartup()
>   Cc["@mozilla.org/login-manager;1"].getService(Ci.nsILoginManager);
> 
>   if (gMustLoadSidebar) {
>     var sidebar = document.getElementById("sidebar");
>     var sidebarBox = document.getElementById("sidebar-box");
>     sidebar.setAttribute("src", sidebarBox.getAttribute("src"));
>   }
> 
>+  UpdateUrlbarSearchSplitterState();
>+
>   initPlacesDefaultQueries();
>   initBookmarksToolbar();
>   PlacesUtils.bookmarks.addObserver(gBookmarksObserver, false);
>   PlacesStarButton.init();
> 
>   // called when we go into full screen, even if it is
>   // initiated by a web page script
>   window.addEventListener("fullscreen", onFullScreen, true);
>@@ -2151,16 +2153,42 @@ function canonizeUrl(aTriggeringEvent, a
>   }
> 
>   gURLBar.value = getShortcutOrURI(url, aPostDataRef);
> 
>   // Also update this so the browser display keeps the new value (bug 310651)
>   gBrowser.userTypedValue = gURLBar.value;
> }
> 
>+function UpdateUrlbarSearchSplitterState()
>+{
>+  var splitter = document.getElementById("urlbar-search-splitter");
>+
>+  var urlbar = document.getElementById("urlbar-container");
>+  var searchbar = document.getElementById("search-container");
>+  var ibefore = null;
>+  if (urlbar.nextSibling == searchbar)
>+    ibefore = searchbar;
>+  else if (searchbar.nextSibling == urlbar)
>+    ibefore = urlbar;
>+  else if (splitter.parentNode)
>+    splitter.parentNode.removeChild(splitter);

Shouldn't this be |else if (splitter)|
>+
>+  if (ibefore) {
>+    if (!splitter) {
>+      splitter = document.createElement("splitter");
>+      splitter.id = "urlbar-search-splitter";
>+      splitter.setAttribute("resizebefore", "flex");
>+      splitter.setAttribute("resizeafter", "flex");
>+      splitter.setAttribute("class", "chromeclass-toolbar-additional");

nit: use .className.


r=mano otherwise.

Comment 12

[Neil Deakin]

Attachment: address comments

Comment 13

[u88484]

So is this splitter resize supposed to be the ugly bar again and not the 4 dots?

Comment 14

[Martin Stubenschrott]

(In reply to comment #13)
> So is this splitter resize supposed to be the ugly bar again and not the 4
> dots?

As it is only visible when customizing the toolbar, it should be better visible than the 4 dots which looked nice when they are there always, but lack contrast/discoverability when you open the configure toolbars dialog.

PS: I really like the idea of just showing in the customize dialog.

Also, why is that customize toolbars dialog so ... ugly? I mean, i like the general idea how it looks, but the icons there are not placed in a grid like e.g. icons in a filemanager, but they just look like they are placed randomly. Also the 1. line has 5 icons, the 2nd and 3rd just 4 icons. Why?

Comment 15

[Jonathan Haas]

I think it would make much more sense if the splitter would be visible only in customize mode. Currently it is the other way round.

And there is an invisible splitter in customize mode now, that can be dragged to a toolbar and causes the splitter to disappear in normal mode.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Could this have caused bug 403724? The tree is currently closed so it would be
great if someone could look into it asap.

Comment 17

[Ria Klaassen (not reading all bugmail)]

This bug makes bookmarks toolbar items disappear (when search bar is in customize window).

Comment 18

[Ria Klaassen (not reading all bugmail)]

The bookmarks toolbar items are gone after a restart.
There are more issues without search bar on toolbar: after opening customize window the menus turn grey and dysfunctional and you can't open the customize window for a second time. After a restart the menus are functional again until you reopen the customize window. 
So you can open the customize window only once per session.

Comment 19

[Ria Klaassen (not reading all bugmail)]

Sorry for not being clear, but also the other issues from comment 18 are tested with the hourlies from http://hourly-archive.localgho.st/ and they indicated to this bug as the possible cause but also bug 400872 is still open and maybe this could also have influenced this?

Comment 20

[pal-moz]

(In reply to comment #18)
> you can't open the customize window for a second time. 
> 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007111312 Minefield/3.0b2pre ID:2007111312

I can.
no problem with 2nd/3rd.... time.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Backed out to see if that fixes Ts regression in bug 403724

Comment 22

[Jim Jeffery not reading bug-mail 1/2/11]

As a side-note, this patch also broke the 'native styling' of the re-sizer.

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007111413 Minefield/3.0b2pre Firefox/3.0 ID:2007111413

Comment 23

[Reed Loden [:reed]]

(In reply to comment #21)
> Backed out to see if that fixes Ts regression in bug 403724

Relanded at 2007-11-14 19:38.

Comment 24

[David Koppelman]

The bookmarks problem described in Comment 18 have reappeared in the 2007111505 build, perhaps due to the re-landing. See Bug 403743. 

Comment 25

[Stefan Breunig]

It doesn't seem like there is an about:config pref to easily get rid of that splitter. Adding 
#urlbar-search-splitter {
  display:none;
}
to userChrome.css is rather uncomfortable. Should I open a new bug for this?

Comment 26

[Elmar Ludwig]

(In reply to comment #25)
> It doesn't seem like there is an about:config pref to easily get rid of that
> splitter.[...] Should I open a new bug for this?

see bug 403732 - toolbar resizer cannot be removed

Comment 27

[ronin.achilles]

One of the points made above is quite valid, not sure as to why has that scenario not been taken care of. So for example, if a user places a few toolbar buttons between location and search bar, is there going to be no way at all for resizing them?

Comment 28

[Michael Ventnor]

This regressed the styling from bug 393718 so now we have the ugly thick line again. Someone needs to replace this:

+      splitter.className = "chromeclass-toolbar-additional";

with this:

+      splitter.className = "chromeclass-toolbar-additional toolbar-splitter";

Comment 29

[Reed Loden [:reed]]

(In reply to comment #28)
> This regressed the styling from bug 393718 so now we have the ugly thick line
> again.

See bug 403705.

Comment 30

[Michael Ventnor]

Mid air collision, I was just going to post that I just noticed that. Sorry :-)

Comment 31

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007120704 Minefield/3.0b2pre.   Also for linux and windows trunk builds also.  

Comment 32

[Stephen Donner [:stephend] Not actively reading bugmail]

https://litmus.mozilla.org/show_test.cgi?id=5147

in-litmus+