Closed Bug 290920 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: Selection, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta2

People

(Reporter: masayuki, Assigned: masayuki)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta3
*** Bug 235101 has been marked as a duplicate of this bug. ***
Attached patch Patch rv1.0 (obsolete) — Splinter Review
Attachment #181120 - Flags: superreview?(bzbarsky)
Attachment #181120 - Flags: review?(bzbarsky)
So this fixes the issue I commented on in bug 181336 comment 10 as well, right?
Blocks: 181336
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.
Attachment #181120 - Flags: superreview?(bzbarsky)
Attachment #181120 - Flags: review?(bzbarsky)
Attached patch Patch rv2.0 (obsolete) — Splinter Review
Attachment #181120 - Attachment is obsolete: true
Attachment #181279 - Flags: superreview?(bzbarsky)
Attachment #181279 - Flags: review?(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.
Attachment #181279 - Flags: superreview?(bzbarsky)
Attachment #181279 - Flags: superreview-
Attachment #181279 - Flags: review?(bzbarsky)
Attachment #181279 - Flags: review-
Attached patch Patch rv3.0 (obsolete) — Splinter Review
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.
Attachment #181279 - Attachment is obsolete: true
Attachment #181296 - Flags: superreview?(bzbarsky)
Attachment #181296 - Flags: review?(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.
Attachment #181296 - Attachment is obsolete: true
Attachment #181300 - Flags: superreview?(bzbarsky)
Attachment #181300 - Flags: review?(bzbarsky)
Attachment #181296 - Flags: superreview?(bzbarsky)
Attachment #181296 - Flags: review?(bzbarsky)
Attachment #181300 - Flags: superreview?(bzbarsky)
Attachment #181300 - Flags: superreview+
Attachment #181300 - Flags: review?(bzbarsky)
Attachment #181300 - Flags: review+
Comment on attachment 181300 [details] [diff] [review]
Patch rv3.1

The risk is low.
Attachment #181300 - Flags: approval1.8b2?
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)?
Attachment #181300 - Flags: approval1.8b2? → approval1.8b2+
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.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.8beta3 → mozilla1.8beta2
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. ***
You need to log in before you can comment on or make changes to this bug.