337427 - URLbar/searchbox border shifts slightly when icon appears in the URLbar

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Joe Hughes]

Because (as of bug 258679) the searchbox is now flexible, whenever the an icon appears or disappears on the right side of the the URLbar, the boundary between the URLbar and the searchbox shifts slightly.

Comment 1

[Mike Connor [:mconnor]]

That's really, uh, odd, but I've seen that from time to time.

Comment 2

[Joe Hughes]

Attachment: Patch that adds hidden spacer elements to remove shifting border.

Comment 3

[Joe Hughes]

Per offline discussion with Gavin, I'm going to try it with: (1) explicitly hidden spacing elements, (2) hbox instead of textbox, and (3) the same images that are used in the real urlbar

Comment 4

[Joe Hughes]

Attachment: Updated to use visibility:hidden for the spacer (the other two suggestions didn't pan out)

The visibility:hidden change works, but replacing the textbox with an hbox doesn't work (they have different flex characteristics).  I also didn't see a straightforward way to use the exact same icons and CSS spacing characteristics for the spacer, in a way that wouldn't add undue complexity and duplication to the code.

Comment 5

[Joe Hughes]

FWIW, I've tested this patch's functionality on Ubuntu/Gnome, Windows, and Mac OS X.  A good test case for this is to use GMail using an https:// URL--that way you get both the SSL and the RSS icons in the urlbar.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 230491 [details] [diff] [review]
Updated to use visibility:hidden for the spacer (the other two suggestions didn't pan out)

Hmm, this is a little bit risky, since it adds another node between the URL bar element and it's toolbar parent, which breaks code that assumes that gURLBar.parentNode.parentNode is the <toolbar> that contains the URL bar (see http://lxr.mozilla.org/seamonkey/search?string=gurlbar.parent ). I think those are the only places that do that in the tree (I checked for direct references to the textbox's childNodes, too), but it's possible that extensions may have made the same assumption, and it's possible that I may not have found all the cases where that happens. I think this should be OK, though perhaps it's best to not make this change on the branch at this point. I don't think that bug is that serious, anyways.

r=me if you fix the references to gURLBar.parentNode.parentNode identified in the LXR query above.

Comment 7

[Mike Connor [:mconnor]]

Pushing to the nom list.  I don't think this is at all serious enough to take on much risk for, but at the same time we're changing other pieces of the same toolbaritem in the visual refresh, so extension might break anyway.

Comment 8

[Joe Hughes]

Attachment: Fixes the relative accesses to the URLBar's parent toolbar by referring to the urlbar-container absolutely by ID

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 231121 [details] [diff] [review]
Fixes the relative accesses to the URLBar's parent toolbar by referring to the urlbar-container absolutely by ID

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

> function openLocation()
> {
>-  if (gURLBar && !gURLBar.parentNode.parentNode.collapsed &&
>-      !(window.getComputedStyle(gURLBar.parentNode, null).display == "none")) {
>+  if (gURLBar && !gURLBarContainer.parentNode.collapsed &&
>+      !(window.getComputedStyle(gURLBarContainer, null).display == "none")) {

While you're touching this, can you change the last condition to:
document.defaultView.getComputedStyle(gURLBarContainer, null).display != "none"

Comment 10

[Adam Guthrie]

*** Bug 346442 has been marked as a duplicate of this bug. ***

Comment 11

[Joe Hughes]

Attachment: Addressed gavin's minor comment

Comment 12

[:Gavin Sharp [email: [e-mail]]]

The main part of my comment was actually the s/window/document.defaultView/ part, but since other places already make the assumption that the global window object implements nsIDOMViewCSS, I guess it can be left for now.

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 231486 [details] [diff] [review]
Addressed gavin's minor comment

This is really ugly... if there isn't a bug on the core bug, please file one?  (I think this is bad behaviour, but we should take it to a core bug)

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

If it's going to be a hardcoded 100px, why go through all the trouble of making a <deck> (which is quite expensive, since it creates widgets).  Why not just add a style rule like:

#urlbar { min-width: 100px; }

?

Comment 15

[Mike Connor [:mconnor]]

Comment on attachment 231486 [details] [diff] [review]
Addressed gavin's minor comment

a=mconnor on behalf of drivers for checkin to 1.8.  This is a bit of a hack, there may be other cleaner ways to prevent this, but there should be a followup to find a cleaner way, or fix XUL to not need this type of hack.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Or, if you still want a min-width larger than 100px to apply in case something creates it somehow, create a stack with the urlbar and a 100px-wide spacer?  No deck, and no extra textbox?)

Comment 17

[Joe Hughes]

(In reply to comment #14)
> If it's going to be a hardcoded 100px, why go through all the trouble of making
> a <deck> (which is quite expensive, since it creates widgets).  Why not just
> add a style rule like:
> 
> #urlbar { min-width: 100px; }

It's not that the urlbar needs to be 100px; it's that the resizing of the status icon box within the urlbar's textbox throws off the flex calculations.  And I don't think we can just give the icon box a min width, because then it would prematurely truncate the textual content of the urlbar when no icons were displayed.  So this was the best solution I could find given the constraints (alternate ideas welcome!).

I agree with mconnor, the root cause is that the child nodes of the textbox cause the textbox to expand, which should really be addressed in XUL.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #17)
> I agree with mconnor, the root cause is that the child nodes of the textbox
> cause the textbox to expand, which should really be addressed in XUL.

Which is happening because the flex is starting from the min-width, which for XUL defaults to being intrinsic.  That's a good thing, but it's not what you want in this case, so you should override it.

Comment 19

[Joe Hughes]

(In reply to comment #18)
> (In reply to comment #17)
> > I agree with mconnor, the root cause is that the child nodes of the textbox
> > cause the textbox to expand, which should really be addressed in XUL.
> 
> Which is happening because the flex is starting from the min-width, which for
> XUL defaults to being intrinsic.  That's a good thing, but it's not what you
> want in this case, so you should override it.

I tried all of your suggestions above (min-width on urlbar, min-width on urlbar-icons, stack around urlbar-icons), but none produced the desired result.  Namely:
(1) size of urlbar not shifting when switching between SSL + RSS site and one with no icons
(2) text in urlbar not prematurely truncated when there's more whitespace available

I'm going to land this one as-is and hope that we can find a better solution (probably by fixing the underlying XUL problem) later.

Comment 20

[Joe Hughes]

landed on 1.8 branch

Comment 21

[Joe Hughes]

Attachment: Patch as submitted on branch & trunk

Comment 22

[Joe Hughes]

Attachment: Patch as submitted on branch & trunk (ignoring whitespace changes)

Comment 23

[Joe Hughes]

landed on trunk

Comment 24

[:Gavin Sharp [email: [e-mail]]]

*** Bug 344635 has been marked as a duplicate of this bug. ***