290920 - If an element has 'font-variant: small-caps', the selection color is broken

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect, P1)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This is regression of bug 181336.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv3.0

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv3.1

Thank you.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 181300 [details] [diff] [review]
Patch rv3.1

The risk is low.

Comment 12

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

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

Comment 13

[Hideo Saito]

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,

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in.

Comment 16

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

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.

Comment 17

[Hideo Saito]

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

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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