677091 - Layout broken on preference dialog and find bar

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Alice0775 White]

Attachment: 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]

Attachment: testcase XUL

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This also affects the find bar.

Comment 3

[Girish Sharma [:Optimizer]]

Also the customize dialog box text "Use small icons " 
"icons' is coming in next line

Comment 5

[Alice0775 White]

WORKAROUND:

.checkbox-check {
  width: 13px;
  height: 13px;
}

Comment 6

[WildcatRay]

Would this bug also show on Shredder (Tb 8.0a1), too?

Comment 7

[Dão Gottwald [:dao]]

(In reply to Ray Murphy (WildcatRay) from comment #6)
> Would this bug also show on Shredder (Tb 8.0a1), too?

yes

Comment 8

[WildcatRay]

Thanks.

Comment 9

[remixedcat]

the workaround worked thank you very much!!!! ;-)

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Looks like bug 677109 was the Fennec version of this bug.

Comment 12

[Bruce A. Wittmeier]

This may also be affecting the OK button from dismissing the dialog box after changes are made.

Comment 13

[WildcatRay]

(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]

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]

Attachment: 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]

This should also fix bugs 677087 and 677161.

Comment 21

[Siddhartha Dugar [:sdrocking]]

Please make this a priority so that people stop filing duplicate bugs, and also because Aurora merge is close.

Comment 25

[Peter Henkel [:Terepin]]

Are you actually going to fix this, or you are waiting for another 20 duplicates?

Comment 26

[Boris Zbarsky [:bzbarsky]]

Peter, the patch is waiting for review, no?

Comment 27

[Peter Henkel [:Terepin]]

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 [:bzbarsky]]

> 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]]

There is only one dev that can provide feedback on this?

Comment 31

[Notlost]

oh, great fires of hell, yet another duplicate!

Comment 32

[Notlost]

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]]

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-4, no longer working on Mozilla)]

(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-4, no longer working on Mozilla)]

Attachment: fix ComputeSizeWithIntrinsicDimensions

Comment 36

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

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-4, no longer working on Mozilla)]

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)]

(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]

(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-4, no longer working on Mozilla)]

(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]

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]

Attachment: 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-4, no longer working on Mozilla)]

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]

(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]

Oh, and the Customize Toolbar dialog box. "Use Small Icons" there.

Comment 46

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

Nope; you're welcome to do so.

Comment 47

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

(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 [:bzbarsky]]

> (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]

[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-4, no longer working on Mozilla)]

(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-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4789623b77f2

Comment 53

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/4789623b77f2

Comment 54

[Siddhartha Dugar [:sdrocking]]

Please land this for Aurora (Firefox 8) also.

Comment 55

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

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]

(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) (Not reading bugmail; email directly if needed)]

(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]

I forget if it was pushed into place, but starting with yesterday's Aurora nightly, it is 8.0a2.

Comment 59

[Matt Brubeck (:mbrubeck)]

(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]

(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-4, no longer working on Mozilla)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/620e3bda721b

Comment 63

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

Added reftest:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e09d10ebeaa8

Comment 64

[bogas04]

is it also related to the increased height find bar?

Comment 65

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(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 (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/e09d10ebeaa8

Comment 68

[[e-mail]]

(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 Badau, Desktop QA]

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