Control borders are not drawn correctly

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Ehsan, Assigned: masayuki)

Tracking

({regression})

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

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 2 obsolete attachments)

Created attachment 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.

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?
Regression window is http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1188831060&maxdate=1188833699

Comment 2

10 years ago
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
Blocks: 394109
(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

10 years ago
(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...
Created attachment 295598 [details] [diff] [review]
Patch rv1.0

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.
adding bug 382458 members to cc.
Attachment #295598 - Flags: superreview+
Attachment #295598 - Flags: review?(roc)
Attachment #295598 - Flags: review+
(Assignee)

Updated

10 years ago
Attachment #295598 - Flags: approval1.9?

Updated

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

Updated

10 years ago
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.
Created attachment 296303 [details] [diff] [review]
Patch for nsCoord.h

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
Depends on: 411706
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.
Created attachment 296484 [details] [diff] [review]
Backing out patch (including notification comments)
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
(Assignee)

Updated

10 years ago
Assignee: nobody → masayuki
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Created attachment 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.
Created attachment 296640 [details]
Screencap of expected display
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
Last Resolved: 10 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

Updated

10 years ago
Duplicate of this bug: 410885
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.