410748 - Control borders are not drawn correctly

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[(no longer active)]

Attachment: Screenshot of the problem

See the URL <http://moz.qaparticipation.sgizmo.com/>.  The left border of the first drop-down list as well as the two text areas are not drawn in the latest nightly as well as beta 2.  The screenshot attached demonstrates the problem.

In my testing, the left borders are never drawn when the page loads initially.  After that, if the window is resized, the left borders blink, and sometimes when you stop resizing, the borders remain drawn, and other times they don't.  When they remain drawn, causing the window to invalidate by things other than resizing the window (such as minimizing/restoring it, or moving an object in front of it) don't affect the left border's visibility, but resizing again could make it disappear.

The same page renders correctly in Firefox 2.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Regression window is http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1188831060&maxdate=1188833699

Comment 2

[Simon Bünzli]

Might be the same issue:
1. Set accessibility.tabfocus to 3
2. Go to https://bugzilla.mozilla.org/enter_bug.cgi?product=Firefox
3. Scroll down until the Component <select> is invisible
4. Select the Severity drop-down
5. Shift+Tab back 4 times (until the Component <select> is completely visible)

The top border of the Component <select> is missing...

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008010304

Comment 3

[(no longer active)]

(In reply to comment #2)
> Might be the same issue:
> 1. Set accessibility.tabfocus to 3
> 2. Go to https://bugzilla.mozilla.org/enter_bug.cgi?product=Firefox
> 3. Scroll down until the Component <select> is invisible
> 4. Select the Severity drop-down
> 5. Shift+Tab back 4 times (until the Component <select> is completely visible)
> 
> The top border of the Component <select> is missing...
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008010304

Confirming:
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2008010502 Minefield/3.0b3pre

I've filed a separate bug for it (bug 410885), which should be duped to this bug *if* they're the same issue.  I did it to make sure that the issue you mentioned would not be ignored.

Comment 4

[Worcester12345]

(In reply to comment #0)
> Created an attachment (id=295340) [details]
> Screenshot of the problem
> 
> See the URL <http://moz.qaparticipation.sgizmo.com/>.  The left border of the
> first drop-down list as well as the two text areas are not drawn in the latest
> nightly as well as beta 2.  The screenshot attached demonstrates the problem.

Not seeing the behavior here using Build identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.9b3pre) Gecko/2008010502 SeaMonkey/2.0a1pre

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

.sgControlSet has "overflow: auto;", therefore, the negative area that has left border of the select/textarea is clipped...

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

I have fixed negative rounding in gfxRect::Round(). We need same fix in gfxPoint::Round().

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

er, the patch is not enough for this bug, but it's really needed I think.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

adding bug 382458 members to cc.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in the patch. it might make new regressions like bug 394109. we need to find the cause of this bug, but I have no idea about the cause now.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that the cause is here:

http://mxr.mozilla.org/mozilla/source/view/src/nsView.cpp#343
 343 nsRect nsView::CalcWidgetBounds(nsWindowType aType)
 344 {
 345   nsCOMPtr<nsIDeviceContext> dx;
 346   mViewManager->GetDeviceContext(*getter_AddRefs(dx));
 347   NS_ASSERTION(dx, "View manager can't be created without a device context");
 348   PRInt32 p2a = dx->AppUnitsPerDevPixel();
 349 
 350   nsRect viewBounds(mDimBounds);
 351 
 352   if (GetParent()) {
 353     // put offset into screen coordinates
 354     nsPoint offset;
 355     nsIWidget* parentWidget = GetParent()->GetNearestWidget(&offset);
 356     viewBounds += offset;
 357 
 358     if (parentWidget && aType == eWindowType_popup &&
 359         mVis == nsViewVisibility_kShow) {
 360       nsRect screenRect(0,0,1,1);
 361       parentWidget->WidgetToScreen(screenRect, screenRect);
 362       viewBounds += nsPoint(NSIntPixelsToAppUnits(screenRect.x, p2a),
 363                             NSIntPixelsToAppUnits(screenRect.y, p2a));
 364     }
 365   }
 366 
 367   nsRect newBounds(viewBounds);
 368   newBounds.ScaleRoundPreservingCentersInverse(p2a);
 369 
 370   nsPoint roundedOffset(NSIntPixelsToAppUnits(newBounds.x, p2a),
 371                         NSIntPixelsToAppUnits(newBounds.y, p2a));
 372   mViewToWidgetOffset = viewBounds.TopLeft() - roundedOffset;
 373 
 374   return newBounds;
 375 }

mViewToWidgetOffset can be negative offset. At that time, the control position is moved to negative position by gfxWindowsNativeDrawing::TransformToNativeRect.

However, I have no idea what is correct.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for nsCoord.h

And I found a bug of negative rounding.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that this is a bug of View or ViewManager. If you cannot agree, please reset the component.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

And the related code of the view offset is added in bug 380438. I think that the really regression cause is it.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually, I've changed my mind. I think we shouldn't use NS_round here or in gfxPoint or gfxRect.

Using floor(v + 0.5) has the property that if v2 - v1 (e.g. the width of a rect) is an integer W, then floor(v1 + 0.5) - floor(v2 + 0.5) is also equal to W. This is very important; for example it means when we round a border rect to an integer in layout, we can be sure it will actually be the same number of pixels wide even when we round coordinates.

But using "round" we don't have that property. For example, when v1 = -0.5 and v2 = 0.5, the width after rounding is 2 instead of 1.

I think we should back out the changes to gfxPoint and gfxRect and then see what bugs remain.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #14)
> Using floor(v + 0.5) has the property that if v2 - v1 (e.g. the width of a
> rect) is an integer W, then floor(v1 + 0.5) - floor(v2 + 0.5) is also equal to
> W. This is very important; for example it means when we round a border rect to
> an integer in layout, we can be sure it will actually be the same number of
> pixels wide even when we round coordinates.

How about adding |Floor()| method to them? |Round()| is misleadable.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #15)
> (In reply to comment #14)
> > Using floor(v + 0.5) has the property that if v2 - v1 (e.g. the width of a
> > rect) is an integer W, then floor(v1 + 0.5) - floor(v2 + 0.5) is also equal to
> > W. This is very important; for example it means when we round a border rect to
> > an integer in layout, we can be sure it will actually be the same number of
> > pixels wide even when we round coordinates.
> 
> How about adding |Floor()| method to them? |Round()| is misleadable.

Oops, not |Floor()|, |RoundUp()|?

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > Using floor(v + 0.5) has the property that if v2 - v1 (e.g. the width of a
> > > rect) is an integer W, then floor(v1 + 0.5) - floor(v2 + 0.5) is also equal to
> > > W. This is very important; for example it means when we round a border rect to
> > > an integer in layout, we can be sure it will actually be the same number of
> > > pixels wide even when we round coordinates.
> > 
> > How about adding |Floor()| method to them? |Round()| is misleadable.
> 
> Oops, not |Floor()|, |RoundUp()|?

Or, |RoundOut()| is good name for using NS_round()?

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think Round() is OK, I can't think of anything better. We should just
document it. It's definitely doing some kind of rounding and there isn't any
other rounding behaviour that makes sense.

RoundAwayFromZero would be a good name for code that used NS_round, but we really don't want that here.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Backing out patch (including notification comments)

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

reset the component to gfx/thebes

Comment 21

[Justin Dolske [:Dolske]]

Attachment: Screencap of fuzzy favicon

While testing bug 389273, I found that the previous patch here causes favicons on HTTPS sites to display incorrectly. The icon in the URL bar is scaled (to 16x17?), and looks fuzzy. Locally backing out the change to gfxPoint.h fixed the problem.

Comment 22

[Justin Dolske [:Dolske]]

Attachment: Screencap of expected display

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in.

Comment 24

[Dão Gottwald [:dao]]

(In reply to comment #21)
> Created an attachment (id=296639) [details]
> Screencap of fuzzy favicon
> 
> While testing bug 389273, I found that the previous patch here causes favicons
> on HTTPS sites to display incorrectly. The icon in the URL bar is scaled (to
> 16x17?), and looks fuzzy. Locally backing out the change to gfxPoint.h fixed
> the problem.

bug 411296