Union solid text underline rect with overflow rect even if the style is none

RESOLVED FIXED in mozilla20

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks 1 bug)

Trunk
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Posted patch PatchSplinter Review
bug 780436 comment 7

>  David Baron [:dbaron] 2012-08-06 16:46:29 PDT
> 
> I think in general we've been taking the opposite approach -- trying to 
> include the underline in the overflow rect (at least for common underline
> styles) so that we don't have to reflow.
> 
> Then again, I think Mats was working on supporting a stronger variant of
> UpdateOverflowArea that might be sufficient here (in addition to repaint).

I'm not sure the second paragraph. Anyway, we should fix this for bug 770780.
Attachment #681948 - Flags: review?(dbaron)
Summary: Union solid text underline rect even if the style is none → Union solid text underline rect with overflow rect even if the style is none
Sorry for taking so long to get back to this; I was confused by the bug summary and having trouble remembering what had motivated me to make my original comment.

What I meant in bug 780436 comment 7 was that the approach we'd been taking in the past meant that we would not have to reflow for any text-decoration-line changes (which bug 780436 introduced in some cases, and which I'd probably like to see removed).  The way we do that is by ensuring that the overflow needed for the current text-decoration-style is always included in the overflow area, whether or not text-decoration-line says we're currently drawing that decoration.

I don't understand why we'd want to do what the bug summary here says.  The none value for text-decoration-*style* is an edge case; it's a -moz-none value to support a particular IME case (bug 59109 comment 42 through bug 59109 comment 44).

It seems like what you're doing here is not actually bug 780436 comment 7 (as comment 0 suggests), but an attempt to fix the bug you described in bug 780436 comment 6 in a different way from what you suggested there (but also different from what I suggested in bug 780436 comment 7, which was the idea that text-decoration-style changes cause reflow but text-decoration-line changes do not).
That said, it seems like you're proposing an alternative model, that we reflow in even fewer cases:  only when text-decoration-style changes and the change involves 'wavy' or 'double'.  That's a reasonable alternative, and this patch is half of what's needed to get us there; the other half is always including the overflow for the current style even when text-decoration-lines says the lines aren't being drawn.
Comment on attachment 681948 [details] [diff] [review]
Patch

so r=dbaron; this looks good
Attachment #681948 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/217f9d114eee

Yes, this is very edge case. Probably, nobody has never seen the repaint problem actually.

> the other half is always including the overflow for the current style even when text-decoration-lines says the lines aren't being drawn.

We do it. It's for performance. Some web pages has following style:

a {
  text-decoration: none;
}
a:hover {
  text-decoration: underline;
}

If we do reflow at every hover/unhover time, CPU usage is risen up crazily. See bug 421353.
My original complaint was that bug 780436 was *not* doing it that way when the text-decoration-style was wavy or double.
https://hg.mozilla.org/mozilla-central/rev/217f9d114eee
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.