The default bug view has changed. See this FAQ.

Inconsistent extra 1 pixel width of XUL window

RESOLVED FIXED in mozilla17

Status

()

Core
XP Toolkit/Widgets: XUL
RESOLVED FIXED
13 years ago
14 days ago

People

(Reporter: Wang Xianzhu, Assigned: darktrojan)

Tracking

(Depends on: 1 bug)

Trunk
mozilla17
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

13 years ago
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:
(Reporter)

Comment 1

13 years ago
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.
(Reporter)

Updated

13 years ago
Attachment #139101 - Attachment description: the red background is invisible after the first sizeToContent or reload → the red background is visible after the first sizeToContent or reload

Comment 2

13 years ago
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.
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

12 years ago
This bug still exists in FireFox 1.0.7.

Comment 5

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


Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

9 years ago
Duplicate of this bug: 392503

Updated

9 years ago
Component: DOM: Views and Formatting → DOM: CSS Object Model
QA Contact: ian → general
QA Contact: general → style-system
Assignee: general → nobody
Component: DOM: CSS Object Model → XP Toolkit/Widgets: XUL
QA Contact: style-system → xptoolkit.xul
(Assignee)

Comment 7

6 years ago
>    /* 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.
(Assignee)

Comment 8

6 years ago
Created attachment 558444 [details] [diff] [review]
patch
Attachment #558444 - Flags: review?(neil)

Comment 9

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

Comment 10

6 years ago
Oh, hmm, those could be fun to track down. :-/
(Assignee)

Comment 11

6 years ago
Created attachment 558763 [details] [diff] [review]
patch

That instance is the only one I could find, using the powers of MXR and grep -A.
Attachment #558444 - Attachment is obsolete: true
Attachment #558763 - Flags: review?(neil)
Attachment #558444 - Flags: review?(neil)

Updated

6 years ago
Attachment #558763 - Flags: review?(neil) → review+
(Assignee)

Comment 12

6 years ago
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
Status: NEW → ASSIGNED
Keywords: checkin-needed

Updated

6 years ago
Assignee: nobody → geoff

Updated

6 years ago
Keywords: checkin-needed
Whiteboard: [inbound]
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.
Whiteboard: [inbound]
(Assignee)

Comment 14

6 years ago
Okay, I'm in way over my head here. I'm going to leave this one to the layout ninjas.
Assignee: geoff → nobody
Status: ASSIGNED → NEW
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.
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.
Duplicate of this bug: 696746

Comment 18

6 years ago
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.
(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

5 years ago
Markus, did you look into this issue?
I did at some point, and the first thing I tried didn't work. I'll look into it again now.
Created attachment 581637 [details] [diff] [review]
don't use CGContextClearRect when clearing background

Does this patch fix it?

Comment 23

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

5 years ago
I'm now seeing the cocoa alpha painting issue with other patches too:
  https://tbpl.mozilla.org/?tree=Try&rev=e4ebb5eff940
Created attachment 592114 [details] [diff] [review]
another attempt

Does this work?
Attachment #581637 - Attachment is obsolete: true

Comment 26

5 years ago
(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.
I believe the bug was 689962

Comment 28

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

Updated

5 years ago
Blocks: 357725
Depends on: 734381
I've filed bug 734381.

Comment 30

5 years ago
ping
(Assignee)

Comment 31

5 years ago
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
(Assignee)

Comment 32

5 years ago
Landing attempt #2, 10 months later :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/283e4488bb3e
Assignee: nobody → geoff
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/283e4488bb3e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

14 days ago
Depends on: 1345454
You need to log in before you can comment on or make changes to this bug.