640420 - Add draggable splitter between urlbar and searchbar

Status: RESOLVED FIXED

(SeaMonkey :: Search, enhancement)

Description

[Robert Kaiser]

Firefox has an invisible draggable splitter between the location and search bars, we should probably add this in SeaMonkey as well.

Comment 1

[Philip Chee]

I thought they got rid of it because it caused too many problems and side effects?

Comment 2

[Robert Kaiser]

(In reply to comment #1)
> I thought they got rid of it because it caused too many problems and side
> effects?

Well, on my Minefield build from today, it seems to work. It's not a separate customization element, though, no idea how they actually are doing it.

Comment 3

[Philip Chee]

> no idea how they actually are doing it.
Same concept that IanN used to make the urlbar groove box in modern include the Go and Search buttons.

<http://mxr.mozilla.org/comm-central/search?string=UpdateUrlbarSearchSplitterState>

Bug 400327 - automatically add splitter whenever location bar and search bar are adjacent
http://hg.mozilla.org/mozilla-central/rev/08159a575452

Bug 403854 - Removing searchbar or location bar breaks many things
http://hg.mozilla.org/mozilla-central/rev/bdfb2721d184

Also see:
Bug 393718 - Style searchbar splitter nicely
  Bug 418463 - now that the adjacent sibling selector is updated dynamically, remove the hack from bug 393718
  Bug 403196 - Cannot remove resizer in toolbar.
> It is a regression from Bug 393718. Before Bug 393718 it had 4 drag-sensitive
> pixels (the first 4, horizontally), now only one (the first pixel); if you try
> dragging the second pixel it drags the adjacent object away). So it *is*
> possible to drag the resizer to the customize window, but some padding would be
> nice.

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch

This enables the splitter whenever the location bar container and search bar container are directly adjacent.

We'll probably have to see whether this looks acceptable in all cases (from a visual POV). I checked Win 7, Kubuntu Oneiric and Modern.

Comment 5

[Stefan [:stefanh]]

Comment on attachment 574099 [details] [diff] [review]
patch

This looks good, except for one problem: If I position the search field to the left of the url bar, I don't get any drag option.

Comment 6

[Stefan [:stefanh]]

(In reply to Stefan [:stefanh] from comment #5)
> This looks good, except for one problem: If I position the search field to
> the left of the url bar, I don't get any drag option.

So, the reason why there's no splitter in this case is because the #window-controls hbox is between the #search-container and the #nav-bar-inner.

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Stefan [:stefanh] from comment #6)
> So, the reason why there's no splitter in this case is because the
> #window-controls hbox is between the #search-container and the
> #nav-bar-inner.

Either you have a very strange setup or this is Mac-specific (and then I wouldn't know how). For me (on Linux and Windows), #window-controls is the rightmost part on the location toolbar and cannot be between #search-container and #nav-bar. If I have the two directly next to each other, no matter in which order, I get the splitter in between them.

Can anyone else on CC here reproduce Stefan's problem with #window-control?


I found a different issue, though. Opening the Customize Toolbar sheet and simply closing it without doing anything currently toggles between the availability and absence of the splitter (provided that the condition for it to be there in the first place is met, see above). Carefully examining the patch reveals the problem: If the splitter is in place, the two text box containers are not directly adjacent anymore (d'oh) so ibefore stays null and consequently the splitter gets removed. This can easily be solved of course, e.g. like this:

if ((navbar.nextSibling == splitter && splitter.nextSibling == searchbar) ||
    (searchbar.nextSibling == splitter && splitter.nextSibling == navbar))
  return;

inside the "if (navbar && searchbar) {" check.

Since it's so easy I'll just leave this comment here for now instead of updating the patch. :-)

Comment 8

[Stefan [:stefanh]]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #7)
> (In reply to Stefan [:stefanh] from comment #6)
> > So, the reason why there's no splitter in this case is because the
> > #window-controls hbox is between the #search-container and the
> > #nav-bar-inner.
> 
> Either you have a very strange setup or this is Mac-specific (and then I
> wouldn't know how). For me (on Linux and Windows), #window-controls is the
> rightmost part on the location toolbar and cannot be between
> #search-container and #nav-bar. If I have the two directly next to each
> other, no matter in which order, I get the splitter in between them.

The only difference I can see is that we, on Mac, hide the close and minimize buttons in suite/browser/mac/platformNavigationBindings.xul.

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

Stefan, can you try and see whether you still have an issue with the change from comment 7 applied?

Karsten, can you check whether you can reproduce Stefan's issue on Mac, with and without the change from comment 7 in addition to the patch? [I'm currently at work so cannot provide an updated patch right now.]

Comment 10

[Stefan [:stefanh]]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #9)

> [I'm
> currently at work so cannot provide an updated patch right now.]

Me too ;-)

Comment 11

[Stefan [:stefanh]]

OK, so it appears that my test profile probably is broken... With a new profile, #window-controls is in the right place. Sorry, I should have checked that :-(

I'll take a another look at this in a few days.

Comment 12

[Stefan [:stefanh]]

Comment on attachment 574099 [details] [diff] [review]
patch

This is fine from a mac pow (with the issue in comment 7 addressed, of course), just forget about the #window-controls issue - I guess I somehow messed up my toolbar in one of my profiles.

Comment 13

[Ian Neal]

Patch with changes from comment 7 added:
1/ Customize toolbar
2/ Add Search Box to the right of url box
3/ Note Gap between url box and search box
4/ Close Customize box
5/ Gap increases

With search box next to url box:
1/ Customize toolbar
2/ Remove Search Box
3/ Note Gap between url box and next widget
4/ Close Customize box
5/ Gap decreases

Not sure how easy that is to fix though. Maybe the whole thing needs to be done more through css?

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

I cannot reproduce the issue from comment 13, neither on Linux nor Win7. With Modern of course there's a difference anyway because in customization mode the location and search boxes have separate borders.

Either way, do you really think this is an actual issue? Users cannot change the gap size anyway, and if they add a spacer in between, the dragability is gone.

Comment 15

[Ian Neal]

Comment on attachment 574099 [details] [diff] [review]
patch

As there are UI changes I would suggest getting a ui-review/superreview from Neil too.

Comment 16

[[e-mail]]

Comment on attachment 574099 [details] [diff] [review]
patch

>   // Do all UI building here:
>   UpdateNavBar();
>   updateWindowState();
> 
>+  UpdateUrlbarSearchSplitterState();
>+
Nit: Move before updateWindowState because it's related to UpdateNavBar().

>+  if (navbar && searchbar) {
This is always true under our toolbar customisation scheme.

> function BrowserToolboxCustomizeDone(aToolboxChanged)
> {
>   toolboxCustomizeDone("main-menubar", getNavToolbox(), aToolboxChanged);
> 
>   UpdateNavBar();
> 
>   // Update the urlbar
>+  UpdateUrlbarSearchSplitterState();
The problem here is that UpdateNavBar() gets confused by the splitter. In fact, I think you can completely confuse toolbar customisation by creating a custom toolbar, adding the urlbar and search bar, finishing customisation, then removing them again. The only way I can see to fix this is to remove the splitter in BrowserToolboxCustomizeInit(). (Also the call to update the splitter state doesn't belong after this comment.)

>+#urlbar-search-splitter {
>+  min-width: 8px;
>+  width: 8px;
>+  background-image: none;
>+  margin: 0 -4px;
[I don't think negative right margins are useful on splitters.]

>+  position: relative;
>+  height: 22px;
Is this necessary?

>+#urlbar-search-splitter {
>+  -moz-appearance: none;
>+  min-width: 6px;
>+  -moz-margin-start: -6px;
>+  border: none;
>+  background: transparent;
Don't need to change the appearance or background in Modern.

Comment 17

[Jens Hatlak (:InvisibleSmiley)]

Reality check: I don't think I'm going to get to this any time soon. Honestly, questions like "Is this necessary?" scare me off, since it means doing yet another round of adapting, checking Linux and Windows (7 only, don't have XP anymore), getting feedback for Mac etc. Maybe I should just turn this into an add-on, throw it onto my site, and be done with it... Don't get me wrong, I understand the requirements to not accept something for the default build that might regress something else. It's just that I feel I could fix a ton of other bugs instead of further giving this one a try. Sorry.

Comment 18

[Jens Hatlak (:InvisibleSmiley)]

xref Bug 585946 |Location / search bar splitter moves to the end of the toolbar when toggling "tabs on top"| (Neil was the reviewer so no news for him: support for new attribute "skipintoolbarset" was added to widgets/toolbar.xml and used/set for the splitter this bug is about)

Comment 19

[Philip Chee]

Attachment: New patch WIP without theme changes

This is a WIP patch fully functional but without the theme changes in attachment 574099 [details] [diff] [review] because I don't understand why those specific changes are needed.

Comment 20

[[e-mail]]

Comment on attachment 8484368 [details] [diff] [review]
New patch WIP without theme changes

This is code inspection only.

>+  // If the splitter is already in the right place, we don't need to do anything:
>+  if (splitter &&
>+      ((splitter.nextSibling == searchbar &&
>+        splitter.matches("#nav-bar-inner ~ #urlbar-search-splitter")) ||
>+       (splitter.nextSibling == urlbar &&
>+        splitter.matches("#search-container ~ #urlbar-search-splitter")))) {
>+    return;
>+  }
You don't need to special-case this here. In the worst case, just test that splitter.nextSibling != ibefore before inserting it.

>+      splitter.classList.add("chromeclass-toolbar-additional",
>+                             "nav-bar-class");
This looks like it will do the wrong thing if there is another toolbar item between the search and urlbar, I think you need to update the splitter before updating the nav-bar-class elements.

>+    urlbar.parentNode.insertBefore(splitter, ibefore);
I know they all have the same parent at this point, but it looks more readable with if you use ibefore.parentNode.

>+  UpdateUrlbarSearchSplitterState(true);
For what this does it's hardly worth trying to merge it with the code for inserting a splitter.

Comment 21

[Philip Chee]

Attachment: Patch v2

>>+  // If the splitter is already in the right place, we don't need to do anything:
>>+  if (splitter &&
>>+      ((splitter.nextSibling == searchbar &&
>>+        splitter.matches("#nav-bar-inner ~ #urlbar-search-splitter")) ||
>>+       (splitter.nextSibling == urlbar &&
>>+        splitter.matches("#search-container ~ #urlbar-search-splitter")))) {
>>+    return;
>>+  }

> You don't need to special-case this here. In the worst case, just test 
> that splitter.nextSibling != ibefore before inserting it.
Removed this chunk.

>>+      splitter.classList.add("chromeclass-toolbar-additional",
>>+                             "nav-bar-class");

> This looks like it will do the wrong thing if there is another toolbar 
> item between the search and urlbar, I think you need to update the 
> splitter before updating the nav-bar-class elements.
Now updates the splitter before updating the urlbar.

>>+    urlbar.parentNode.insertBefore(splitter, ibefore);

> I know they all have the same parent at this point, but it looks more 
> readable with if you use ibefore.parentNode.
Now using ibefore.parentNode.

>>+  UpdateUrlbarSearchSplitterState(true);

> For what this does it's hardly worth trying to merge it with the code for 
> inserting a splitter.
Moved this bit to BrowserToolboxCustomizeInit().

> +      splitter.classList.add("chromeclass-toolbar-additional",
Is "chromeclass-toolbar-additional" needed here?

Stefanh: is this needed on OSX?

> +++ b/suite/themes/classic/mac/navigator/navigator.css
> +#urlbar-search-splitter {
> +  width: 8px;
> +  margin: 0 -4px;
> +}

Comment 22

[[e-mail]]

Out of interest, why did you change ibefore = urlbar to ibefore = searchbar.nextSibling?

Comment 23

[Philip Chee]

(In reply to [e-mail] from comment #22)
> Out of interest, why did you change ibefore = urlbar to ibefore =
> searchbar.nextSibling?

I thought that putting the splitter immediately to the right or left of the searchbar container makes it more obvious that the searchbar can be resized

Comment 24

[Stefan [:stefanh]]

Comment on attachment 8488787 [details] [diff] [review]
Patch v2

(In reply to Philip Chee from comment #21) 
> Stefanh: is this needed on OSX?

(assuming the review request is only about this)
> 
> > +++ b/suite/themes/classic/mac/navigator/navigator.css
> > +#urlbar-search-splitter {
> > +  width: 8px;
> > +  margin: 0 -4px;
> > +}

Yes, and you also need all the other rules that was in attachment #574099 [details] [diff] [review]. So, nothing have changed :-) The "height: 22px;" is needed so we match the height of the urlbar and search field, the min-width is used to override the .nav-bar-class min-width, negative margins are needed to compensate for the left/right margins of the surrounding boxes, the "background-image: none;" rule is used for overriding the background-image in toolkit/themes/osx/global/splitter.css. Without the "position: relative;" rule the splitter cursor will only be visible on one side of the splitter area. So iotw, this is how it should look:

+#urlbar-search-splitter {
+  min-width: 8px;
+  width: 8px;
+  background-image: none;
+  margin: 0 -4px;
+  position: relative;
+  height: 22px;
+}

Comment 25

[[e-mail]]

Comment on attachment 8488787 [details] [diff] [review]
Patch v2

> 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;
>   padding: 0 !important;
>+}
>+
>+toolbar[mode="text"] > .nav-bar-class:not(.urlbar-search-splitter),
>+toolbar[iconsize="small"] > .nav-bar-class:not(.urlbar-search-splitter),
>+toolbar[mode="text"] > toolbarpaletteitem > .nav-bar-class,
>+toolbar[iconsize="small"] > toolbarpaletteitem > .nav-bar-class {
>   border: none !important;
> }
What's this change achieving?

Comment 26

[Philip Chee]

(In reply to [e-mail] from comment #25)
> Comment on attachment 8488787 [details] [diff] [review]
> Patch v2

> What's this change achieving?
Without this change the splitter becomes invisible in small icon (no labels) mode. I might be able to write another rule to override this, but this block of CSS has !important sprinkled all over.

Comment 27

[Stefan [:stefanh]]

(In reply to Philip Chee from comment #26)
> (In reply to [e-mail] from comment #25)
> > Comment on attachment 8488787 [details] [diff] [review]
> > Patch v2
> 
> > What's this change achieving?
> Without this change the splitter becomes invisible in small icon (no labels)
> mode. I might be able to write another rule to override this, but this block
> of CSS has !important sprinkled all over.

Shouldn't it be invisible (see comment #0)?

Comment 28

[Philip Chee]

(In reply to Stefan [:stefanh] from comment #27)
> (In reply to Philip Chee from comment #26)
> > (In reply to [e-mail] from comment #25)
> > > Comment on attachment 8488787 [details] [diff] [review]
> > > Patch v2
> > 
> > > What's this change achieving?
> > Without this change the splitter becomes invisible in small icon (no labels)
> > mode. I might be able to write another rule to override this, but this block
> > of CSS has !important sprinkled all over.
> 
> Shouldn't it be invisible (see comment #0)?

Should it be invisible on OS X? I can fix that in classic/mac if you want. In bugzilla I found some screenshots of Firefox on OSX and there appears to be a small nubbin vertically centred between the urlbar and the search bar.

On the other hand, an invisible splitter is essentially undiscoverable.

Comment 29

[Stefan [:stefanh]]

I thought the whole idea was to have an invisible splitter. Anyway, the style rules in comment #24 achieves that (as I explained).

Comment 30

[Philip Chee]

Attachment: Patch v3 with mac CSS fixed as per stefanh ui-review

Fixed OSX styles as per stefanh:

+#urlbar-search-splitter {
+  min-width: 8px;
+  width: 8px;
+  background-image: none;
+  margin: 0 -4px;
+  position: relative;
+  height: 22px;
+}

Comment 31

[[e-mail]]

(In reply to Philip Chee from comment #26)
> (In reply to from comment #25)
> > What's this change achieving?
> Without this change the splitter becomes invisible in small icon (no labels)
> mode. I might be able to write another rule to override this, but this block
> of CSS has !important sprinkled all over.
Aha, it wasn't obvious that you were splitting the existing rule into two.

Comment 32

[[e-mail]]

Comment on attachment 8492649 [details] [diff] [review]
Patch v3 with mac CSS fixed as per stefanh ui-review

>+      splitter.classList.add("chromeclass-toolbar-additional",
>+                             "nav-bar-class",
>+                             "urlbar-search-splitter");
Don't add urlbar-search-splitter to the class list, just use :not(#urlbar-search-splitter) or maybe :not(splitter) instead.

> 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;
>   padding: 0 !important;
>+}
>+
>+toolbar[mode="text"] > .nav-bar-class:not(.urlbar-search-splitter),
>+toolbar[iconsize="small"] > .nav-bar-class:not(.urlbar-search-splitter),
>+toolbar[mode="text"] > toolbarpaletteitem > .nav-bar-class,
>+toolbar[iconsize="small"] > toolbarpaletteitem > .nav-bar-class {
>   border: none !important;
> }
The problem here being that the splitter doesn't like having the nav-bar-class which is needed to make the three look good together, it's not just the top and bottom border that's affected, but also the left and right border, the padding and the minimum width, so those will all have to be special-cased.

Comment 33

[Philip Chee]

Attachment: Patch v4 a slightly different approach.

@stefanh: Please check that the Mac rules are correct. Thanks.

> The problem here being that the splitter doesn't like having the 
> nav-bar-class which is needed to make the three look good together, it's 
> not just the top and bottom border that's affected, but also the left and 
> right border, the padding and the minimum width, so those will all have 
> to be special-cased.

My current approach fixes all your issues (I hope) but in a different way.

0. This patch moves UpdateUrlbarSearchSplitterState() to the end of UpdateNavBar(). The splitter is inserted *after* UpdateNavBar() is done. The splitter also retains the nav-bar-class class. What this does is to allow UpdateNavBar() to do its work without needing to knowing about the seach bar splitter.

1. The splitter is always next to the searchbar container either on the right or left.

2. If the splitter is between two nav-bar-class elements it will look part of the navbar outline because of the nav-bar-class.

3. If the splitter is between the search container and a non nav-bar element then the search container has the nav-bar-end or nav-bar-start class and has rounded corners. The splitter with the nav-bar-class ends up as a singleton not connected to the search container outline. The nav-bar class makes the splitter look more like the search container without looking like part of it. Without this class the splitter looks ugly and out of place next to the search container.

I've tested with the search-container in various positions. Before and after the urlbar. Separated by non nav-bar-class elements, and sandwiched between two nav-bar-class elements.

+++ b/suite/themes/classic/navigator/navigator.css
+#urlbar-search-splitter {
+  margin: 5px 0px;
+}

Without this the splitter is stretched the whole vertical hight of the toolbar. With the top and bottom margins it looks about the same height as the other elements on the nav-bar.

+++ b/suite/themes/modern/navigator/navigator.css
+#search-container.nav-bar-last + #urlbar-search-splitter {
+  -moz-margin-start: 4px;
+}

This adds some visual space between the search container and the splitter. At least on my notebook.

Comment 34

[[e-mail]]

(In reply to Philip Chee from comment #33)
> 2. If the splitter is between two nav-bar-class elements it will look part
> of the navbar outline because of the nav-bar-class.
> 
> 3. If the splitter is between the search container and a non nav-bar element
> then the search container has the nav-bar-end or nav-bar-start class and has
> rounded corners. The splitter with the nav-bar-class ends up as a singleton
> not connected to the search container outline. The nav-bar class makes the
> splitter look more like the search container without looking like part of
> it. Without this class the splitter looks ugly and out of place next to the
> search container.

I'm afraid I disagree, the splitter just looks like a graphical glitch to me.

> +#search-container.nav-bar-last + #urlbar-search-splitter {
> +  -moz-margin-start: 4px;
> +}
> 
> This adds some visual space between the search container and the splitter.
> At least on my notebook.

Should be 5px though.

Comment 35

[Philip Chee]

Attachment: Patch v5 Use an invisible splitter

Avoid bikeshedding by using an invisible splitter. Any additonal visual tweaks should go into new bugs.

Comment 36

[[e-mail]]

Comment on attachment 8648756 [details] [diff] [review]
Patch v5 Use an invisible splitter

Trees have special splitter handling so you can hover a splitter that isn't there, but that doesn't work elsewhere, so setting left margin on a splitter doesn't do what you want. I'm going to assume here that you'll set the right margin (only) to the width of the splitter instead, but you might want to set the width of the splitter to the right margin (and remove the other margin).

>+  else if (splitter)
>+    splitter.remove();
When does this happen? There's no splitter at startup or during customisation.

>+#urlbar-search-splitter {
>+  min-width: 8px;
(Were you trying for an even number? 9px is default on Mac.)

>+  width: 8px;
No point setting the width too. (This applies to all themes.)

>+  position: relative;
???

>+  height: 22px;
??? (Haven't tried this on the Mac, it's busy with work.)

>+  background: transparent;
background-color

> 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;
>   padding: 0 !important;
>+}
>+
>+toolbar[mode="text"] > .nav-bar-class:not(splitter),
>+toolbar[iconsize="small"] > .nav-bar-class:not(splitter),
>+toolbar[mode="text"] > toolbarpaletteitem > .nav-bar-class,
>+toolbar[iconsize="small"] > toolbarpaletteitem > .nav-bar-class {
>   border: none !important;
> }
The splitter doesn't want a border anyway, so you don't need this.

>+#urlbar-search-splitter {
>+  min-width: 6px;
(And 5px is default for Modern.)

>+  background: transparent;
background-color

Comment 37

[Philip Chee]

Why does Firefox do it this way?
Linux:
942 #urlbar-search-splitter {
943   -moz-appearance: none;
944   width: 8px;

OSX:
1766 #urlbar-search-splitter {
1767   min-width: 8px;
1768   width: 8px;
1769   background-image: none;
1770   margin: 0 -4px;
1771   position: relative;
1772   height: 22px;

Windows:
1425 #urlbar-search-splitter {
1426   min-width: 6px;
1427   -moz-margin-start: -3px;
1428   border: none;
1429   background: transparent;

Comment 38

[[e-mail]]

(In reply to Philip Chee from comment #37)
> Why does Firefox do it this way?
> Linux:
> 942 #urlbar-search-splitter {
> 943   -moz-appearance: none;
Good point: I hadn't looked at Linux, and it uses native appearance for splitters, which I assume you'll want to turn off. The other Firefox styles that I've already commented on make no or bad sense.

Comment 39

[Philip Chee]

Attachment: Patch v6 more nits fixed.

> Trees have special splitter handling so you can hover a splitter that 
> isn't there, but that doesn't work elsewhere, so setting left margin on a 
> splitter doesn't do what you want. I'm going to assume here that you'll 
I don't know about trees, but my code is definitely working in that hovering over the splitter works.

> set the right margin (only) to the width of the splitter instead, but you 
> might want to set the width of the splitter to the right margin (and 
> remove the other margin).
When I do that the splitter is ungrabbable. Perhaps I misunderstand you. See attached patch.

>+  else if (splitter)
>+    splitter.remove();
> When does this happen? There's no splitter at startup or during
> customisation.
Removed.

>+#urlbar-search-splitter {
>+  min-width: 8px;
> (Were you trying for an even number? 9px is default on Mac.)
Stefanh wants this.

>+  width: 8px;
> No point setting the width too. (This applies to all themes.)
Removed.

>+  position: relative;
> ???
Stefanh wants this.

>+  height: 22px;
> ??? (Haven't tried this on the Mac, it's busy with work.)
Stefanh wants this.

classic/mac:
> +#urlbar-search-splitter {
> +  min-width: 8px;
> +  margin: 0 -4px;
> +  background-image: none;
> +  position: relative;
> +  height: 22px;

I removed the width but left the rest unchanged as Stefan says they are all needed. See comment 24.

>+  background: transparent;
> background-color
Fixed.

> 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;
>   padding: 0 !important;
>+}
>+
>+toolbar[mode="text"] > .nav-bar-class:not(splitter),
>+toolbar[iconsize="small"] > .nav-bar-class:not(splitter),
>+toolbar[mode="text"] > toolbarpaletteitem > .nav-bar-class,
>+toolbar[iconsize="small"] > toolbarpaletteitem > .nav-bar-class {
>   border: none !important;
> }
> The splitter doesn't want a border anyway, so you don't need this.
Fixed.

>+#urlbar-search-splitter {
>+  min-width: 6px;
> (And 5px is default for Modern.)
Fixed.

>+  background: transparent;
> background-color
Fixed.

Comment 40

[Stefan [:stefanh]]

(In reply to Philip Chee from comment #39)
> Created attachment 8650086 [details] [diff] [review]
> Patch v6 more nits fixed.
> 
> > Trees have special splitter handling so you can hover a splitter that 
> > isn't there, but that doesn't work elsewhere, so setting left margin on a 
> > splitter doesn't do what you want. I'm going to assume here that you'll 
> I don't know about trees, but my code is definitely working in that hovering
> over the splitter works.
> 
> > set the right margin (only) to the width of the splitter instead, but you 
> > might want to set the width of the splitter to the right margin (and 
> > remove the other margin).
> When I do that the splitter is ungrabbable. Perhaps I misunderstand you. See
> attached patch.
> 
> >+  else if (splitter)
> >+    splitter.remove();
> > When does this happen? There's no splitter at startup or during
> > customisation.
> Removed.
> 
> >+#urlbar-search-splitter {
> >+  min-width: 8px;
> > (Were you trying for an even number? 9px is default on Mac.)
> Stefanh wants this.
> 
> >+  width: 8px;
> > No point setting the width too. (This applies to all themes.)
> Removed.
> 
> >+  position: relative;
> > ???
> Stefanh wants this.
> 
> >+  height: 22px;
> > ??? (Haven't tried this on the Mac, it's busy with work.)
> Stefanh wants this.
> 
> classic/mac:
> > +#urlbar-search-splitter {
> > +  min-width: 8px;
> > +  margin: 0 -4px;
> > +  background-image: none;
> > +  position: relative;
> > +  height: 22px;
> 
> I removed the width but left the rest unchanged as Stefan says they are all
> needed. See comment 24.
> 
Iirc (on a mobile device atm) we want the splitter to be 8px wide here and I wonder if it is that now when the width rule is removed.

Comment 41

[[e-mail]]

(In reply to Philip Chee from comment #39)
> > set the right margin (only) to the width of the splitter instead, but you 
> > might want to set the width of the splitter to the right margin (and 
> > remove the other margin).
> When I do that the splitter is ungrabbable. Perhaps I misunderstand you. See
> attached patch.
I probably got my left and right mixed up, not that it matters (see below).

> >+  else if (splitter)
> >+    splitter.remove();
> > When does this happen? There's no splitter at startup or during
> > customisation.
> Removed.
Come to thing of it, the if (!splitter) check is always true...

> >+  background: transparent;
> > background-color
> Fixed.
Linux still needs the appearance turned off.

> >+  position: relative;
> > ???
> Stefanh wants this.
> 
> >+  height: 22px;
> > ??? (Haven't tried this on the Mac, it's busy with work.)
> Stefanh wants this.
> 
> classic/mac:
> > +#urlbar-search-splitter {
> > +  min-width: 8px;
> > +  margin: 0 -4px;
> > +  background-image: none;
> > +  position: relative;
> > +  height: 22px;
> I removed the width but left the rest unchanged as Stefan says they are all
> needed. See comment 24.
Ah, so the position: relative; is a hack to work around the lack of hover. We can remove the hack but then we can only set the left margin... I guess the position: relative hack is the lesser of the two evils, and then you can go back to your previous widths and margins. Sorry for the confusion.

(In reply to Stefan from comment #40)
> Iirc (on a mobile device atm) we want the splitter to be 8px wide here and I
> wonder if it is that now when the width rule is removed.

(Don't iDevices have caps lock?) Don't forget the min-width is still there.
(Why 8px? Splitters on OSX are normally 9px. Although we're going back anyway.)

Comment 42

[Stefan [:stefanh]]

(In reply to [e-mail] from comment #41)
> (In reply to Stefan from comment #40)
> > Iirc (on a mobile device atm) we want the splitter to be 8px wide here and I
> > wonder if it is that now when the width rule is removed.
> 
> (Don't iDevices have caps lock?) Don't forget the min-width is still there.
> (Why 8px? Splitters on OSX are normally 9px. Although we're going back
> anyway.)

(I wasn't really refering to the small caps usage. And I was on an Android device :P ). 8px matches the hover and while it's true that the splitter in toolkit/themes/osx/global/splitter.css is supposed to be 9px it's not really common in mac apps nowadays. To quote the Apple HIG: "The standard splitter is known as a “zero-width” splitter, although it is actually 1 point wide. There is also a wider splitter available, which measures 9 points in width, but it is not frequently used" (the zero-width splitter has "hot zone" 2points left/right of the splitter). Remember also that this is an invisible splitter (I guess we could call it a real zero-width splitter with the whole area between urlbar and searchbar indicating draggability), so it's not a standard splitter.

Comment 43

[Philip Chee]

Attachment: Patch v7 hopefully final fixes

>+  else if (splitter)
>+    splitter.remove();
> When does this happen? There's no splitter at startup or during
> customisation.
> Removed.
> Come to thing of it, the if (!splitter) check is always true...
Remove if

> Linux still needs the appearance turned off.
Computed styles says that they are already all -moz-appearance: none: but I've added it to both classic and modern.

> Ah, so the position: relative; is a hack to work around the lack of hover.
> We can remove the hack but then we can only set the left margin... I guess
> the position: relative hack is the lesser of the two evils, and then you can
> go back to your previous widths and margins. Sorry for the confusion.
Reverted including width. I think this may not be needed.

Comment 44

[[e-mail]]

(In reply to Philip Chee from comment #43)
> > Linux still needs the appearance turned off.
> Computed styles says that they are already all -moz-appearance: none:
Well it's right there in line 14 of mozilla/toolkit/themes/linux/global/splitter.css
> but I've added it to both classic and modern.
Should have been just Classic.

> > Ah, so the position: relative; is a hack to work around the lack of hover.
> > We can remove the hack but then we can only set the left margin... I guess
> > the position: relative hack is the lesser of the two evils, and then you can
> > go back to your previous widths and margins. Sorry for the confusion.
> Reverted including width. I think this may not be needed.
You're right, I was too vague, and I only meant min-widths.

Comment 45

[[e-mail]]

Comment on attachment 8654900 [details] [diff] [review]
Patch v7 hopefully final fixes

>+  width: 8px;
>+  width: 6px;
>+  -moz-appearance: none;
>+  width: 6px;
r=me with the above 6 lines removed as per comment #44.

Comment 46

[Rainer Bielefeld]

Works fine with  SeaMonkey 2.42a1 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0 from public download area)  Gecko/20100101  Firefox/ 45.0  Build 20151111021032, (Classic Theme) on German WIN7 64bit, no problems found with various tests.