Last Comment Bug 677091 - Layout broken on preference dialog and find bar
: Layout broken on preference dialog and find bar
Status: VERIFIED FIXED
[qa!]
: regression, verified-aurora, verified-beta
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86 Windows XP
: -- normal with 8 votes (vote)
: mozilla9
Assigned To: Neil Deakin
:
: Neil Deakin
Mentors:
: 677108 677497 678172 678468 678491 678633 678764 678826 678841 679085 680387 680720 (view as bug list)
Depends on:
Blocks: 585069 677087 677886
  Show dependency treegraph
 
Reported: 2011-08-07 08:21 PDT by Alice0775 White
Modified: 2011-10-04 05:15 PDT (History)
51 users (show)
Ms2ger: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed


Attachments
screen shot (32.32 KB, image/png)
2011-08-07 08:21 PDT, Alice0775 White
no flags Details
testcase XUL (298 bytes, application/vnd.mozilla.xul+xml)
2011-08-07 10:01 PDT, Alice0775 White
no flags Details
possible patch (1.23 KB, patch)
2011-08-09 15:05 PDT, Neil Deakin
dbaron: feedback-
Details | Diff | Splinter Review
fix ComputeSizeWithIntrinsicDimensions (1.89 KB, patch)
2011-08-16 17:40 PDT, David Baron :dbaron: ⌚️UTC-10
roc: review+
asa: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
issue with findbar testcase (528 bytes, application/vnd.mozilla.xul+xml)
2011-08-17 06:08 PDT, Neil Deakin
no flags Details

Description Alice0775 White 2011-08-07 08:21:48 PDT
Created attachment 551321 [details]
screen shot

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/b422fd99fe0d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110807 Firefox/8.0a1 ID:20110807030727

Layout broken on preference dialog

Reproducible: Always

Steps to Reproduce:

1. Start browser
2. Alt > Tools > Options > Advanced tab or Tabs tab etc...
3.

Actual Results: 
  Layout broken

Expected Results: 
  Should not

Regression window(m-i hourly):
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/1c136ef5cac2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110805 Firefox/8.0a1 ID:20110805121955
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/3985e7570ab6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110805 Firefox/8.0a1 ID:20110805122919
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1c136ef5cac2&tochange=3985e7570ab6
Comment 1 Alice0775 White 2011-08-07 10:01:59 PDT
Created attachment 551324 [details]
testcase XUL
Comment 2 Wes Kocher (:KWierso) 2011-08-07 10:16:58 PDT
This also affects the find bar.
Comment 3 Girish Sharma [:Optimizer] 2011-08-07 11:54:52 PDT
Also the customize dialog box text "Use small icons " 
"icons' is coming in next line
Comment 4 Alice0775 White 2011-08-07 17:26:45 PDT
*** Bug 677108 has been marked as a duplicate of this bug. ***
Comment 5 Alice0775 White 2011-08-07 20:49:22 PDT
WORKAROUND:

.checkbox-check {
  width: 13px;
  height: 13px;
}
Comment 6 WildcatRay 2011-08-08 09:26:24 PDT
Would this bug also show on Shredder (Tb 8.0a1), too?
Comment 7 Dão Gottwald [:dao] 2011-08-08 10:30:52 PDT
(In reply to Ray Murphy (WildcatRay) from comment #6)
> Would this bug also show on Shredder (Tb 8.0a1), too?

yes
Comment 8 WildcatRay 2011-08-08 10:40:26 PDT
Thanks.
Comment 9 remixedcat 2011-08-08 14:14:54 PDT
the workaround worked thank you very much!!!! ;-)
Comment 10 Wes Kocher (:KWierso) 2011-08-08 23:29:21 PDT
Looks like bug 677109 was the Fennec version of this bug.
Comment 11 tymerkaev 2011-08-09 05:19:01 PDT
*** Bug 677497 has been marked as a duplicate of this bug. ***
Comment 12 Bruce A. Wittmeier 2011-08-09 11:27:53 PDT
This may also be affecting the OK button from dismissing the dialog box after changes are made.
Comment 13 WildcatRay 2011-08-09 11:59:17 PDT
(In reply to Bruce A. Wittmeier from comment #12)
> This may also be affecting the OK button from dismissing the dialog box
> after changes are made.

Have you tested this after applying the CSS code of Comment 5?

I just did a quick test using Options > Advanced > General > System Defaults and OK WFM.
Comment 14 Bruce A. Wittmeier 2011-08-09 12:08:20 PDT
Ray,

I just tested and the wrap is corrected with the .css added from comment #5.  But the OK button still does not dismiss the dialog box.  I think it should...but maybe not.
Comment 15 Neil Deakin 2011-08-09 15:05:04 PDT
Created attachment 551901 [details] [diff] [review]
possible patch

This is caused when a xul image has no specified width or height but does have a minimum width or height. This can happen if the image src hasn't been set yet.

<html:img> defaults to an intrinsic size of 300x150, however this default doesn't seem reasonable for xul images.

This patch fixes the issue but I don't understand the image computations well enough to know what the right fix might be.
Comment 16 Neil Deakin 2011-08-09 15:06:13 PDT
This should also fix bugs 677087 and 677161.
Comment 17 baffclan 2011-08-12 04:52:58 PDT
*** Bug 678172 has been marked as a duplicate of this bug. ***
Comment 18 Jim Mathies [:jimm] 2011-08-12 07:21:43 PDT
*** Bug 678491 has been marked as a duplicate of this bug. ***
Comment 19 Alice0775 White 2011-08-12 09:17:39 PDT
*** Bug 678468 has been marked as a duplicate of this bug. ***
Comment 20 Alice0775 White 2011-08-12 18:00:52 PDT
*** Bug 678633 has been marked as a duplicate of this bug. ***
Comment 21 Siddhartha Dugar [:sdrocking] 2011-08-12 21:13:28 PDT
Please make this a priority so that people stop filing duplicate bugs, and also because Aurora merge is close.
Comment 22 Alice0775 White 2011-08-13 18:43:06 PDT
*** Bug 678764 has been marked as a duplicate of this bug. ***
Comment 23 Alice0775 White 2011-08-14 07:29:38 PDT
*** Bug 678826 has been marked as a duplicate of this bug. ***
Comment 24 Thomas Ahlblom 2011-08-14 13:28:54 PDT
*** Bug 678841 has been marked as a duplicate of this bug. ***
Comment 25 Peter Henkel [:Terepin] 2011-08-14 13:33:28 PDT
Are you actually going to fix this, or you are waiting for another 20 duplicates?
Comment 26 Boris Zbarsky [:bz] (still a bit busy) 2011-08-14 13:35:34 PDT
Peter, the patch is waiting for review, no?
Comment 27 Peter Henkel [:Terepin] 2011-08-14 13:42:36 PDT
No, it is waiting for feedback. Four days! And during that time of period we had 7 duplicates. How many more we need to get this regression fixed?
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2011-08-14 19:52:23 PDT
> Four days!

This may have something to do with the fact that dbaron has a _lot_ of people asking him for feedback and review.  See https://bugzilla.mozilla.org/request.cgi?action=queue&type=all&requestee=dbaron%40dbaron.org (which is _not_ as long as it is because dbaron isn't doing his reviews).
Comment 29 Peter Henkel [:Terepin] 2011-08-15 11:23:19 PDT
There is only one dev that can provide feedback on this?
Comment 30 Thomas Ahlblom 2011-08-15 13:18:07 PDT
*** Bug 679085 has been marked as a duplicate of this bug. ***
Comment 31 Notlost 2011-08-15 13:25:00 PDT
oh, great fires of hell, yet another duplicate!
Comment 32 Notlost 2011-08-15 13:25:48 PDT
Cnt' we just set overflow=scroll here somewhere? A scrollbar would be a 'hotfix' for this issue that would stop the duplicates...
Comment 33 Michael Kraft [:morac] 2011-08-16 12:22:12 PDT
Note, this is also affecting add-ons that use checkbox's with labels.  The labels wrap which causes the window size calculations to be wrong (i.e. height of 26 instead of 13).   There are some workarounds such as giving checkbox containers a "flex" value, but that doesn't work in all instances.
Comment 34 David Baron :dbaron: ⌚️UTC-10 2011-08-16 17:37:49 PDT
(In reply to Neil Deakin from comment #15)
> This is caused when a xul image has no specified width or height but does
> have a minimum width or height.

By "minimum width or height", are you referring to 'min-width' and 'min-height'?

> This can happen if the image src hasn't been
> set yet.

Does it continue to cause problems after the src has been set?  If so, there's a deeper bug here.

> <html:img> defaults to an intrinsic size of 300x150, however this default
> doesn't seem reasonable for xul images.
> 
> This patch fixes the issue but I don't understand the image computations
> well enough to know what the right fix might be.

I don't see why the patch makes any sense.

So I'll presume based on the first paragraph of comment 15 that the case you're hitting is that you're calling nsLayoutUtils::ComputeAutoSizeWithIntrinsicDimensions() with minWidth and/or minHeight having useful values, maxWidth and maxHeight both being NS_UNCONSTRAINEDSIZE, and tentWidth and tentHeight being 0.

Given that, ComputeSizeWithIntrinsicDimensions should take the else of the if tentWidth>0 and likewise for the if tentHeight>0.  (In both cases, this means that the part of the spec saying "for replaced elements with an intrinsic ratio" doesn't really apply, so the spec doesn't really require that we use the table.)

Then, if both minWidth and minHeight are nonzero, it should then fall into the second part of the long if-else chain (tentWidth < minWidth), enter the first if inside that (tentHeight<minHeight), and then, given the previous paragraph produce width=tentWidth and height=minHeight, when in fact we wanted width=minWidth.

Similar problems occur when only minWidth is > 0 (and minHeight is 0), which leads into the second if in the big chain, and into the else inside that, which makes height=tentHeight (when it should be minHeight).  Likewise, when only minHeight is > 0 (and minWidth is 0), it leads into the third part of the big chain, and the second part within thait, which leads to width=tentWidth (instead of minWidth).


In other words, I'd think this change, based on what you describe, is working around a bug in ComputeSizeWithIntrinsicDimensions.  I'll attach the patch that I think would fix it, and then try testing it.
Comment 35 David Baron :dbaron: ⌚️UTC-10 2011-08-16 17:40:34 PDT
Created attachment 553642 [details] [diff] [review]
fix ComputeSizeWithIntrinsicDimensions
Comment 36 David Baron :dbaron: ⌚️UTC-10 2011-08-16 17:48:43 PDT
Hmmm, it actually looks like this is Windows-only.  That means I'll have to wait quite a few hours for a build...
Comment 37 David Baron :dbaron: ⌚️UTC-10 2011-08-16 17:55:40 PDT
Actually, try server is down right now, so I can't even do that.  I'll try tomorrow, unless somebody else wants to test it.
Comment 38 Matt Brubeck (:mbrubeck) 2011-08-16 17:58:39 PDT
(In reply to David Baron [:dbaron] from comment #36)
> Hmmm, it actually looks like this is Windows-only.  That means I'll have to
> wait quite a few hours for a build...

Bug 677109 seems to be the same bug on Android.  An Android build might be faster if you have the requisite build/test setup.
Comment 39 Neil Deakin 2011-08-16 18:11:00 PDT
(In reply to David Baron [:dbaron] from comment #34)
> So I'll presume based on the first paragraph of comment 15 that the case
> you're hitting is that you're calling
> nsLayoutUtils::ComputeAutoSizeWithIntrinsicDimensions() with minWidth and/or
> minHeight having useful values, maxWidth and maxHeight both being
> NS_UNCONSTRAINEDSIZE, and tentWidth and tentHeight being 0.
> 

Yes, this is the situation that is causing this bug. The underlying issue isn't Windows only, there are manifestations on Linux as well (Ctrl+F to show the find bar -- the previous/next buttons are cut off.)
Comment 40 David Baron :dbaron: ⌚️UTC-10 2011-08-16 21:02:44 PDT
(In reply to Matt Brubeck (:mbrubeck) from comment #38)
> Bug 677109 seems to be the same bug on Android.  An Android build might be
> faster if you have the requisite build/test setup.

Build setup, yes.  Test setup, not so much.

(In reply to Neil Deakin from comment #39)
> Yes, this is the situation that is causing this bug. The underlying issue
> isn't Windows only, there are manifestations on Linux as well (Ctrl+F to
> show the find bar -- the previous/next buttons are cut off.)

I don't see the previous/next problem on Linux.
Comment 41 Stanley Chan 2011-08-17 05:49:59 PDT
I can confirm the problem in Linux; however, I am not completely sure if its related or not.
At the moment, I cannot get a screenshot as I am at work now. Though, if you would still like to see it, I will see what I can do later tonight.

Can someone test the proposed patches and see if it fixes the issues?
Comment 42 Neil Deakin 2011-08-17 06:08:08 PDT
Created attachment 553742 [details]
issue with findbar testcase

I can confirm that the previous/next buttons issue is now fixed with this latest patch. Here is a simplified testcase for this specific aspect of the issue. It may not occur on some platforms due to it being dependent on the native theme rendering.
Comment 43 David Baron :dbaron: ⌚️UTC-10 2011-08-17 09:25:24 PDT
OK, I've tested that attachment 553642 [details] [diff] [review] does in fact fix the problems on the Windows preference dialog.  I'll try writing a reftest for the patch and posting it for review.


I'd also like to know which of the following is true and why:

 (1) there are images whose intrinsic size is permanently 0x0

 (2) there are images whose intrinsic size is initially 0x0 because they haven't loaded yet, and we don't reflow correctly when the images load
Comment 44 WildcatRay 2011-08-17 09:28:26 PDT
(In reply to David Baron [:dbaron] from comment #43)
> OK, I've tested that attachment 553642 [details] [diff] [review] does in
> fact fix the problems on the Windows preference dialog.  I'll try writing a
> reftest for the patch and posting it for review.
> 
> 
> I'd also like to know which of the following is true and why:
> 
>  (1) there are images whose intrinsic size is permanently 0x0
> 
>  (2) there are images whose intrinsic size is initially 0x0 because they
> haven't loaded yet, and we don't reflow correctly when the images load

Did you think to check the find bar (F3), too? The text "Match Case" shows on 2 lines.
Comment 45 WildcatRay 2011-08-17 09:29:43 PDT
Oh, and the Customize Toolbar dialog box. "Use Small Icons" there.
Comment 46 David Baron :dbaron: ⌚️UTC-10 2011-08-17 09:39:40 PDT
Nope; you're welcome to do so.
Comment 47 David Baron :dbaron: ⌚️UTC-10 2011-08-17 09:54:00 PDT
(In reply to David Baron [:dbaron] from comment #43)
> I'd also like to know which of the following is true and why:
> 
>  (1) there are images whose intrinsic size is permanently 0x0
> 
>  (2) there are images whose intrinsic size is initially 0x0 because they
> haven't loaded yet, and we don't reflow correctly when the images load

So in the case of a checkbox, it seems like we have a xul:image which has:
 * no actual image
 * a background-image
 * -moz-appearance: checkbox
 * min-height: 13px; min-width: 13px;
Comment 48 Boris Zbarsky [:bz] (still a bit busy) 2011-08-17 10:52:43 PDT
> (1) there are images whose intrinsic size is permanently 0x0

Yes.  See bug 333954 comment 13.  Does this patch change the behavior in those cases for the better?
Comment 49 Gary King 2011-08-17 12:28:48 PDT
[url=https://bugzilla.mozilla.org/attachment.cgi?id=551321]screen shot[/url]

Is this bug ever going to be fixed? It does not seem all that hard to implement.
Comment 50 David Baron :dbaron: ⌚️UTC-10 2011-08-17 13:54:53 PDT
(In reply to Boris Zbarsky (:bz) from comment #48)
> > (1) there are images whose intrinsic size is permanently 0x0
> 
> Yes.  See bug 333954 comment 13.  Does this patch change the behavior in
> those cases for the better?

Yes.  Though in this case the issue is xul:image, which appears to be used in a bunch of XBL bindings even though it doesn't need to be an image element at all.  (My guess is that the checkbox was once a foreground image, but then when native theme work was done, it was changed to a background image because that's overridden by native theme.)
Comment 51 David Baron :dbaron: ⌚️UTC-10 2011-08-17 14:34:14 PDT
Comment on attachment 553642 [details] [diff] [review]
fix ComputeSizeWithIntrinsicDimensions

I've been trying to write a non-platform-specific reftest for this (i.e., one not depending on -moz-appearance) but I've failed so far.
Comment 52 David Baron :dbaron: ⌚️UTC-10 2011-08-17 17:57:11 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/4789623b77f2
Comment 53 Marco Bonardo [::mak] 2011-08-18 03:57:53 PDT
http://hg.mozilla.org/mozilla-central/rev/4789623b77f2
Comment 54 Siddhartha Dugar [:sdrocking] 2011-08-18 05:26:04 PDT
Please land this for Aurora (Firefox 8) also.
Comment 55 David Baron :dbaron: ⌚️UTC-10 2011-08-18 08:40:07 PDT
Comment on attachment 553642 [details] [diff] [review]
fix ComputeSizeWithIntrinsicDimensions

We should either land this on aurora or back out bug 585069.  (I'm not sure if anything yet depends on bug 585069, but Neil would probably know.)

And it would be good to do that before we update all our aurora users to 8.0.
Comment 56 WildcatRay 2011-08-18 08:54:18 PDT
(In reply to David Baron [:dbaron] from comment #55)
> Comment on attachment 553642 [details] [diff] [review]
>
> And it would be good to do that before we update all our aurora users to 8.0.

Aurora is already 8.0a2.
Comment 57 Wes Kocher (:KWierso) 2011-08-18 08:56:26 PDT
(In reply to Ray Murphy (WildcatRay) from comment #56)
> (In reply to David Baron [:dbaron] from comment #55)
> > Comment on attachment 553642 [details] [diff] [review]
> >
> > And it would be good to do that before we update all our aurora users to 8.0.
> 
> Aurora is already 8.0a2.

But I didn't think an official Aurora update had been pushed out to the Aurora channel quite yet.
Comment 58 WildcatRay 2011-08-18 09:02:18 PDT
I forget if it was pushed into place, but starting with yesterday's Aurora nightly, it is 8.0a2.
Comment 59 Matt Brubeck (:mbrubeck) 2011-08-18 09:46:09 PDT
(In reply to Ray Murphy (WildcatRay) from comment #58)
> I forget if it was pushed into place, but starting with yesterday's Aurora
> nightly, it is 8.0a2.

Updates are temporarily disabled, and the download links at https://mozilla.com/firefox/channel/ still point to 7.0a2, so all users who get Aurora through normal channels are still on version 7.
Comment 60 WildcatRay 2011-08-18 09:54:43 PDT
(In reply to Matt Brubeck (:mbrubeck) from comment #59)
> (In reply to Ray Murphy (WildcatRay) from comment #58)
> > I forget if it was pushed into place, but starting with yesterday's Aurora
> > nightly, it is 8.0a2.
> 
> Updates are temporarily disabled, and the download links at
> https://mozilla.com/firefox/channel/ still point to 7.0a2, so all users who
> get Aurora through normal channels are still on version 7.

FYI: Aurora 8.0a2 builds are being made, and, for example, are available here: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2011-08-18-04-20-04-mozilla-aurora/
Comment 61 David Baron :dbaron: ⌚️UTC-10 2011-08-18 15:22:17 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/620e3bda721b
Comment 62 Mats Palmgren (:mats) 2011-08-19 10:01:54 PDT
*** Bug 680387 has been marked as a duplicate of this bug. ***
Comment 63 David Baron :dbaron: ⌚️UTC-10 2011-08-19 15:04:57 PDT
Added reftest:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e09d10ebeaa8
Comment 64 bogas04 2011-08-19 19:39:59 PDT
is it also related to the increased height find bar?
Comment 65 Wes Kocher (:KWierso) 2011-08-19 19:56:41 PDT
(In reply to bogas04 from comment #64)
> is it also related to the increased height find bar?

That's correct. It should be fixed in recent Nightly builds and the most recent Aurora build, though.
Comment 66 :Ms2ger (⌚ UTC+1/+2) 2011-08-21 11:38:05 PDT
http://hg.mozilla.org/mozilla-central/rev/e09d10ebeaa8
Comment 67 (mostly gone) XtC4UaLL [:xtc4uall] 2011-08-21 15:21:42 PDT
*** Bug 680720 has been marked as a duplicate of this bug. ***
Comment 68 neil@parkwaycc.co.uk 2011-08-22 03:18:21 PDT
(In reply to David Baron [:dbaron] from comment #50)
> Yes.  Though in this case the issue is xul:image, which appears to be used
> in a bunch of XBL bindings even though it doesn't need to be an image
> element at all.  (My guess is that the checkbox was once a foreground image,
> but then when native theme work was done, it was changed to a background
> image because that's overridden by native theme.)
Only in the default theme, of course. Other themes use it as an image.
Comment 69 Simona B [:simonab ] 2011-10-04 05:15:26 PDT
Verified on Windows XP using the latest Nighlty, Aurora and Firefox 8.0b1. There are no layout problems on preference dialog and Find bar.

Mozilla/5.0 (Windows NT 5.1; rv:10.0a1) Gecko/20111003 Firefox/10.0a1
Mozilla/5.0 (Windows NT 5.1; rv:9.0a2) Gecko/20111003 Firefox/9.0a2
Mozilla/5.0 (Windows NT 5.1; rv:8.0) Gecko/20100101 Firefox/8.0

Note You need to log in before you can comment on or make changes to this bug.