See https://bugzilla.mozilla.org/attachment.cgi?id=180877, and select the text. If you select the two or more words, the selection color is broken.

This is regression of bug 181336.

*** Bug 235101 has been marked as a duplicate of this bug. ***

So this fixes the issue I commented on in bug 181336 comment 10 as well, right?

Comment on attachment 181120 [details] [diff] [review] Patch rv1.0 So.. why is this the right change? What is it actually doing? Whatever the answer is, you may want to document it in the code right about where you call GetColor() on the rendering context, I think.

Er... _that_ much of the explanation is clear enough from the code. Let me see if I understand what's going on. The color in aTextStyle.mColor>mColor is just the style color for the text. The color we actually want to be using is not actually explicitly passed to this function; instead it's set on the rendering context before the function is called. Right? If that's the case, then the general idea looks ok, though the place where our function is declared needs some documentation to explain what's going on. Also, there's no need to change the color before calling PaintTextDecorations, since that method sets colors as needed (this should probably be documented). Finally, the checks for textColor against aTextStyle.mColor>mColor are wrong, since PaintTextDecorations will set the current rendering context color to various things that are not so related to either of those two colors. Just set the color to the text color unconditionally when painting text.

Sorry. I have two mistakes. 1. You're right. The "if" check is wrong. I removed. 2. The color setting before calling |PaintTextDecoration| is not necessary. I couldn't understand bug 181336's problem. Therefore, I remove it too.

So... how about just having that comment say: // Save the color we want to use for the text, since calls to // PaintTextDecorations in this method will call SetColor() on the rendering // context. and document where RenderString is declared something like: // The passed-in rendering context must have its color set to the color the // text should be rendered in.

Thank you.

Comment on attachment 181300 [details] [diff] [review] Patch rv3.1 The risk is low.

Comment on attachment 181300 [details] [diff] [review] Patch rv3.1 a=dbaron, but this patch seems to me like an odd approach -- shouldn't PaintTextDecorations be responsible for leaving the rendering context in the same state that it started in (i.e., either PushState/PopState or preferably (for performance) GetColor/SetColor)?

I think that the color using on RenderString should be saved by PaintTextDecorations as follows. --- ./layout/generic/nsTextFrame.cpp2 Thu Apr 21 12:22:08 2005 +++ ./layout/generic/nsTextFrame.cpp Thu Apr 21 15:20:23 2005 @@ -1896,16 +1896,18 @@ nsTextFrame::PaintTextDecorations(nsIRen PRUint32 aIndex, /*= 0*/ PRUint32 aLength, /*= 0*/ const nscoord* aSpacing /* = nsnull*/ ) { // Quirks mode text decoration are rendered by children; see bug 1777 // In non-quirks mode, nsHTMLContainer::Paint and nsBlockFrame::Paint // does the painting of text decorations. + nscolor currentColor; + aRenderingContext.GetColor(currentColor); if (eCompatibility_NavQuirks == aPresContext->CompatibilityMode()) { nscolor overColor, underColor, strikeColor; PRBool useOverride = PR_FALSE; nscolor overrideColor; PRUint8 decorations = NS_STYLE_TEXT_DECORATION_NONE; // A mask of all possible decorations. @@ -2108,16 +2110,17 @@ nsTextFrame::PaintTextDecorations(nsIRen break; } } } aDetails = aDetails->mNext; } } + aRenderingContext.SetColor(currentColor); } nsresult nsTextFrame::GetContentAndOffsetsForSelection(nsPresContext *aPresContext, nsIContent **aContent, PRInt32 *aOffset, PRInt32 *aLength) { if (!aContent || !aOffset || !aLength) @@ -2898,17 +2901,16 @@ nsTextFrame::RenderString(nsIRenderingCo < (PRUint32)aTextStyle.mNumJustifiableCharacterReceivingExtraJot) { glyphWidth++; } } if (nextFont != lastFont) { pendingCount = bp - runStart; if (0 != pendingCount) { // Measure previous run of characters using the previous font - aRenderingContext.SetColor(aTextStyle.mColor->mColor); aRenderingContext.DrawString(runStart, pendingCount, aX, aY + mAscent, -1, spacing ? sp0 : nsnull); // Note: use aY not small-y so that decorations are drawn with // respect to the normal-font not the current font. PaintTextDecorations(aRenderingContext, aStyleContext, aPresContext, aTextStyle, aX, aY, width, runStart, aDetails,

I don't think so. I think that the caller of Gfx functions should initialize the context itself when it is necessary. If it isn't so, we need to do unnecessary color setting. I.e, final color setting in PaintTextDecoration is not necessary.

checked-in.

I much prefer comment 13's approach to what was checked in, if only because it's much easier to understand and less likely to cause future bugs -- since, frankly, it's not obvious to somebody who doesn't know the ugly details of 'text-decoration' in CSS that PaintTextDecorations would have to set a color at all.

Masayuki, is it right behavior that the change of the color by PaintTextDecorations influences subsequent drawing?

I think so. For to inhibit the future bugs, I think that your opinion is better approach than my patch. But the approach is not existing on other functions in nsTextFrame.cpp. In other code, when we draw something, we MUST set the color and other settings. I added same behavior in RenderString by my patch.

> But the approach is not existing on other functions in nsTextFrame.cpp. nsTextFrame.cpp is generally a cess-pit, yes. I'd also prefer the approach that David suggested.

*** Bug 298735 has been marked as a duplicate of this bug. ***