The default bug view has changed. See this FAQ.

automatically add splitter whenever location bar and search bar are adjacent

VERIFIED FIXED in Firefox 3 beta2

Status

()

Firefox
Toolbars and Customization
P2
normal
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: beltzner, Assigned: Neil Deakin)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 3 beta2
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [has patch])

Attachments

(3 attachments)

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

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

Comment 2

10 years ago
Created attachment 285466 [details] [diff] [review]
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.
Attachment #285466 - Flags: review?(mano)
(Assignee)

Updated

10 years ago
Attachment #285466 - Flags: superreview?(neil)
Attachment #285466 - Flags: review?(neil)
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Flags: blocking-firefox3?

Comment 3

10 years ago
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.
(Assignee)

Comment 4

10 years ago
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

10 years ago
(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

10 years ago
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.
Attachment #285466 - Flags: superreview?(neil)
Attachment #285466 - Flags: superreview+
Attachment #285466 - Flags: review?(neil)
Attachment #285466 - Flags: review+

Comment 7

10 years ago
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

10 years ago
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

10 years ago
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.).
(Assignee)

Comment 10

10 years ago
Created attachment 286562 [details] [diff] [review]
address comments from Neil and Dão
Attachment #286562 - Flags: review?(mano)
(Assignee)

Updated

10 years ago
Attachment #285466 - Flags: review?(mano)

Updated

10 years ago
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 M10

Updated

10 years ago
Blocks: 393718
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.
Attachment #286562 - Flags: review?(mano) → review+

Updated

10 years ago
Priority: -- → P2
Whiteboard: [has patch][need final patch for landing]
(Assignee)

Updated

10 years ago
Attachment #286562 - Flags: approval1.9?
(Assignee)

Comment 12

10 years ago
Created attachment 288328 [details] [diff] [review]
address comments

Updated

10 years ago
Attachment #286562 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 13

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

Comment 14

10 years ago
(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?

Updated

10 years ago
Depends on: 403705

Comment 15

10 years ago
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.
Depends on: 403724
No longer depends on: 403724
Depends on: 403724
Could this have caused bug 403724? The tree is currently closed so it would be
great if someone could look into it asap.
This bug makes bookmarks toolbar items disappear (when search bar is in customize window).
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.
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

10 years ago
(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.

Updated

10 years ago
Whiteboard: [has patch][need final patch for landing] → [has patch]
Backed out to see if that fixes Ts regression in bug 403724
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
(In reply to comment #21)
> Backed out to see if that fixes Ts regression in bug 403724

Relanded at 2007-11-14 19:38.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Depends on: 403854
Depends on: 403732

Comment 24

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

Comment 25

10 years ago
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

10 years ago
(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

10 years ago
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?

Updated

10 years ago
Depends on: 403959

Comment 28

10 years ago
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";
(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

10 years ago
Mid air collision, I was just going to post that I just noticed that. Sorry :-)
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.  
Status: RESOLVED → VERIFIED
Flags: in-litmus?
https://litmus.mozilla.org/show_test.cgi?id=5147

in-litmus+
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.