Last Comment Bug 230959 - Inconsistent extra 1 pixel width of XUL window
: Inconsistent extra 1 pixel width of XUL window
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XP Toolkit/Widgets: XUL (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla17
Assigned To: Geoff Lankow (:darktrojan)
:
Mentors:
: 392503 696746 (view as bug list)
Depends on: 734381
Blocks: 357725
  Show dependency treegraph
 
Reported: 2004-01-14 21:43 PST by Wang Xianzhu
Modified: 2012-07-26 05:07 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
the red background is visible after the first sizeToContent or reload (1.47 KB, application/vnd.mozilla.xul+xml)
2004-01-14 21:52 PST, Wang Xianzhu
no flags Details
patch (1.79 KB, patch)
2011-09-06 04:38 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Splinter Review
patch (2.74 KB, patch)
2011-09-07 03:28 PDT, Geoff Lankow (:darktrojan)
neil: review+
Details | Diff | Splinter Review
don't use CGContextClearRect when clearing background (1.41 KB, patch)
2011-12-14 07:45 PST, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
another attempt (1.50 KB, patch)
2012-01-27 06:06 PST, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review

Description Wang Xianzhu 2004-01-14 21:43:25 PST
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 2004-01-14 21:52:43 PST
Created attachment 139101 [details]
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 2004-02-03 17:32:11 PST
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] 2005-09-27 02:08:25 PDT
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 2005-09-27 18:23:56 PDT
This bug still exists in FireFox 1.0.7.
Comment 5 Matt Crocker 2008-06-02 19:49:09 PDT
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 6 Matt Crocker 2008-06-02 19:50:05 PDT
*** Bug 392503 has been marked as a duplicate of this bug. ***
Comment 7 Geoff Lankow (:darktrojan) 2011-08-29 03:06:14 PDT
>    /* 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) 2011-09-06 04:38:23 PDT
Created attachment 558444 [details] [diff] [review]
patch
Comment 9 neil@parkwaycc.co.uk 2011-09-06 08:02:38 PDT
(From update of attachment 558444 [details] [diff] [review])
What about existing consumers, such as mentioned in comment #5?
Comment 10 Geoff Lankow (:darktrojan) 2011-09-06 15:45:04 PDT
Oh, hmm, those could be fun to track down. :-/
Comment 11 Geoff Lankow (:darktrojan) 2011-09-07 03:28:37 PDT
Created attachment 558763 [details] [diff] [review]
patch

That instance is the only one I could find, using the powers of MXR and grep -A.
Comment 12 Geoff Lankow (:darktrojan) 2011-09-07 16:11:32 PDT
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 :Ehsan Akhgari 2011-09-08 08:34:51 PDT
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) 2011-09-08 19:15:56 PDT
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) 2011-09-08 22:03:06 PDT
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] 2011-09-09 00:57:52 PDT
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 17 Markus Stange [:mstange] 2011-10-29 09:15:07 PDT
*** Bug 696746 has been marked as a duplicate of this bug. ***
Comment 18 Neil Deakin 2011-11-01 18:46:33 PDT
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] 2011-11-02 13:15:36 PDT
(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 2011-12-05 10:02:37 PST
Markus, did you look into this issue?
Comment 21 Markus Stange [:mstange] 2011-12-14 07:09:13 PST
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] 2011-12-14 07:45:58 PST
Created attachment 581637 [details] [diff] [review]
don't use CGContextClearRect when clearing background

Does this patch fix it?
Comment 23 Neil Deakin 2011-12-14 17:21:51 PST
(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 2012-01-11 14:00:15 PST
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] 2012-01-27 06:06:30 PST
Created attachment 592114 [details] [diff] [review]
another attempt

Does this work?
Comment 26 Neil Deakin 2012-01-27 13:50:28 PST
(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] 2012-01-27 13:52:42 PST
I believe the bug was 689962
Comment 28 Neil Deakin 2012-02-14 05:43:54 PST
Markus, as your patch seems to work now, can it be reviewed and checked in? Possibly in another bug?
Comment 29 Markus Stange [:mstange] 2012-03-09 07:07:00 PST
I've filed bug 734381.
Comment 30 Ben Bucksch (:BenB) 2012-07-25 03:21:11 PDT
ping
Comment 31 Geoff Lankow (:darktrojan) 2012-07-25 05:13:45 PDT
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) 2012-07-25 20:50:43 PDT
Landing attempt #2, 10 months later :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/283e4488bb3e
Comment 33 Ed Morley [:emorley] 2012-07-26 05:07:58 PDT
https://hg.mozilla.org/mozilla-central/rev/283e4488bb3e

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