Control borders are not drawn correctly

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
12 years ago
Last year

People

(Reporter: Ehsan, Assigned: masayuki)

Tracking

({regression})

Trunk
x86
Windows Vista
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(4 attachments, 2 obsolete attachments)

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.
Flags: blocking1.9?
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
(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.
(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
.sgControlSet has "overflow: auto;", therefore, the negative area that has left border of the select/textarea is clipped...
Posted patch Patch rv1.0 (obsolete) — Splinter Review
I have fixed negative rounding in gfxRect::Round(). We need same fix in gfxPoint::Round().
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #295598 - Flags: review?(roc)
er, the patch is not enough for this bug, but it's really needed I think.
Attachment #295598 - Flags: approval1.9? → approval1.9+
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.
Assignee: masayuki → nobody
Status: ASSIGNED → NEW
Depends on: 411296
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.
Posted patch Patch for nsCoord.h (obsolete) — Splinter Review
And I found a bug of negative rounding.
Attachment #296303 - Flags: superreview?(roc)
Attachment #296303 - Flags: review?(roc)
I think that this is a bug of View or ViewManager. If you cannot agree, please reset the component.
Assignee: nobody → roc
Component: GFX → Layout: View Rendering
QA Contact: general → ian
And the related code of the view offset is added in bug 380438. I think that the really regression cause is it.
Blocks: 380438
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.
(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.
(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()|?
(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()?

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.
Assignee: roc → masayuki
Attachment #295598 - Attachment is obsolete: true
Attachment #296303 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #296484 - Flags: superreview?(roc)
Attachment #296484 - Flags: review?(roc)
Attachment #296303 - Flags: superreview?(roc)
Attachment #296303 - Flags: review?(roc)
reset the component to gfx/thebes
Assignee: masayuki → nobody
Status: ASSIGNED → NEW
Component: Layout: View Rendering → GFX: Thebes
QA Contact: ian → thebes
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.
Attachment #296484 - Flags: superreview?(roc)
Attachment #296484 - Flags: superreview+
Attachment #296484 - Flags: review?(roc)
Attachment #296484 - Flags: review+
Attachment #296484 - Flags: approval1.9+
checked-in.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
(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
Duplicate of this bug: 410885
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.