230959 - Inconsistent extra 1 pixel width of XUL window

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Wang Xianzhu]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; zh-CN; rv:1.5) Gecko/20031007
Build Identifier: Most recent

For the attached file, the results of sizeToContents are inconsistent. 
After the first sizeToContent, the window width is 1 pixel wider than expected.
 After the second sizeToContent, the window width is correct.

You can repeat the above by click SizeToC button after resize the whole window.

I know this is caused by the following code in
content/base/src/nsDocumentViewer.cpp (about line 2700):

   /* presContext's size was calculated in twips and has already been
      rounded to the equivalent pixels (so the width/height calculation
      we just performed was probably exact, though it was based on
      values already rounded during ResizeReflow). In a surprising
      number of instances, this rounding makes a window which for want
      of one extra pixel's width ends up wrapping the longest line of
      text during actual window layout. This makes the window too short,
      generally clipping the OK/Cancel buttons. Here we add one pixel
      to the calculated width, to circumvent this problem. */
   NS_ENSURE_SUCCESS(treeOwner->SizeShellTo(docShellAsItem, width+1, height),
      NS_ERROR_FAILURE);

I think this 'width+1' is harmful in some circumstances.  If the extra 1 pixel
is in deed required, it should be consistent for sizeToContent calls.


Reproducible: Always

Steps to Reproduce:

Comment 1

[Wang Xianzhu]

Attachment: the red background is visible after the first sizeToContent or reload

In the file, box3's width style is set to 0px, but it is 1px when displayed. 
This is unexpected.

Comment 2

[Wang Xianzhu]

I think the following modification can solve this problem:

float tempWidth = (float)shellArea.width * pixelScale;
width = PRInt32(tempWidth);
if (tempWidth - width > 0.001)
{   // to let the extra 1 pixel when it is required
    width ++;
}

......
and remove the "+1" in the following SizeShellTo call.

Comment 3

[Gervase Markham [:gerv]]

This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/

Comment 4

[Wang Xianzhu]

This bug still exists in FireFox 1.0.7.

Comment 5

[Matt Crocker]

Still exists in FF3rc1.

You can work around it by checking dimensions and only calling sizeToContent when the window will actually change size, or you can simply subtract 1 from the width as in http://lxr.mozilla.org/seamonkey/source/toolkit/components/alerts/resources/content/alert.js#138

Comment 7

[Geoff Lankow (:darktrojan)]

>    /* presContext's size was calculated in twips and has already been
>       rounded to the equivalent pixels (so the width/height calculation
>       we just performed was probably exact, though it was based on
>       values already rounded during ResizeReflow). In a surprising
>       number of instances, this rounding makes a window which for want
>       of one extra pixel's width ends up wrapping the longest line of
>       text during actual window layout. This makes the window too short,
>       generally clipping the OK/Cancel buttons. Here we add one pixel
>       to the calculated width, to circumvent this problem. */
>    NS_ENSURE_SUCCESS(treeOwner->SizeShellTo(docShellAsItem, width+1, height),
>       NS_ERROR_FAILURE);

I think this +1 is no longer necessary. I'm going to change it on my local build and use it for a while to see if anything breaks.

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: patch

Comment 9

[[e-mail]]

(From update of attachment 558444 [details] [diff] [review])
What about existing consumers, such as mentioned in comment #5?

Comment 10

[Geoff Lankow (:darktrojan)]

Oh, hmm, those could be fun to track down. :-/

Comment 11

[Geoff Lankow (:darktrojan)]

Attachment: patch

That instance is the only one I could find, using the powers of MXR and grep -A.

Comment 12

[Geoff Lankow (:darktrojan)]

For whoever's on checkin duty, this patch passed try before the latest change, and I see no reason why that change would break it.
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=ac0d4351702f

Comment 13

[(no longer active)]

This was backed out of mozilla-inbound as part of this push <https://tbpl.mozilla.org/?tree=Mozilla-Inbound&usebuildbot=1&rev=9f150c4e1a48> which turned the 32-bit OS X reftests perma-orange.

Comment 14

[Geoff Lankow (:darktrojan)]

Okay, I'm in way over my head here. I'm going to leave this one to the layout ninjas.

Comment 15

[Timothy Nikkel (:tnikkel)]

Don't give up! If your patch passed try then it is highly likely that it was one of the other patches in the push that were at fault and not yours.

Comment 16

[Markus Stange [:mstange]]

Looks like it hadn't passed try after all. The try push in comment 12 shows the same Mac OS X 10.5 / 32 bit reftest failures.

Comment 18

[Neil Deakin]

My version of the patch causes the following failures:

REFTEST TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/test/build/reftest/tests/image/test/reftest/ico/ico-bmp-1bpp/ico-size-1x1-1bpp.ico | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/test/build/reftest/tests/image/test/reftest/jpeg/jpg-srgb-icc.jpg | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/xul/menuitem-key.xul | image comparison (==)

menuitem-key.xul for some reason has a greenish coloured background, rather than a white one. The reference has a slightly paler background. Not sure why, but I can't reproduce on 10.6 or 10.5 locally. Perhaps something isn't being initialized in this specific case. Of particular note, this reftest contains <menuitems> not in a popup.

The native theme drawing code for menuitems contains a call to CGContextClearRect to set the background. Perhaps the bug is that this call is using some state (alpha, background color, etc.) that was set somewhere else that should be set explicitly.

Markus, any thoughts here?

The two icon tests have a pixel at (8,8) that differs: (249, 250, 6) or (255, 254, 3) instead of white.

Comment 19

[Markus Stange [:mstange]]

(In reply to Neil Deakin from comment #18)
> The native theme drawing code for menuitems contains a call to
> CGContextClearRect to set the background.

Oh! That can't work when painting into a context without alpha. I suspect that call sets the alpha components to zero and leaves garbage in the RGB values, and then we ignore the alpha values and display those garbage RGB values. I'll think about a fix.

Comment 20

[Neil Deakin]

Markus, did you look into this issue?

Comment 21

[Markus Stange [:mstange]]

I did at some point, and the first thing I tried didn't work. I'll look into it again now.

Comment 22

[Markus Stange [:mstange]]

Attachment: don't use CGContextClearRect when clearing background

Does this patch fix it?

Comment 23

[Neil Deakin]

(In reply to Markus Stange from comment #22)
> Does this patch fix it?

Doesn't look like it is fixed:
  https://tbpl.mozilla.org/?tree=Try&rev=8dfec9ac5de6

Comment 24

[Neil Deakin]

I'm now seeing the cocoa alpha painting issue with other patches too:
  https://tbpl.mozilla.org/?tree=Try&rev=e4ebb5eff940

Comment 25

[Markus Stange [:mstange]]

Attachment: another attempt

Does this work?

Comment 26

[Neil Deakin]

(In reply to Markus Stange from comment #25)
> Created attachment 592114 [details] [diff] [review]
> another attempt
> 
> Does this work?

That patch fixed the menu test.

The failure with the image test still happens (see https://tbpl.mozilla.org/?tree=Try&rev=1f0ea28942df ), but Jeff said he had a patch for that I think. Hopefully, he remembers this and can comment.

Comment 27

[Jeff Muizelaar [:jrmuizel]]

I believe the bug was 689962

Comment 28

[Neil Deakin]

Markus, as your patch seems to work now, can it be reviewed and checked in? Possibly in another bug?

Comment 29

[Markus Stange [:mstange]]

I've filed bug 734381.

Comment 30

[Ben Bucksch (:BenB)]

ping

Comment 31

[Geoff Lankow (:darktrojan)]

I pushed my patch to Try again, and it looks like the OS X reftests are passing now, so if nothing else has broken in the meantime I'll land it.

https://tbpl.mozilla.org/?tree=Try&rev=6969c5e13af2

Comment 32

[Geoff Lankow (:darktrojan)]

Landing attempt #2, 10 months later :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/283e4488bb3e

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/283e4488bb3e

Comment 34

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336