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.

er, the testcase is for windows.

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

Is this a regression?

maybe, but I'm not sure. I cannot reproduce this bug on Fx2 with the testcase.

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.

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.

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

o.k. this patch fixes this bug.

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

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

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.

sorry for the spam.

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.

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

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

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

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

Sorry for the spam...

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.

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

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.

updating for comment 35.

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

checked-in. -> FIXED

Could this have regressed bug 406729?

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

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

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

Comment on attachment 291844 [details] [diff] [review] Patch rv4.2 Umm.... The some other points need the same change...

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

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

updating for latest trunk and merged the additional patch.

oops, this is correct, sorry for the spam.

gfx part was forgotten in previous patch...

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.

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.

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

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.