392785 - overflowed underline sometimes is not repainted at scrolling

Status: RESOLVED FIXED

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

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

er, the testcase is for windows.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

I can reproduce this bug on the result of search of Google :-(

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is this a regression?

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

maybe, but I'm not sure.

I cannot reproduce this bug on Fx2 with the testcase.

Comment 5

[Daniel Holbert [:dholbert]]

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.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think we should fix this by making text decorations part of the overflow area.  Does that seem reasonable?

Comment 7

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

Yeah. I thought masayuki had a patch for that somewhere

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

This bug is only reproduced at quirks mode.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: incomplete patch #1

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

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

er, I see what is a cause, please wait.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.0

o.k. this patch fixes this bug.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

er, if we can check whether the textframe is in editor,
I can improve |nsTextFrame::UnionTextDecorationOverflow|...

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 289024 [details] [diff] [review]
Patch rv1.0

oops, I don't check the dbaron's comment...

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.1 (for quirks mode)

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.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv1.1 (for quirks mode)

sorry for the spam.

Comment 20

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

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.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv2.0

This is better. |nsCSSRendering::PaintDecorationLine| is using the computing of overflow area. This makes simple code and bug-less code.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

Comment 24

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

You want to do this in nsLineLayout::RelativePositionFrames where it does "// Compute a new combined area for the child span".

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv3.0

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

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv3.1

Oops, I forgot to change nsTextFrameThebes, it should also check the line-through area for very small font size.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv3.1

Sorry for the spam...

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

oops, I forgot to change the IID of nsIDeviceContext, it will be changed next patch (after review).

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

roc:

Would you review this?

Comment 30

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

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.

Comment 31

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

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?

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

Comment 33

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

Yes, we do need to reflow to recompute overflow, unfortunately.

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv4.0

This patch checking the nsIFrame::mState for IME/spellchecking.
I'm not sure that the params of FrameNeedsReflow is correct, please check it.

Comment 35

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

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.

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv4.1

updating for comment 35.

Comment 37

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

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

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

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

Comment on attachment 290868 [details] [diff] [review]
Patch rv4.1

OK add a comment explaining why no rounding is needed.

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

checked-in.

-> FIXED

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv4.1.1 (for checked-in)

Comment 42

[Ed Lee :Mardak]

Could this have regressed bug 406729?

Comment 43

[Dan Mosedale (:dmosedale, :dmose)]

Backed out at the request of roc & mconnor; reopening.

Comment 44

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

The cause of bug 406642 is that FAYT uses nsTextFrame::GetPointFromOffset during dirty via Scroll. This issue should be fixed in FAYT? or nsTextFrame?

Comment 45

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv4.2

This patch fixes bug 406642. But this patch might need the patch of bug 402524.

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 291844 [details] [diff] [review]
Patch rv4.2

Umm.... The some other points need the same change...

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Additional patch rv1.0

This is only additional part after rv4.1.1. These points are changing the selection range before ScrollSelectionIntoView.

Comment 48

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

I confirmed that the patch of bug 402524 fixes bug 406729, so, we can reland the patch of this bug after bug 402524.

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv4.2 (for re-check-in)

updating for latest trunk and merged the additional patch.

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv4.2 (for re-check-in)

oops, this is correct, sorry for the spam.

Comment 51

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Patch rv4.2 (for re-check-in)

gfx part was forgotten in previous patch...

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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.

Comment 53

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

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.

Comment 54

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

bug 402524 is fixed now, I'll land this after some hours if there are no problem in bug 402524.

Comment 55

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

checked-in.

Comment 56

[Reed Loden [:reed]]

This was a ~4ms Ts regression on Linux at least.

Comment 57

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

Comment 58

[Steve England [:stevee]]

I believe this has caused bug 419531

Comment 59

[Rolf Bode-Meyer]

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.