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

Status: VERIFIED FIXED

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

Description

[Mats Palmgren (inactive)]

[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.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Screenshot, Firefox 2007112404 on Linux

Comment 2

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

dup of bug 392785?

Comment 3

[Mats Palmgren (inactive)]

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

Comment 4

[Stuart Parmenter]

based on the deps punting to layout....  send back to gfx if you disagree.

Comment 5

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

bug 392785 was fixed, so this should be fixed now.

Comment 6

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

Taking off the blocking list, should be fixed.

Comment 7

[Mats Palmgren (inactive)]

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.

Comment 8

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

oh, I see. I can reproduce on my system. Only I can reproduce at scrolling to top of the page.

Comment 9

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

I can reproduce on Mac too.

Comment 10

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

Attachment: Patch v1.0

At scrolling to top of the patch, the frame offset might be negative. In that case there is the rounding problem in nsCSSRenderling::GetTextDecorationRectInternal.

Comment 11

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

Comment on attachment 306762 [details] [diff] [review]
Patch v1.0

OK.

Comment 12

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

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.

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Well, NS_round() does exactly what it says, it rounds :)  You want NSToIntCoord(), I believe, for the same-direction rounding semantics.

Comment 14

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

Attachment: Patch v1.0.1

Thank you, roc. Let's take this. This risk is too low. This fixes only rounding mistakes.

Comment 15

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

(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.

Comment 16

[Stuart Parmenter]

NS_(l)round(f) are meant to be libc compatible.  Changing them doesn't make sense.  feel free to add new functions!

Comment 17

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

Using them doesn't make sense either, so why not just remove them?

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 306950 [details] [diff] [review]
Patch v1.0.1

a1.9=beltzner

Comment 19

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

checked-in.

Comment 20

[Alfred Kayser]

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+/

Comment 21

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

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.