Closed Bug 392785 Opened 13 years ago Closed 12 years ago

overflowed underline sometimes is not repainted at scrolling

Categories

(Core :: Layout: Block and Inline, defect, P4)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

(Whiteboard: [dbaron-1.9:Rw])

Attachments

(2 files, 15 obsolete files)

7.35 KB, text/html
Details
59.27 KB, patch
Details | Diff | Splinter Review
Attached file testcase
This is remaining bug of bug 365414.

At scrolling, the bug still be reproduced, but I don't find the cause yet...

# when you drag the scrollbar on the attached testcase, you can see this bug.
Flags: blocking1.9?
er, the testcase is for windows.
I can reproduce this bug on the result of search of Google :-(
maybe, but I'm not sure.

I cannot reproduce this bug on Fx2 with the testcase.
Flags: blocking1.9? → blocking1.9+
I see on both Windows XP and Windows Vista.

Specifically, I'm seeing a few lines that are missing the underline below both "parent" strings, towards the end of the document when I scroll to the end.
Whiteboard: [dbaron-1.9:Rw]
I think we should fix this by making text decorations part of the overflow area.  Does that seem reasonable?
Yeah. I thought masayuki had a patch for that somewhere
(In reply to comment #7)
> Yeah. I thought masayuki had a patch for that somewhere

No, I was not working on this... I'm not sure why this can be reproduced at scrolling...
This bug is only reproduced at quirks mode.
Attached patch incomplete patch #1 (obsolete) — Splinter Review
This patch works fine on most sites, but I can reproduce this bug on google-ja...
Google-ja is using "font-family: arial,sans-serif;". The font fallback is a cause??
er, I see what is a cause, please wait.
I'd actually started working on this yesterday, but it looks like you've gotten farther.

We should do the same for nsHTMLContainerFrame as well, which requires figuring out which Reflow methods to touch, since it has a BuildDisplayList method that may not be overridden by frames that do override Reflow.  (And there, the aPresContext arguments to GetTextDecorations, HasTextFrameDescendant, and HasTextFrameDescendantOrInFlow could be removed.)

I'd note that in the code you moved into HasTextDecorations, you can remove the |hasDecorations| variable and just use context->HasTextDecorations().
Attached patch Patch rv1.0 (obsolete) — Splinter Review
o.k. this patch fixes this bug.
Assignee: nobody → masayuki
Attachment #289010 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #289024 - Flags: superreview?(roc)
Attachment #289024 - Flags: review?(roc)
er, if we can check whether the textframe is in editor,
I can improve |nsTextFrame::UnionTextDecorationOverflow|...
(And for what it's worth, I found the code in nsCSSRendering::PaintDecorationLine rather complicated -- so you need to be careful to match exactly the area that it draws.)
Comment on attachment 289024 [details] [diff] [review]
Patch rv1.0

oops, I don't check the dbaron's comment...
Attachment #289024 - Flags: superreview?(roc)
Attachment #289024 - Flags: review?(roc)
(In reply to comment #15)
> (And for what it's worth, I found the code in
> nsCSSRendering::PaintDecorationLine rather complicated -- so you need to be
> careful to match exactly the area that it draws.)

Yes. I wrote it...
Attached patch Patch rv1.1 (for quirks mode) (obsolete) — Splinter Review
Thank you, dbaron. I'm reading the code of nsHTMLContainerFrame now. This patch that is for quirks mode doesn't obstruct it, so we can land this first.
Attachment #289024 - Attachment is obsolete: true
Attachment #289031 - Flags: superreview?(roc)
Attachment #289031 - Flags: review?(roc)
Attachment #289031 - Attachment description: Patch rv1.1 → Patch rv1.1 (for quirks mode)
Attached patch Patch rv1.1 (for quirks mode) (obsolete) — Splinter Review
sorry for the spam.
Attachment #289031 - Attachment is obsolete: true
Attachment #289032 - Flags: superreview?(roc)
Attachment #289032 - Flags: review?(roc)
Attachment #289031 - Flags: superreview?(roc)
Attachment #289031 - Flags: review?(roc)
Don't pass aPresContext to RecomputeOverflowRect. The frame can always just call PresContext(). It's OK to pass it to UnionTextDecorationOverflow and HasTextDecorations because these are private methods so adding parameters to them is not such a big deal. It's important to keep exposed interfaces as simple as 
possible.

I think UnionTextDecorationOverflow should get the fontgroup from mTextRun. Then we don't need that comment in PropertyProvider.

+  PRBool HasTextDecorations(nsPresContext* aPresContext,
+                            PRUint8* aDecorations,
+                            nscolor* aOverColor = nsnull,
+                            nscolor* aUnderColor = nsnull,
+                            nscolor* aStrikeColor = nsnull);

Call this GetTextDecorations. Also, create a struct to return the three colors and the mask as a direct result. Instead of returning false, you can just return  NS_STYLE_TEXT_DECORATION_NONE in the mask.

+  if (aOverflowRect->width == 0)
+    return;
+

Why are you doing this? Speed? I don't think it's worth optimizing for.

Do we reflow on text-decoration changes? It looks like we should, with this patch.

Should we move some of UnionTextDecorationOverflow to nsCSSRendering so that it can be next to the painting code it needs to mirror? Seems to me we want a function that takes similar parameters to PaintDecorationLine but returns the vertical extent of what's drawn.
Attached patch Patch rv2.0 (obsolete) — Splinter Review
This is better. |nsCSSRendering::PaintDecorationLine| is using the computing of overflow area. This makes simple code and bug-less code.
Attachment #289032 - Attachment is obsolete: true
Attachment #289410 - Flags: superreview?(roc)
Attachment #289410 - Flags: review?(roc)
Attachment #289032 - Flags: superreview?(roc)
Attachment #289032 - Flags: review?(roc)
And I removed |aPreferredHeight| from |nsCSSRendering::PaintDecorationLine|. Because authors cannot control the decoration line height with CSS3 in latest draft. And it is used only at overline and strikeout-line. Their line height is not controlled by us too. So, we don't need it now.
It seems that I need to change |nsInlineFrame::ReflowFrames| for the overflow inline elements in standard mode:

> 588   // For now our overflow area is zero. The real value will be
> 589   // computed during vertical alignment of the line we are on.
> 590   aMetrics.mOverflowArea.SetRect(0, 0, 0, 0);

But roc, you have changed the overflow rect to this in bug 252771, so, it looks like that you don't want to change here... I think that there are two ways:

1. changing here, and if it's necessary, we recompute the overflow rect in nsLineLayout.
2. not changing here, we only compute overflow rect in nsLineLayout after the frame width is fixed. (But I don't found which is best point in nsLineLayout)

# And note that looks like that the testcase of bug 252771 is not works fine on latest trunk...
You want to do this in nsLineLayout::RelativePositionFrames where it does "// Compute a new combined area for the child span".
Blocks: 405308
Attached patch Patch rv3.0 (obsolete) — Splinter Review
This patch fixes this bug completely (both quirks mode and standards mode).
I have a question, nsDisplayTextDecoration::GetBounds is not uses actual decoration rect, is it o.k.?
Attachment #289410 - Attachment is obsolete: true
Attachment #290380 - Flags: superreview?(roc)
Attachment #290380 - Flags: review?(roc)
Attachment #289410 - Flags: superreview?(roc)
Attachment #289410 - Flags: review?(roc)
Attached patch Patch rv3.1 (obsolete) — Splinter Review
Oops, I forgot to change nsTextFrameThebes, it should also check the line-through area for very small font size.
Attachment #290380 - Attachment is obsolete: true
Attachment #290381 - Flags: superreview?(roc)
Attachment #290381 - Flags: review?(roc)
Attachment #290380 - Flags: superreview?(roc)
Attachment #290380 - Flags: review?(roc)
Attached patch Patch rv3.1 (obsolete) — Splinter Review
Sorry for the spam...
Attachment #290381 - Attachment is obsolete: true
Attachment #290382 - Flags: superreview?(roc)
Attachment #290382 - Flags: review?(roc)
Attachment #290381 - Flags: superreview?(roc)
Attachment #290381 - Flags: review?(roc)
oops, I forgot to change the IID of nsIDeviceContext, it will be changed next patch (after review).
roc:

Would you review this?
This is going to cause reflows on hovering links in pages that style links to not have an underline but be underlined on hover --- something I think is quite common. But I think those reflows will be fast enough (and asynchronous) so this won't be a significant problem.

ComputeTightBounds needs to call UnionTextDecorationOverflow.

+nsTextFrame::UnionTextDecorationOverflow(
+               nsPresContext* aPresContext,
+               const gfxTextRun::Metrics& aTextMetrics,
+               nsRect* aOverflowRect)
+{
+  EnsureTextRun();

I think it makes more sense to require callers to have called EnsureTextRun before we get here. So just add an assertion that mTextRun is not null.

+  // XXX the members of gfxTextRun::Metrics are in nscoord but the type is
+  // gfxFloat...

"appunits" not "nscoord". I actually don't think this needs an XXX comment.

If we always add the IME underline size to every frame, and that underline overflows the regular frame bounds, then this is going to hurt performance *very* badly because almost every frame in the tree will have to have an overflow area property. We can't allow that, we have to find another way.

Spellcheck changes will call nsTextFrame::SetSelected, do IME underline changes also call SetSelected? If so then UnionTextDecorationOverflow could check NS_FRAME_SELECTED_CONTENT, we'd also have to arrange to have selection changes reflow but I think that might be OK.
GetTextDecorationRect and GetTextDecorationRectInternal should just return a gfxRect directly.

Do we really need to round the decoration x and width? I don't think so. If so then GetTextDecorationRect doesn't really need to deal with width values, it only needs to work in the vertical direction, so it can take a height instead of a size and return two gfxFloats instead of a gfxRect.

Can we share code between nsLineLayout::CombineTextDecorations and UnionTextDecorationOverflow?

Maybe the way to deal with the IME underline problem is to add the text decoration size to the ascent and descent of the text frame instead of the overflow area. That wouldn't create a performance problem. It might change line spacing in a visible way though, not sure how serious that would be. How much do the IME underlines extend outside the font-box, usually?
(In reply to comment #31)
> Do we really need to round the decoration x and width? I don't think so. If so
> then GetTextDecorationRect doesn't really need to deal with width values, it
> only needs to work in the vertical direction, so it can take a height instead
> of a size and return two gfxFloats instead of a gfxRect.

I think yes. We need to do. Because line start/end might be AAed or lacked.

> How much do the IME underlines extend outside the font-box, usually?

On Mac, the ratio is 2. So, it is double size of the default underline size... It's too large height.

I have a question, do we need reflow for overflow computing? So, it seems that the DisplayList always refer the latest information of the frame. So, cannot include the IME/spellchecking underline height at directly?
Yes, we do need to reflow to recompute overflow, unfortunately.
Attached patch Patch rv4.0 (obsolete) — Splinter Review
This patch checking the nsIFrame::mState for IME/spellchecking.
I'm not sure that the params of FrameNeedsReflow is correct, please check it.
Attachment #290382 - Attachment is obsolete: true
Attachment #290843 - Flags: superreview?(roc)
Attachment #290843 - Flags: review?(roc)
Attachment #290382 - Flags: superreview?(roc)
Attachment #290382 - Flags: review?(roc)
Hmm, I think this approach should work. Good!

+    if (needReflow) {
+      PresContext()->PresShell()->FrameNeedsReflow(this,
+                                                   nsIPresShell::eStyleChange,
+                                                   NS_FRAME_IS_DIRTY);
+    } else {
+      // Selection might change anything. Invalidate the overflow area.
+      Invalidate(GetOverflowRect(), PR_FALSE);
+    }

We should always invalidate, because a reflow is not guaranteed to repaint the entire frame.

The definitions of GfxUnitsToAppUnits in comments should say that the coordinates are rounded to the nearest appunit.

+    PRBool needReflow = oldState != mState;
+    if (needReflow) {
+      // However, if the non-selected text already has underline, we don't need
+      // to reflow.
+      TextDecorations decorations = GetTextDecorations(aPresContext);
+      needReflow =
+        !(decorations.mDecorations & NS_STYLE_TEXT_DECORATION_UNDERLINE);
+      if (!needReflow) {
+        // However, if the IME underline is thicker than normal underline,
+        // we need to reflow.
+        nsILookAndFeel* look = aPresContext->LookAndFeel();
+        float ratio;
+        look->GetMetric(nsILookAndFeel::eMetricFloat_IMEUnderlineRelativeSize,
+                        ratio);
+        needReflow = ratio > 1.0;
+      }

This would be clearer if it was written as a big boolean expression.

I'll have another look through this tomorrow.
Attached patch Patch rv4.1 (obsolete) — Splinter Review
updating for comment 35.
Attachment #290843 - Attachment is obsolete: true
Attachment #290868 - Flags: superreview?(roc)
Attachment #290868 - Flags: review?(roc)
Attachment #290843 - Flags: superreview?(roc)
Attachment #290843 - Flags: review?(roc)
+  r.x = aPresContext->GfxUnitsToAppUnits(rect.X());
+  r.y = aPresContext->GfxUnitsToAppUnits(rect.Y());
+  r.width = aPresContext->GfxUnitsToAppUnits(rect.Width());
+  r.height = aPresContext->GfxUnitsToAppUnits(rect.Height());

Should probably use ScaleRoundOut here somehow.
(In reply to comment #37)
> +  r.x = aPresContext->GfxUnitsToAppUnits(rect.X());
> +  r.y = aPresContext->GfxUnitsToAppUnits(rect.Y());
> +  r.width = aPresContext->GfxUnitsToAppUnits(rect.Width());
> +  r.height = aPresContext->GfxUnitsToAppUnits(rect.Height());
> 
> Should probably use ScaleRoundOut here somehow.

I don't think so, because the rect is already rounded by nsCSSRendering::GetTextDecorationRectInternal.
Comment on attachment 290868 [details] [diff] [review]
Patch rv4.1

OK add a comment explaining why no rounding is needed.
Attachment #290868 - Flags: superreview?(roc)
Attachment #290868 - Flags: superreview+
Attachment #290868 - Flags: review?(roc)
Attachment #290868 - Flags: review+
checked-in.

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attached patch Patch rv4.1.1 (for checked-in) (obsolete) — Splinter Review
Attachment #290868 - Attachment is obsolete: true
Depends on: 406642
Could this have regressed bug 406729?
Depends on: 406729
Backed out at the request of roc & mconnor; reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The cause of bug 406642 is that FAYT uses nsTextFrame::GetPointFromOffset during dirty via Scroll. This issue should be fixed in FAYT? or nsTextFrame?
Attached patch Patch rv4.2 (obsolete) — Splinter Review
This patch fixes bug 406642. But this patch might need the patch of bug 402524.
Attachment #291219 - Attachment is obsolete: true
Attachment #291844 - Flags: superreview?(roc)
Attachment #291844 - Flags: review?(roc)
Comment on attachment 291844 [details] [diff] [review]
Patch rv4.2

Umm.... The some other points need the same change...
Attachment #291844 - Flags: superreview?(roc)
Attachment #291844 - Flags: review?(roc)
Attachment #291844 - Flags: review-
Attached patch Additional patch rv1.0 (obsolete) — Splinter Review
This is only additional part after rv4.1.1. These points are changing the selection range before ScrollSelectionIntoView.
Attachment #291844 - Attachment is obsolete: true
Attachment #291847 - Flags: superreview?(roc)
Attachment #291847 - Flags: review?(roc)
Attachment #291219 - Attachment is obsolete: false
I confirmed that the patch of bug 402524 fixes bug 406729, so, we can reland the patch of this bug after bug 402524.
Attachment #291847 - Flags: superreview?(roc)
Attachment #291847 - Flags: superreview+
Attachment #291847 - Flags: review?(roc)
Attachment #291847 - Flags: review+
Attached patch Patch rv4.2 (for re-check-in) (obsolete) — Splinter Review
updating for latest trunk and merged the additional patch.
Attachment #291219 - Attachment is obsolete: true
Attachment #291847 - Attachment is obsolete: true
Attached patch Patch rv4.2 (for re-check-in) (obsolete) — Splinter Review
oops, this is correct, sorry for the spam.
Attachment #302827 - Attachment is obsolete: true
gfx part was forgotten in previous patch...
Attachment #302828 - Attachment is obsolete: true
roc:

I want to land this. Is it OK? That makes to reopen bug 406729, because bug  	402524 is still under reviewing.

But I can work for bug 417014 and bug 412251.

And this patch is risky. I want to land ASAP.
Blocks: 412251, 417014
It would really be better if we don't regress anything even for a short time. We should be able to get Stuart to review bug 402524 tomorrow.
bug 402524 is fixed now, I'll land this after some hours if there are no problem in bug 402524.
checked-in.
Status: REOPENED → RESOLVED
Closed: 13 years ago12 years ago
Resolution: --- → FIXED
This was a ~4ms Ts regression on Linux at least.
(In reply to comment #56)
> This was a ~4ms Ts regression on Linux at least.

It's strange. I guessed that if the patch has performance regression, it is tp damage. At starting-up, we need/run such many text reflow/rendering??
Depends on: 418470
Depends on: 419531
I believe this has caused bug 419531
Has this caused bug 421782? Looking into bonsai and ftp it looks it was landed before firefox trunk build but after seamonkey trunk build on 20080216. And that are exactly the builds that first show and last don't show the slowdown respectively.
Depends on: 423676
Duplicate of this bug: 93717
You need to log in before you can comment on or make changes to this bug.