Secure connection text overflows location bar

RESOLVED FIXED in Firefox 16

Status

()

Firefox
Theme
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: mcomella, Assigned: roc)

Tracking

({regression, reproducible, testcase})

16 Branch
Firefox 18
regression, reproducible, testcase
Points:
---

Firefox Tracking Flags

(firefox16+ fixed, firefox17+ fixed, firefox18+ verified)

Details

Attachments

(8 attachments, 2 obsolete attachments)

Created attachment 657083 [details]
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.
Hardware: x86 → x86_64

Comment 1

5 years ago
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.
status-firefox18: affected → ---
tracking-firefox16: --- → ?
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?
Keywords: regression
Version: 18 Branch → 16 Branch

Comment 2

5 years ago
Created attachment 657242 [details]
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.
Side note: I filed bug 787525 which also has location bar overflow issues.
See Also: → bug 787525

Comment 4

5 years ago
(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.
tracking-firefox16: ? → +
tracking-firefox17: ? → +
tracking-firefox18: ? → +
Keywords: qawanted, regressionwindow-wanted, reproducible
(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

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

5 years ago
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?
Assignee: nobody → gavin.sharp
Bisecting on inbound gives the first bug in that range:

The first bad revision is: 
changeset: 97489:47bb129f9fbe 
user: Charly Molter <charly.molter@gmail.com> 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
Depends on: 716875
Keywords: qawanted, regressionwindow-wanted
Hmm I can have a look at that. Seems to be an overflow problem. I'm going to have a look
Duplicate of this bug: 787525
Blocks: 716875
No longer depends on: 716875
Assignee: gavin.sharp → charly.molter
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?
Michael, do you see the same thing?
Created attachment 658362 [details]
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.
Just to check, that's using what build?
I can also still see the overflow with the latest build:
http://hg.mozilla.org/mozilla-central/rev/a86b00fa6bc6
Build ID: 20120904030512
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
(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
(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.
(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...
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?
Keywords: qawanted
Mats, do you have the bandwidth for this?
(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.
Keywords: qawanted → testcase-wanted
However we want to label it; the important part is to make it happen...
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?
Created attachment 660620 [details] [diff] [review]
wallpaper?

Anyway, this seems to fix it in my local OSX build.
Something similar should work for the other themes.
Created attachment 660631 [details]
Testcase #1
Created attachment 660635 [details]
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 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.

Updated

5 years ago
Attachment #660620 - Attachment is obsolete: true
(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.
Keywords: testcase-wanted → testcase
Fixed on beta by backout.
status-firefox16: --- → fixed
status-firefox17: --- → affected
status-firefox18: --- → affected
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.
(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)
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?
Just to be clear - I'm not working on this bug, and I'm unlikely to do so
in the next 6 weeks.
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.
Created attachment 666826 [details]
reduced testcase

This testcase is pretty minimal at showing the difference between builds (beta vs trunk).
Created attachment 666835 [details]
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.
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).
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.
Created attachment 666854 [details] [diff] [review]
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.
Assignee: charly.molter → roc
Attachment #666854 - Flags: review?(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.
(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.
(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?
Created attachment 667240 [details] [diff] [review]
fix v2

Moves pinstripe searchbox min-width to searchbar.css.
Attachment #666854 - Attachment is obsolete: true
Attachment #666854 - Flags: review?(dao)
Attachment #667240 - Flags: review?(dao)
Comment on attachment 667240 [details] [diff] [review]
fix v2

Thanks!
Attachment #667240 - Flags: review?(dao) → review+

Updated

5 years ago
Component: Location Bar → Theme
OS: Mac OS X → All
Hardware: x86_64 → All
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4c95cde82e9

After this has landed and stuck, we should get it on FF17 as well.
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.
Attachment #667240 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/a4c95cde82e9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox18: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Comment on attachment 667240 [details] [diff] [review]
fix v2

approving for uplift, please get this landed before the monday oct 8th merge.
Attachment #667240 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/ea1910a8ffe4
status-firefox17: affected → fixed
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.
status-firefox18: fixed → verified
Keywords: verifyme
QA Contact: virgil.dicu
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.