787274 - Secure connection text overflows location bar

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Screenshot of the issue

1) Open Firefox
2) Right-click the Chrome and select "Customize..."
3) Move the location bar to the tabs menu
4) Close the customization mode.
5) Open enough tabs so that the tab bar needs to scroll. This will cause the location bar to shrink.
6) Open https://mozilla.org

Expected: The URL bar expands to showcase the secure connection text
Actual: The secure connection text overflows the URL bar and writes over other elements in the chrome.

See attached screenshot.

Comment 1

[Loic]

It's a regression since Firefox 16.

m-c
good=2012-06-23
bad=2012-06-24
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bb4b37094b9f&tochange=cb2904476d14

My guess goes to:
Stephen Horlander — Bug 766985 - Backout Mixed State Icon. r=dao

It looks like that's the only bug about location bar/theme but maybe another underlying bug is responsible for this current regression... Bisecting needed.

Comment 2

[Loic]

Attachment: screenshot during moving the location bar

On this screenshot, during moving the location bar, we can see the location bar is unlimited on the right.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Side note: I filed bug 787525 which also has location bar overflow issues.

Comment 4

[Alex Keybl [:akeybl]]

(In reply to Loic from comment #1)
> My guess goes to:
> Stephen Horlander — Bug 766985 - Backout Mixed State Icon. r=dao

Anthony - are you all able to bisect further than Comment 1 into the changes? If not, testing FF14/15 before and after bug 766985 landed should tell us whether bug 766985 is the cause.

Comment 5

[u279076]

(In reply to Alex Keybl [:akeybl] from comment #4)
> (In reply to Loic from comment #1)
> > My guess goes to:
> > Stephen Horlander — Bug 766985 - Backout Mixed State Icon. r=dao
> 
> Anthony - are you all able to bisect further than Comment 1 into the
> changes? If not, testing FF14/15 before and after bug 766985 landed should
> tell us whether bug 766985 is the cause.

I'm not familiar with bisecting down to less then 24 hours. I'm open to learn if someone is willing to teach me. That said here is my Firefox 14/15 comparison based on a landing date of 2012-06-22:

Firefox 14.0 Beta 2012-06-21: does not reproduce
Firefox 14.0 Beta 2012-06-22: does not reproduce
Firefox 14.0 Beta 2012-06-23: does not reproduce
Firefox 14.0 Beta 2012-06-24: does not reproduce

Firefox 15.0 Aurora 2012-06-21: does not reproduce
Firefox 15.0 Aurora 2012-06-22: does not reproduce
Firefox 15.0 Aurora 2012-06-23: does not reproduce
Firefox 15.0 Aurora 2012-06-24: does not reproduce

...and just to confirm I can reproduce this with Firefox 16.0 Nightly 2012-06-24.

This would seem to indicate bug 766985 is NOT the cause of this regression.

Comment 6

[Loic]

The regression range for mozilla-inbound:

m-i
good=2012-06-23
bad=2012-06-24
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=daf9c42be518&tochange=e23e10a4638b

In fact the changeset for bug 766985 is not present in this changelog. So it's not the culprit.

Comment 7

[Alex Keybl [:akeybl]]

Hey Gavin, I think we've gotten as far as we can with this bug without developer attention. Do you know who would be a good assignee?

Comment 8

[Virgil Dicu [:virgil] [QA]]

Bisecting on inbound gives the first bug in that range:

The first bad revision is: 
changeset: 97489:47bb129f9fbe 
user: Charly Molter <[e-mail]> and Alexandre Dumont <Alexandre Dumont > 
date: Sat Jun 23 13:11:00 2012 +0200 
summary: Bug 716875 - Make nsTextControlFrame inherits from nsContainerFrame instead of nsStackFrame. r=roc,bz

Comment 9

[Charly Molter :lahabana]

Hmm I can have a look at that. Seems to be an overflow problem. I'm going to have a look

Comment 11

[Charly Molter :lahabana]

I've checked and it doesn't seem to have the bug anymore on Nightly. 
I think the fix for https://bugzilla.mozilla.org/show_bug.cgi?id=782660 has also fixed that. I was because the overflow was never invalidated.
What should I do in that case? Close the bug?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Michael, do you see the same thing?

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Screenshot of the issue 9/4

(In reply to Boris Zbarsky (:bz) from comment #12)
> Michael, do you see the same thing?

I still see the issue on OS X.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Just to check, that's using what build?

Comment 15

[Virgil Dicu [:virgil] [QA]]

I can also still see the overflow with the latest build:
http://hg.mozilla.org/mozilla-central/rev/a86b00fa6bc6
Build ID: 20120904030512

Comment 16

[Charly Molter :lahabana]

Ho ok I see it now I didn't get the whole bug sorry. So it seems to come only when the secure text is there. I would say it might be like the nsTextControlFrames' minSize that is not properly fixed. but I'm not really sure. 
I'm going to experiment that

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Boris Zbarsky (:bz) from comment #14)
> Just to check, that's using what build?

While I'm not sure how to find the Build ID number...
From about:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:18.0) Gecko/18.0 Firefox/18.0

And from about:buildconfig
Built from http://hg.mozilla.org/mozilla-central/rev/a86b00fa6bc6

Comment 18

[Alex Keybl [:akeybl]]

(In reply to Charly Molter :lahabana from comment #16)
> Ho ok I see it now I didn't get the whole bug sorry. So it seems to come
> only when the secure text is there. I would say it might be like the
> nsTextControlFrames' minSize that is not properly fixed. but I'm not really
> sure. 
> I'm going to experiment that

Any updates? We're getting close to our final FF16 builds, so if we're going to fix, it has to happen soon. Hoping for a simple low risk fix here before Tuesday.

Comment 19

[Charly Molter :lahabana]

(In reply to Alex Keybl [:akeybl] from comment #18)
> (In reply to Charly Molter :lahabana from comment #16)
> > Ho ok I see it now I didn't get the whole bug sorry. So it seems to come
> > only when the secure text is there. I would say it might be like the
> > nsTextControlFrames' minSize that is not properly fixed. but I'm not really
> > sure. 
> > I'm going to experiment that
> 
> Any updates? We're getting close to our final FF16 builds, so if we're going
> to fix, it has to happen soon. Hoping for a simple low risk fix here before
> Tuesday.

I would be glad to fix it but I don't have much clue on what causes it. If anybody could point me toward something...

Comment 20

[Boris Zbarsky [:bzbarsky]]

Alex, what we need here is for someone to try to create a smaller XUL testcase than "the entire Firefox UI" that shows the problem.   That would go a long way toward making this tractable.

I can try to look into this on Monday even without a smaller testcase, if needed, but not before then.  So if we really want a reviewed low-risk fix before Tuesday we really need someone tasked with looking into this, ASAP.  Can we scrounge someone up?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Mats, do you have the bandwidth for this?

Comment 22

[u279076]

(In reply to Boris Zbarsky (:bz) from comment #20)
> Alex, what we need here is for someone to try to create a smaller XUL
> testcase than "the entire Firefox UI" that shows the problem.

I think testcase-wanted is more appropriate than qawanted in this case; unless you are specifically asking someone in QA to scrounge up this testcase.

Comment 23

[Boris Zbarsky [:bzbarsky]]

However we want to label it; the important part is to make it happen...

Comment 24

[Mats Palmgren (inactive)]

Yeah, it's hard to say if this is a layout bug or not without a simplified
testcase.  The content doesn't fit the #ulrbar container which has width:7em,
so it overflows.  I'm guessing we lost some "grow to fit the child overflow"
feature when removing nsStackFrame base class?

Comment 25

[Mats Palmgren (inactive)]

Attachment: wallpaper?

Anyway, this seems to fix it in my local OSX build.
Something similar should work for the other themes.

Comment 26

[Mats Palmgren (inactive)]

Attachment: Testcase #1

Comment 27

[Mats Palmgren (inactive)]

Attachment: Testcase #2

It "works" without a specified min-width (or min-width:0).
Otherwise, the specified value is used, even if it's less than the
shrink-wrap width.

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 660620 [details] [diff] [review]
wallpaper?

This doesn't look right to me at first glance. The content width varies depending on the secure connection label, which would cause the location bar to change its width.

Comment 29

[Mats Palmgren (inactive)]

(In reply to Boris Zbarsky (:bz) from comment #21)
> Mats, do you have the bandwidth for this?

No, sorry, after investigating it some more my feeling is that this may
need major work.  I think we should backout.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Fixed on beta by backout.

Comment 31

[Mats Palmgren (inactive)]

http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsSprocketLayout.cpp#169
is probably the first place I would investigate.  Just sprinkle it with
printf's of the various widths it calculates for the children it lays out.
Then run it on Testcase #1, and compare the results before/after our changes.

Comment 32

[Mats Palmgren (inactive)]

(At least that's the Layout method I think is used for the container frame
of our TextControlFrame in that test.  I may be wrong and it's one of the
other Layout methods under layout/xul/ though)

Comment 33

[Lukas Blakk [:lsblakk] use ?needinfo]

We're just over a week away from merging 17 to Beta, Mats do you have time for this in the next 6 weeks or are we looking at another backout?

Comment 34

[Mats Palmgren (inactive)]

Just to be clear - I'm not working on this bug, and I'm unlikely to do so
in the next 6 weeks.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not convinced testcase #2 shows a real bug. Our XUL box layout code lets min-width > 0 override the intrinsic width so the element can become arbitrarily small. That may or may not be desirable but since there's no spec, we can't say it's wrong ... and changing the behavior may break stuff.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: reduced testcase

This testcase is pretty minimal at showing the difference between builds (beta vs trunk).

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: various element types

Oddly, on beta the only kind of element I can find that has this effect (forcing the container to grow beyond its specified min-width) is text <input>s. Other kinds of HTML and XUL elements don't do it, including <stack> which text inputs were based on.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The reason this happens on beta is just a bug, I think. nsSprocketLayout::Layout on the min-width:100px container is working OK until we take the path "if (!newChildRect.IsEqualInterior(childRect))". This is true for the <input> child, for reasons which I haven't delved into but are presumably because it's a <stack> and stacks do that. Then we reach
// Now that a child resized, it's entirely possible that OUR rect is too small.  Now we
// ensure that |originalClientRect| is grown to accommodate the size of |clientRect|.
And we set originalClientRect.width (which was taking into account min-width and max-width) to clientRect (which does not). Then later we reset this frame's bounds to originalClientRect, again ignoring min-width and max-width. So basically, whenever a child changes size during its Layout() (which doesn't normally happen --- layout is supposed to be strictly outside-in), min-width and max-width are ignored (for horizontal boxes).

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So I think the correct thing to do here is to modify the chrome XUL in some way so it doesn't depend on this bug.

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Dao, how about this?

This ensures that non-flexible items in the URL bar, such as the identity box, add their width to the min-width of the container.

Comment 41

[Dão Gottwald [:dao]]

Comment on attachment 666854 [details] [diff] [review]
fix

>Bug 787274. Move urlbar min-width from #urlbar to the text input itself to ensure gadgets like the identity box are allowed to add to the min-width. r=dao

So, when you go from a site without an identity label to one with an identity label, will this cause the location bar to consume more width in a setup like attachment 657083 [details]? I think we want the width to be stable.

>--- a/browser/themes/pinstripe/browser.css
>+++ b/browser/themes/pinstripe/browser.css

>+.searchbar-textbox {
>+  min-width: 7em;
>+}

This should be moved to browser/themes/pinstripe/searchbar.css and probably be changed to 6em for consistency with winstripe and gnomestripe.

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Dão Gottwald [:dao] from comment #41)
> So, when you go from a site without an identity label to one with an
> identity label, will this cause the location bar to consume more width in a
> setup like attachment 657083 [details]?

Yes.

> I think we want the width to be stable.

OK, but behavior on beta/release is that it's not stable. All I was aiming to do here in this bug was to fix the regression by getting back (close to) the beta/release behavior.

If you really want me to change the behavior I can do that, if it's not too hard. But what should the behavior be? If we make the position of the location bar stable I think that has to mean when we browse to a site with an identity label, the identity label will be clipped and you won't be able to see the rest of the URL bar <textbox> (let alone click on it). Is that really what you want?

> >--- a/browser/themes/pinstripe/browser.css
> >+++ b/browser/themes/pinstripe/browser.css
> 
> >+.searchbar-textbox {
> >+  min-width: 7em;
> >+}
> 
> This should be moved to browser/themes/pinstripe/searchbar.css and probably
> be changed to 6em for consistency with winstripe and gnomestripe.

OK.

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #42)
> OK, but behavior on beta/release is that it's not stable. All I was aiming
> to do here in this bug was to fix the regression by getting back (close to)
> the beta/release behavior.
> 
> If you really want me to change the behavior I can do that, if it's not too
> hard. But what should the behavior be? If we make the position of the
> location bar stable I think that has to mean when we browse to a site with
> an identity label, the identity label will be clipped and you won't be able
> to see the rest of the URL bar <textbox> (let alone click on it). Is that
> really what you want?

Maybe it would make sense to take this patch to fix the regression (since we want to fix the regression on Aurora/soon-to-be-Beta) and file a new bug to change the behavior to make the location bar position stable? And let that change ride the trains normally?

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Moves pinstripe searchbox min-width to searchbar.css.

Comment 45

[Dão Gottwald [:dao]]

Comment on attachment 667240 [details] [diff] [review]
fix v2

Thanks!

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4c95cde82e9

After this has landed and stuck, we should get it on FF17 as well.

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 667240 [details] [diff] [review]
fix v2

Review of attachment 667240 [details] [diff] [review]:
-----------------------------------------------------------------

Fixes a regression in the Firefox UI that's present on Aurora. Chrome style change only, very limited impact.

Comment 48

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a4c95cde82e9

Comment 49

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 667240 [details] [diff] [review]
fix v2

approving for uplift, please get this landed before the monday oct 8th merge.

Comment 50

[Dão Gottwald [:dao]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ea1910a8ffe4

Comment 51

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/20100101 Firefox/18.0

Verified in 18 beta 3 - Ubuntu 12.04, Mac OS 10.8, Windows 7.

No overflow with the location bar. Scrolling tabs, moving tabs through the tab strip works as expected with the location bar moved next to the tab strip. Drop down results are displayed as expected - no overflow when selecting any of them.

Comment 52

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old