Closed Bug 405308 Opened 14 years ago Closed 14 years ago

[text-decoration] Link underline disappears (or is misaligned 1px) when scrolling

Categories

(Core :: Layout: Text and Fonts, defect, P1)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: mats, Assigned: masayuki)

References

(Blocks 1 open bug, )

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

[text-decoration] Link underline disappears (or is misaligned 1px) when scrolling

STEPS TO REPRODUCE
1. do a Bugzilla query that results in list of a few hundred bugs
2. click on the scrollbar thumb and scroll up and down a for a while
   without releasing the mouse

ACTUAL RESULT
Link underline is sometimes not painted or painted 1px above where it
should be (fwiw, I've never seen it being painted 1px too low).
Covering the window to force a repaint fixes it.

I've seen this bug in the past but the problem is much easier to reproduce
in recent builds (I can reproduce in a few seconds).

PLATFORMS AND BUILDS TESTED
Bug occurs in Firefox 2007112404 on Linux

I think the regression window is: 2007-08-05-04 -- 2007-08-06-04
(bug 365336?) but I might be wrong.
Flags: blocking1.9?
The testcase in that bug works fine for me on Linux though.
I'll test again once that bug is fixed, marking dependent for now...
Depends on: 392785
based on the deps punting to layout....  send back to gfx if you disagree.
Component: GFX: Thebes → Layout: Fonts and Text
QA Contact: thebes → layout.fonts-and-text
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Assignee: nobody → masayuki
Priority: P2 → P4
bug 392785 was fixed, so this should be fixed now.
Taking off the blocking list, should be fixed.
Flags: blocking1.9-
Masayuki, I'm still seeing this in Firefox and SeaMonkey nightly builds
on Linux.  On a 101x101 DPI screen in case that's a factor.
oh, I see. I can reproduce on my system. Only I can reproduce at scrolling to top of the page.
I can reproduce on Mac too.
OS: Linux → All
Hardware: PC → All
Flags: blocking1.9- → blocking1.9?
Attached patch Patch v1.0 (obsolete) — Splinter Review
At scrolling to top of the patch, the frame offset might be negative. In that case there is the rounding problem in nsCSSRenderling::GetTextDecorationRectInternal.
Attachment #306762 - Flags: superreview?(roc)
Attachment #306762 - Flags: review?(roc)
Comment on attachment 306762 [details] [diff] [review]
Patch v1.0

OK.
Attachment #306762 - Flags: superreview?(roc)
Attachment #306762 - Flags: superreview+
Attachment #306762 - Flags: review?(roc)
Attachment #306762 - Flags: review+
Oh Masayuki-san, please use NS_floor instead of "floor".

Stuart, Vlad, should we change NS_round etc in nsMathUtils.h to not round towards zero, or maybe just remove them, or rename them and add something else that does reasonable rounding? See http://weblogs.mozillazine.org/roc/archives/2008/02/rounding_toward.html.
Well, NS_round() does exactly what it says, it rounds :)  You want NSToIntCoord(), I believe, for the same-direction rounding semantics.
Attached patch Patch v1.0.1Splinter Review
Thank you, roc. Let's take this. This risk is too low. This fixes only rounding mistakes.
Attachment #306762 - Attachment is obsolete: true
Attachment #306950 - Flags: superreview+
Attachment #306950 - Flags: review+
Attachment #306950 - Flags: approval1.9?
(In reply to comment #13)
> Well, NS_round() does exactly what it says, it rounds :)

Hey, I don't think rounding-towards-zero is mandated in my English dictionary!

> You want
> NSToIntCoord(), I believe, for the same-direction rounding semantics.

OK, but what is the point of having NS_round round towards zero when that's just going to burn most users of it who ever have to deal with negative values? If we don't want to change it, I think we should just remove it.

Currently, most users of NS_(l)round(f) are only dealing with non-negative values, and the ones that aren't (most notably nsIDeviceContext::GfxUnitsToAppUnits) are probably buggy.
NS_(l)round(f) are meant to be libc compatible.  Changing them doesn't make sense.  feel free to add new functions!
Using them doesn't make sense either, so why not just remove them?
Comment on attachment 306950 [details] [diff] [review]
Patch v1.0.1

a1.9=beltzner
Attachment #306950 - Flags: approval1.9? → approval1.9+
checked-in.
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: blocking1.9?
Resolution: --- → FIXED
Blocks: 421165
Should we not at least replace all NS_round's with NS_floor(0.5+x) ?
And may be add NS_roundup(x) { return floor(x + 0.5); } to nsMathUtils.h ?
All current users seems to work with positive numbers, and NS_round is more expensive than NS_floor, but the same for positive numbers, and is 'wrong' for negative numbers...

Created bug 421165 to do s/NS_round(/NS_floor(0.5+/
We shouldn't replace NS_round calls with NS_floor(0.5+x), we need to keep using a single function in case we can do performance improvements or something.
Flags: blocking1.9+
Priority: P4 → P1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.