403524 - implement CSS2.1 text-decoration rules for both quirks and standards modes

Status: RESOLVED FIXED

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

Description

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

The last draft or two of CSS2.1 has had new rules for text-decoration that should allow us to combine our quirks- and standards-mode implementations of text-decoration.  Our standards-mode implementation follows the old spec; the spec has now been modified enough towards our quirks mode implementation that we ought to be able to implement the spec for all modes.  If not, the spec should probably be tweaked further.

Comment 1

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

Attachment: example showing a few of the major differences (quirks mode)

Comment 2

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

Attachment: example showing a few of the major differences (standards mode)

Comment 3

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

In a little bit more detail:
 * the backwards-compatible (Nav4) way of drawing text-decoration treated it a lot like an inherited property.  It was drawn only on text, based on the font metrics of that text
 * the CSS1 and CSS2 specifications defined text-decoration as something associated with an inline element.  It is drawn over the contents of the inline element (across non-text items, assuming there was some text) based on the font properties and color of that element.

CSS 2.1 has now changed on the issue of whether to draw across non-text.  (Which is good, since CSS2's rule violated a bunch of incrementality invariants by saying that no decorations were drawn when there was no text at all, but that adding text made the decoration drawn across everything.)  It still has the rule regarding consistency of the color, thickness, and position of the underline (although it's no longer as precise about using the font metrics and color of the inline rather than mixing metrics -- although it's certainly not describing using the metrics/color of each piece of text as the quirks mode rules do).

The testcases I just added don't go into any issues about whether text-decoration is propagated into various types of descendant blocks; there's actually a separate bug (bug 403526) covering at least part of that issue.  CSS 2.1 has also changed things a good bit here as well.

Compare:
http://www.w3.org/TR/1998/REC-CSS2-19980512/text.html#propdef-text-decoration
http://www.w3.org/TR/CSS21/text.html#propdef-text-decoration

The z-ordering rules described in CSS 2.1:
http://www.w3.org/TR/CSS21/zindex.html
actually seem to mandate an implementation that looks a lot more like our current quirks mode text-decoration implementation than our current standards-mode implementation.

Comment 4

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

The current standards-mode text-decoration implementation lives in nsHTMLContainerFrame.cpp (and the virtual function PaintTextDecorationLine, which is overridden for nsBlockFrame).  There may also be a little interaction with the nsIFrame::Compute(|Simple)TightBounds code (which might be ok as standards-mode only if it's only needed for MathML, at least for now).

The current quirks-mode implementation lives in nsTextFrameThebes.cpp.  (The overflow handling addid in bug 392785 is actually spread into nsLineLayout, and looks like it's quirks-mode only, although that might be a bug.)

Comment 5

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

Thanks! This would be great to do. Maybe Michael can do it, since it would simplify the text-shadow patch quite a lot.

I guess we just need to extend our quirks-mode implementation to search its ancestors for decorations and draw them using thicknesses and offsets obtained from the ancestors. So struct TextDecorations would need to have offsets and thicknesses added to it.

Comment 6

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

(In reply to comment #4)
> The overflow handling addid in bug 392785 is actually spread into nsLineLayout,
> and looks like it's quirks-mode only, although that might be a bug.

Don't move the method from nsLineLayout. I'll use it from nsHTMLContainerFrame when I work on CSS3 text-decoration.

Comment 7

[Michael Ventnor]

Not sure of a good way to do this.

My thinking was to make a TextDecoration struct in the style context that has the text-decoration rule, and use the font metrics of the first text frame we encounter within that style context for offsets and thickness. Although I'm scared that the font can be changed in that text frame without deleting and creating a new nsStyleTextReset of distant ancestors.

Comment 8

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

Most of the implementation here belongs in layout, not the style system.  In fact, I think most of what's needed is already there, except for getting the metrics and drawing more than one of the same decoration, but I think we're already walking up the frame tree to find the decorations.

Comment 9

[Michael Ventnor]

(In reply to comment #8)
> Most of the implementation here belongs in layout, not the style system.  In
> fact, I think most of what's needed is already there, except for getting the
> metrics and drawing more than one of the same decoration, but I think we're
> already walking up the frame tree to find the decorations.
> 

We read properties from the style system by walking up the tree, I was considering storing more properties in the nsStyleTextReset. The actual drawing and other stuff would stay in nsTextFrameThebes. But I want to know if I'd have a problem as in comment 7.

Comment 10

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

nsStyleTextReset structs are shared in a bunch of ways; you really don't want to store extra stuff in them that can't be derived directly from the style data.

Comment 11

[Michael Ventnor]

"In determining the position of and thickness of text decoration lines, user agents may consider the font sizes of and dominant baselines of descendants, but must use the same baseline and thickness on each line."

We put the baseline at the same y position on each line by default, right?

With our current nsTextFrameThebes infrastructure, compliance with this spec sounds very difficult unless I'm thinking about too much at once.

(In reply to comment #10)
> nsStyleTextReset structs are shared in a bunch of ways; you really don't want
> to store extra stuff in them that can't be derived directly from the style
> data. 

We need to share info between nsTextFrameThebes's (such as baseline and decoration line thickness) whose only thing they share in common is the ancestor.

Comment 12

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

We *already* walk up the tree to find decorations to paint.  So we already have access to the frame that has the decoration specified.

(In reply to comment #11)
> "In determining the position of and thickness of text decoration lines, user
> agents may consider the font sizes of and dominant baselines of descendants,
> but must use the same baseline and thickness on each line."

That's a "may" (something that's optional) -- we don't need to do that in the first pass -- we can do what we do now in standards mode as a start, which is just consider the metrics for the element that the text-decoration is on.

Or, we can do something fancier, and build up information about the descendants being decorated, and cache that as a property of the frame.  Then we can walk up the tree to the element with text-decoration and look at (or build if it's not there) the structure in the frame property.  (But we'd need to figure out when it would need to be rebuilt.  So I don't suggest doing this as part of the initial pass, probably.)

Comment 13

[Michael Ventnor]

Hmm, looking at the code again, there were a lot of things I did miss...
Is there a way to go from an nsStyleContext to an nsIFrame, though? I'd need that to get font metrics.

If not, can I change the for loop from walking nsStyleContexts to walking nsIFrames and getting the nsStyleContexts from them each time? Or is that assumption wrong somehow?

Comment 14

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

I think you can walk up the frames.

Comment 15

[Michael Ventnor]

"It is not, however, further propagated to floating and absolutely positioned descendants, nor to the contents of 'inline-table' and 'inline-block' descendants."

Is there any way of doing this without relying on nsTextFrameThebes? I tried to enforce this but I ended up losing every underline, including the default ones on links.

Comment 16

[Michael Ventnor]

Attachment: Patch

This is what I have so far. I'll try to continue working on this but real life is showing signs of wanting more of my time, plus I'd hate to bear the enormous amount of regressions this is bound to cause :)

Comment 17

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

(In reply to comment #16)
> Created an attachment (id=321934) [details]
> Patch

Looks like the patch only draws innermost decoration lines. I.e., the nested decoration lines of standards mode cannot be drawn, right?

And the textframe that will draw the decoration lines does not have the drawing areas for decoration lines on its overflow area. So, I think that the redrawing of decoration lines will fail at some cases.

And the textframe always uses the wrong underline offset at painting (that was computed for the fontgroup of the textframe). You need to store the gfxFontGroup on the TextDecorations instead of first font mectrics for *every* decoration lines.

Comment 18

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

And the baseline position (ascent) of the decoration line owner frame and the y distance between the textframe and the owner frame are needed for vertical-align, I think.

Comment 19

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

Attachment: complex testcase for standards mode

Comment 20

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

Maybe, the nsHTMLContainerFrame should continue to handle the decoration lines painting events, and then nsHTMLContainerFrame should call the PaintTextDecoration like method of the children frames. And if the method has the needed decoration metrics in the args, the textframes don't need to walk up the tree for all decoration lines.

Comment 21

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

I think we can do it all from the text frame, we just need to be able to paint an unlimited number of text decorations there. Here's what I think needs to be done:

-- break PaintTextWithSelectionColors into PaintSelectionBackgrounds and PaintTextWithSelectionColors (so we can paint selection backgrounds, then underlines/overlines, *then* the text, then the strikethroughs)

-- make TextDecorations contain three arrays of decoration objects --- one for overlines, one for underlines, and one for strike-throughs. Each decoration object needs a color, an offset, an ascent, and a thickness (as currently computed by nsTextFrame::PaintTextDecorations). GetTextDecorations will walk ancestors and create all the decoration objects using the ancestor frame metrics and font metrics.

-- make PaintTextDecorations take an array of decoration objects and also an order to traverse the array in

-- in the general case, call PaintSelectionBackgrounds, then PaintTextDecorations to paint the underlines array, then PaintTextDecorations to paint the overlines array, then PaintTextWithSelectionColors, then PaintTextDecorations to paint the line-throughs array.

We could avoid creating arrays of decoration objects, by having PaintTextDecorations traverse the ancestor chain itself, but painting text is performance sensitive so it's probably a good idea to just traverse the ancestors once instead of three times.

Comment 22

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

(In reply to comment #21)
> I think we can do it all from the text frame, we just need to be able to paint
> an unlimited number of text decorations there. Here's what I think needs to be
> done:

This sounds good to me.

One question is whether we want to change decoration colors to match the selection foreground color for selected text.  It looks like right now we draw selection on top of decorations, which seems wrong, but avoids the issue.

Comment 23

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

I think we probably shouldn't change the selection colors, but if we wanted to, it would be easy to implement in the above strategy.

Comment 24

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

I have a question, if we do it all from the textframe, the textframe has all ancestor's decoration lines drawing area on its overflow rect?

Comment 25

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

hmm, yes...

Comment 26

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

I think that the textframe should not have all decoration lines drawing area in its overflow rect. It seems that it is difficult and risky thing... (E.g., the outline rect is also changed.) I still think that the redrawing event should be handled in nsHTMLContainerFrame and it calls the descendant's decoration painting method. I think that the patch becomes simple by this approach.

Comment 27

[Michael Ventnor]

Attachment: Patch 2

I managed to do the textframe approach, I actually think this way is simpler. It also works very well, all mochitests pass, and I managed to remove a lot of code from nsHTMLContainerFrame (MathML still requires the presence of some functions) and nsBlockFrame.

One tiny hitch I'm still wondering about, I fail one of the tests here:
http://www.w3.org/Style/CSS/Test/CSS2.1/current/html4/t1504-c543-txt-decor-00-d-g.htm

The first one that says there should be no red. Any hints on why that is happening?

Comment 28

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

(In reply to comment #26)
> I think that the textframe should not have all decoration lines drawing area in
> its overflow rect. It seems that it is difficult and risky thing... (E.g., the
> outline rect is also changed.)

I think that's OK.

Comment 29

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

+    // The spec (all hail the spec) says that text-decoration cannot propagate
+    // to within floating, absolutely positioned, inline-block or inline-table
+    // elements.
+    // If the current frame matches this, bail so we don't inherit our ancestor's
+    // decorations if any.
+    const nsStyleDisplay* styleDisplay = decorContext->GetStyleDisplay();
+    if (styleDisplay->IsFloating() ||
+        styleDisplay->IsAbsolutelyPositioned() ||
+        styleDisplay->mDisplay == NS_STYLE_DISPLAY_INLINE_BLOCK ||
+        styleDisplay->mDisplay == NS_STYLE_DISPLAY_INLINE_TABLE)
+      break;

Shouldn't we stop for non-inline elements too?

+  nsRect decorationArea;
+  for (PRUint32 i = 0; i < decorations.mOverlines.Length(); i++) {
+    gfxSize size(aPresContext->AppUnitsToGfxUnits(aOverflowRect->width),
+                 decorations.mOverlines[i].mSize);
+
+    decorationArea =
+      nsCSSRendering::GetTextDecorationRect(aPresContext, size,
+                        decorations.mOverlines[i].mFontMaxAscent,
+                        decorations.mOverlines[i].mOffset,
+                        NS_STYLE_TEXT_DECORATION_OVERLINE,
+                        decorations.mOverlines[i].mLineStyle);
+    aOverflowRect->UnionRect(*aOverflowRect, decorationArea);
+  }

This loop can be factored out into its own function and used three times. You'd want to pass PR_MAX(ratio, 1.0f) in as a parameter to multiply the height by.

I think it would be helpful to follow comment #21 and call GetTextDecorations outside of PaintTextDecorations, and have PaintTextDecorations just paint one kind of decorations (taking an array of TextDecorationLines).

+  PaintTextDecorations(aCtx, aDirtyRect, aFramePt, aTextBaselinePt,
+                       aTextPaintStyle, aProvider,
+                       NS_STYLE_TEXT_DECORATION_UNDERLINE |
+                       NS_STYLE_TEXT_DECORATION_OVERLINE);

Then this would just call PaintTextDecorations twice.

+      PRUint8  mLineStyle;

You don't seem to use this, and you don't need it.

Comment 30

[Michael Ventnor]

(In reply to comment #29) 
> +      PRUint8  mLineStyle;
> 
> You don't seem to use this, and you don't need it.
> 

I would like to keep this to ease implementation of CSS3 text-decoration-style.

Comment 31

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

(In reply to comment #27)
> One tiny hitch I'm still wondering about, I fail one of the tests here:
> http://www.w3.org/Style/CSS/Test/CSS2.1/current/html4/t1504-c543-txt-decor-00-d-g.htm
> 
> The first one that says there should be no red. Any hints on why that is
> happening?

maybe because you're missing the code to do the equivalent of this:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsHTMLContainerFrame.cpp&rev=3.238&mark=322-324#322

Comment 32

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

Oh, you're changing the block propagation stuff in this patch too?  There was separate work on that in bug 403526.

Comment 33

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

Hmm, this overflow area issue is a problem :-(.

We would need additional code to expand the ascent and descent of the textframe to include the ascent and descent for the fonts of all frames that we could collect text-decorations from. That might not be too bad, though.

Comment 34

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

That said, I guess you have to change it here... but you might want to see that bug for what we ought to be doing.  (I haven't compared the details.)

Comment 35

[Michael Ventnor]

(In reply to comment #33)
> Hmm, this overflow area issue is a problem :-(.
> 
> We would need additional code to expand the ascent and descent of the textframe
> to include the ascent and descent for the fonts of all frames that we could
> collect text-decorations from. That might not be too bad, though.
> 

We store the ascent in the TextDecorationLine class, how is descent a problem related to text-decoration?

Comment 36

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

We need to actually change the ascent and descent of the textframe so it includes the ascent and descent of each ancestor. Otherwise, adding an underline to one of those ancestors would draw that underline in the textframe so we'd need to reflow the textframe to ensure the underline was included in its overflow area. Reflowing things every time you hover a link turns out to be bad.

Comment 37

[Michael Ventnor]

(In reply to comment #36)
> We need to actually change the ascent and descent of the textframe so it
> includes the ascent and descent of each ancestor. Otherwise, adding an
> underline to one of those ancestors would draw that underline in the textframe
> so we'd need to reflow the textframe to ensure the underline was included in
> its overflow area. Reflowing things every time you hover a link turns out to be
> bad.
> 

mAscent is stored in the textframe, where is the descent used?

Can I store the max descent of the font group that has the text-decoration (like how I do with maxAscent) or do I need to compare every ancestor's font group's maxDescent and take whatever the maximum one is?

Comment 38

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

Hmm.  That's potentially a lot of overflow that we might not want to have, since it would mean that any block inside a block with larger text would have to report all of its text frames as overflowing.

I wonder if we should probably add a way of updating overflow areas separate from reflow.  (I want that for left/top optimizations too.)

Comment 39

[Michael Ventnor]

(In reply to comment #38)
> Hmm.  That's potentially a lot of overflow that we might not want to have,
> since it would mean that any block inside a block with larger text would have
> to report all of its text frames as overflowing.

But since this is only the case where text-decoration is in use, would it impact performance a lot?

Comment 40

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

Yeah, it sucks.

The problem is that updating overflow areas can, in general, cause reflow. (E.g. update overflow area -> horizontal scrollbar required -> auto-height overflow:auto element's height increases).

Maybe we could implement it to propagate overflow area up and if we discovered an overflow:auto element is affected, then and only then do a reflow. It would be great if we could do this.

Comment 41

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

(In reply to comment #39)
> But since this is only the case where text-decoration is in use, would it
> impact performance a lot?

I think roc's saying that we want it all the time, since we don't want dynamic changes of text-decoration (which people do on :hover) to cause reflow.

Comment 42

[Michael Ventnor]

OK roc, you've lost me.

I don't know what Reflow is about at all.

Comment 43

[Michael Ventnor]

Plus, how do we control when Reflow happens and when it doesn't?

Comment 44

[Michael Ventnor]

Attachment: Patch 3

OK, so for the past few hours I've just been polishing up the existing code, fixing test suite failures here and there. This patch should now completely conform to CSS2.1 text-decoration AFAIK from W3's test suite.

roc, in your infinite wisdom, can you please guide me as to what exactly I should be doing next, if anything?

Comment 45

[Michael Ventnor]

Attachment: Patch 3.1

After playing around with quite a few self-made minimal testcases, and a couple of a:hover tutorial sites, I haven't yet been able to trigger a reflow by changing the text-decoration. Can anyone give a testcase or an example which causes what everyone is worried about?

This patch fixes a bug in patch 3 that caused inheritance problems in some cases. I did have to keep it to only checking for INLINE_BLOCK and INLINE_TABLE since that's what the spec specifies and checking more inline stuff does cause problems. (<a> within a <div>)

Comment 46

[Jeff Walden [:Waldo]]

(In reply to comment #44)
> please guide me as to what exactly I should be doing next, if anything?

Reftests!  You appear to have changed a fairly large amount of code, fixed a number of bugs, and modified a bunch of behavior, but you don't have any tests to demonstrate that your changes are correct and that you've hit all the corner cases.

Comment 47

[Michael Ventnor]

(In reply to comment #46)
> (In reply to comment #44)
> > please guide me as to what exactly I should be doing next, if anything?
> 
> Reftests!  You appear to have changed a fairly large amount of code, fixed a
> number of bugs, and modified a bunch of behavior, but you don't have any tests
> to demonstrate that your changes are correct and that you've hit all the corner
> cases.
> 

I was meaning in the case of speed optimizations, but I will get to that.

Note that I just tested some popular sites as well and still can't trigger a reflow from changing the text-decoration.

Comment 48

[Michael Ventnor]

Attachment: Patch 3.2

Pass a reference instead of the actual array; save some time and memory.

Comment 49

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

Comment on attachment 322090 [details] [diff] [review]
Patch 3.2

> @@ -3688,64 +3732,57 @@ nsTextFrame::UnionTextDecorationOverflow
>  
>    // When this frame is not selected, the text-decoration area must be in
>    // frame bounds.
>    float ratio;
>    if (!(GetStateBits() & NS_FRAME_SELECTED_CONTENT) ||
>        !HasSelectionOverflowingDecorations(aPresContext, &ratio))
>      return;
>  
> -  nsLineLayout::CombineTextDecorations(aPresContext,
> -                  NS_STYLE_TEXT_DECORATION_UNDERLINE,
> -                  this, *aOverflowRect, mAscent, ratio);
> +  // Union for every text decoration we have including our ancestors'.
> +  TextDecorations decorations = GetTextDecorations(aPresContext);
> +
> +  UnionTextDecorationAncestry(aPresContext, decorations.mUnderlines,
> +                              NS_STYLE_TEXT_DECORATION_UNDERLINE,
> +                              PR_MAX(ratio, 1.0f), aOverflowRect);
> +  UnionTextDecorationAncestry(aPresContext, decorations.mOverlines,
> +                              NS_STYLE_TEXT_DECORATION_OVERLINE,
> +                              1.0f, aOverflowRect);
> +  UnionTextDecorationAncestry(aPresContext, decorations.mLineThroughs,
> +                              NS_STYLE_TEXT_DECORATION_LINE_THROUGH,
> +                              1.0f, aOverflowRect);

This is wrong. You should remove the selection checking in your patch. The decoration lines of ancestors may overflow without selection state of the textframe.

But you don't set TEXT_SELECTION_UNDERLINE_OVERFLOWED if the textframe is not selected. That is only needed for the selection decoration lines (i.e., spellchecker and IME).

 void
+nsTextFrame::UnionTextDecorationAncestry(nsPresContext* aPresContext,
+                                         nsTArray<TextDecorationLine> aDecorations,
+                                         PRUint8 aDecorationType,
+                                         float aSizeMultiplier,
+                                         nsRect* aOverflowRect)
+{
+  nsRect decorationArea;
+  for (PRUint32 i = 0; i < aDecorations.Length(); i++) {
+    gfxSize size(aPresContext->AppUnitsToGfxUnits(aOverflowRect->width),
+                 aDecorations[i].mSize * aSizeMultiplier);
+
+    decorationArea =
+      nsCSSRendering::GetTextDecorationRect(aPresContext, size,
+                        aDecorations[i].mFontMaxAscent,
+                        aDecorations[i].mOffset,
+                        aDecorationType,
+                        aDecorations[i].mLineStyle);
+    aOverflowRect->UnionRect(*aOverflowRect, decorationArea);
+  }
+}

This patch renders the my testcase correctly?? |aDecorations[i].mFontMaxAscent| doesn't prefer the y position difference between the textframe and the decoration line owner frame. Then, I think this patch fails to draw the ancestor's decoration lines when the textframe baseline is not same as the baseline of the owner of the decoration lines. I think that you should add a member to TextDecorationLine. That value is offset from the baseline of the owner of the decoration lines to the baseline of the textframe.

Comment 50

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

And you remove the decoration line painting code from nsHTMLContainerFrame, you can move |nsLineLayout::CombineTextDecorations| to nsTextFrameThebes.cpp.

Comment 51

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

(In reply to comment #49)
> (From update of attachment 322090 [details] [diff] [review])
> > @@ -3688,64 +3732,57 @@ nsTextFrame::UnionTextDecorationOverflow

> > -  nsLineLayout::CombineTextDecorations(aPresContext,
> > -                  NS_STYLE_TEXT_DECORATION_UNDERLINE,
> > -                  this, *aOverflowRect, mAscent, ratio);
> > +  UnionTextDecorationAncestry(aPresContext, decorations.mUnderlines,
> > +                              NS_STYLE_TEXT_DECORATION_UNDERLINE,
> > +                              PR_MAX(ratio, 1.0f), aOverflowRect);

Oh, and you don't use the ratio for the ancestor decoration lines. That is only applied to the underline for IME composition string. So, you should not kill the current code, and use 1.0f for the ratio on your adding code. I.e., the final code should be:

  float ratio;
  if ((GetStateBits() & NS_FRAME_SELECTED_CONTENT) &&
       HasSelectionOverflowingDecorations(aPresContext, &ratio)) {
     nsLineLayout::CombineTextDecorations(aPresContext,
                     NS_STYLE_TEXT_DECORATION_UNDERLINE,
                     this, *aOverflowRect, mAscent, ratio);
     AddStateBits(TEXT_SELECTION_UNDERLINE_OVERFLOWED);
   }

   // Union for every text decoration we have including our ancestors'.
   TextDecorations decorations = GetTextDecorations(aPresContext);

   UnionTextDecorationAncestry(aPresContext, decorations.mUnderlines,
                               NS_STYLE_TEXT_DECORATION_UNDERLINE,
                               1.0f, aOverflowRect);
   UnionTextDecorationAncestry(aPresContext, decorations.mOverlines,
                               NS_STYLE_TEXT_DECORATION_OVERLINE,
                               1.0f, aOverflowRect);
   UnionTextDecorationAncestry(aPresContext, decorations.mLineThroughs,
                               NS_STYLE_TEXT_DECORATION_LINE_THROUGH,
                               1.0f, aOverflowRect);
 }

Comment 52

[Michael Ventnor]

(In reply to comment #49)
> This patch renders the my testcase correctly?? |aDecorations[i].mFontMaxAscent|
> doesn't prefer the y position difference between the textframe and the
> decoration line owner frame. Then, I think this patch fails to draw the
> ancestor's decoration lines when the textframe baseline is not same as the
> baseline of the owner of the decoration lines. I think that you should add a
> member to TextDecorationLine. That value is offset from the baseline of the
> owner of the decoration lines to the baseline of the textframe.
> 

I suppose this all depends how this part of the spec is interpreted, although maybe I'm tired and the answer is right in front of me:
"Relatively positioning a descendant moves all text decorations affecting it along with the descendant's text; it does not affect calculation of the decoration's initial position on that line."

If we do want to do the same as nsHTMLContainerFrame, I'm not sure what properties I should store in the TextDecorationLine or how they can be gotten or used; but maybe the answer will come to me in the morning like other times :)

Comment 53

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

(In reply to comment #52)
> I suppose this all depends how this part of the spec is interpreted, although
> maybe I'm tired and the answer is right in front of me:
> "Relatively positioning a descendant moves all text decorations affecting it
> along with the descendant's text; it does not affect calculation of the
> decoration's initial position on that line."

I think we should not change the decoration line position by vertical-align. Because it breaks the rendering of |<u>2<sup>2</sup></u>|.

> If we do want to do the same as nsHTMLContainerFrame, I'm not sure what
> properties I should store in the TextDecorationLine or how they can be gotten
> or used; but maybe the answer will come to me in the morning like other times
> :)

I think that the decoration owner is inline frame, it's easy. However, I'm not sure what should be stored when the owner creates the new block context (i.e., the owner is block/inline-block etc...).

Comment 54

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

The problem isn't that you're causing reflows right now.  The problem is that in the case where you have a text-decoration (probably easiest to demonstrate for overline) on something with a large font, and something with a small font inside of it, the thing with a small font won't necessarily repaint because the overline is outside the area covered by the text frame.

That said, there are a whole bunch of things that make this less of a problem, but I'm not sure they do so completely:
 * you're likely to invalidate areas on both sides, which, if we're merging areas to be invalidated, is likely to invalidate the area you need
 * we're actually invalidating the ancestor with text-decoration, not the text frame itself.

But I bet you still have an invalidation bug on a testcase like:

<div style="margin-top:300px;text-decoration:overline;font-size:30px;width:100px">
text
<div style="font-size:12px;position:relative;top:-200px;left:100px">
bug here
</div>
text
</div>

Comment 55

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

(And the problem is that the way to fix this bug without doing reflows is something we may not really want to do.)

Comment 56

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

Oh, and the bug is in the case when the text-decoration style in the above example is removed/added dynamically, e.g., with a :hover selector.

Comment 57

[Michael Ventnor]

Does anyone know how to get the x and Y coordinates of a text frame? I've tried everything but I always get back (0, 0).

I'd rather just check in the text-shadow patch as is and leave this to someone with more layout experience...

Comment 58

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

Yeah, I think you're right. Let's move ahead on text-shadow without this.

Creating a way to change overflow rects without doing reflow is going to be tricky.

Comment 59

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

+  nsCOMPtr<nsIDeviceContext> dc;
+  aRenderingContext.GetDeviceContext(*getter_AddRefs(dc));

Looks good, but I think you could just get it via the prescontext.

Comment 60

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

Sorry, that was for another bug.

Comment 61

[Michael Ventnor]

Attachment: Patch 4

After adding quite a bit of ugliness to the code, I managed to get the X and Y of the text frame and fix Masayuki's testcase. Hopefully someone can pick this up later to solve the Reflow problem.

Comment 62

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

See also the work in bug 403526.

Comment 63

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

(In reply to comment #58)
> Creating a way to change overflow rects without doing reflow is going to be
> tricky.

Less tricky if we separate ink overflow and scrollable overflow and only change ink overflow in this case, though.

Comment 64

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

Since it may be relevant here:  the CSS WG just accepted the proposal in
http://lists.w3.org/Archives/Public/www-style/2010Aug/0334.html

Comment 65

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

After the patch that I'll attach to bug 572713 later, the work remaining to do for this bug will be:
 * fix the FIXMEs in that patch
 * remove the standards-mode text-decoration code and make the quirks mode code unconditional
 * check the CSS 2.1 tests and see if there's anything else

Comment 66

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

FYI:

http://www.w3.org/TR/css3-text/#text-decoration-skip

On CSS 3, the decoration lines may be painted with all inline objects. I think that the actual painting code should be in nsTextFrame for space skipping but the painting logic should be called from the nsHTMLContainerFrame and compute the line position by nsHTMLContainerFrame for <img> and other objects.

Comment 67

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

I think any values of text-decoration-skip that don't contain 'objects' are incompatible with the way CSS 2.1 specifies text-decoration (definitely in terms of z-ordering, and probably other aspects).

My preference is to move forward with this approach (which is incompatible with text-decoration-skip not containing 'objects'), but if that's a core feature that's going to stay in css3-text, then we need to revisit CSS 2.1's definition of text-decoration.

Comment 68

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

http://lists.w3.org/Archives/Public/www-style/2011May/0614.html

Comment 69

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

Two things from discussion today:

 * We should look at whether the NS_STYLE_TEXT_DECORATION_LINE_OVERRIDE_ALL stuff that the font element sets is still needed at all.  If it is, we should keep it quirks mode only because it's never been in standards mode before.  If it's not, we should just remove it.

 * After this lands, we probably want to have a look at drawing of decorations when text is selected.  We probably want the decorations to adopt the selection colors, but I don't think we do that now (though maybe we do in quirks mode, in which case we'd be in good shape).

Comment 70

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

Vitor has a working patch to fix the issues with the quirks mode code (per comment 65); hopefully he'll attach it here soon.

Comment 71

[Vitor Menezes]

Attachment: fix line positioning in quirks mode code and set overflow areas to match

Comment 72

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

Comment on attachment 539850 [details] [diff] [review]
fix line positioning in quirks mode code and set overflow areas to match

What happened to the patch header from your patch queue?  Was this
not the file in your patch queue?  It's better to attach that.  (And
it should have a commit message in it.)

There are a whole bunch of places in the patch where one of the editors
you're using inserted carriage returns; the line endings should just
be LF and not CR-LF.

There are also a bunch of places in the patch where you use hard tabs,
but you shouldn't.  (In a few of them I comment below on the indentation
being strange, and then I realized later that the problem was tabs,
which I think your editor shows as an indent to the next 4-space
boundary, but mine is showing as 8.)

nsLineLayout.cpp
================

The comment should be indented one more character, and then
wrapped || line should be indented 3 less (to match the other
half of the ||).

nsTextFrame.h
=============

LineDecoration should have a comment explaining what mBaselineOffset
is an offset from, and in which direction (but see below, where I
suggest reversing its sign).

The LineDecoration() constructor shouldn't pass nscoords by reference;
pass by value is fine.

Also, in the same constructor, |const nsIFrame const*| should be
|const nsIFrame *const|.  As written, it's a duplicate const (I'm
surprised MSVC accepts that; gcc doesn't).

Also, both constructors of LineDecoration should initialize the
params in the order declared, since gcc warns when they're not in that
order.

Local style puts the : before member initializers before the
first one rather than on the line before, i.e.:

  : mFrame(frame),
    mStyle(style),

Also, local style names the arguments with an "a" prefix.

In LineDecoration::operator==, local style puts the && before the
break rather than after (and s/other/aOther/).

Also, why do you have default parameters to LineDecoration's constructor's
parameters?  It seems like the caller should be required to pass all
of them explicitly.

Also, in the operator== and the initializer list, it's probably
better to have one member per line; it makes more sense in terms
of |hg annotate| after the code gets edited a bit.


In |struct TextDecorations|, the three nsTArray<LineDecoration>
should probably all be nsAutoTArray<LineDecoration, 1>, which means we
won't heap-allocate until there are two.  Then you don't need
to initialize them at all in the constructor.


Finally, given that TextDecorations is a more complicated struct now,
I tend to think GetTextDecorations should no longer return it by value
but should instead be:
  void GetTextDecorations(nsPresContext* aPresContext,
                          TextDecorations& aTextDecorations);

nsTextFrameThebes.cpp
=====================

>-GetFontGroupForFrame(nsIFrame* aFrame,
>+GetFontGroupForFrame(const nsIFrame const* aFrame,

Better not to touch this, both because it's unrelated and because
it won't compile on gcc (see above).

>+  PRBool isChild; // ignored

Remove this variable; it's now unused.

>+  nsStyleContext* context;
>+  const nsStyleTextReset* styleText;

Declare these inside the loop, at first assignment.

>+  // We start at mAscent so that the loop below correctly snags
>+  // decorations with *no* offset for this frame.
>+  nscoord relPos(mAscent), posOffset(0);

I think the naming of these variables is confusing, both because
|relPos| seems to imply something about relative positioning (which
it mostly doesn't) and because it's not clear how they differ.

What you call relPos is the vector from the top of |f| to the baseline
of |this|, ignoring offsets that result from relative positioning.

What you call posOffset is the vector from the baseline of |this| to
the baseline that should be used for drawing the decorations that come
from |f| (which also ignores offsets that result from relative
positioning).

I think it would make things clearer if you:
  * make posOffset and LineDecoration::mBaselineOffset the negatives
    of what they currently are (a pretty straightforward
    transformation), so that all three of relPos, posOffset, and
    LineDecoration::mBaselineOffset are positive when above the
    baseline and negative when below (which is the reverse of the way
    we use coordinates generally, but I think makes sense for offsets
    from a baseline).
  * document both relPos and posOffset as being relative to the
    baseline of |this| with positive numbers meaning
    above-the-baseline
  * rename relPos to offsetToTop, and document it as the position of
    of the top of |f|
  * rename posOffset to offsetToBaseline, and document it as the
    position of the baseline of text decorations

>+  for (nsIFrame* f = this; f;
>+          f = nsLayoutUtils::GetParentOrPlaceholderFor(
>+                        aPresContext->FrameManager(), f))

Line up the "f =" with the "nsIFrame*" in the line above

>+      const nscolor color = useOverride ? overrideColor :
>+                                           context->GetVisitedDependentColor(
>+                              eCSSProperty_text_decoration_color);

It's probably better to format this as (: at start of line and
different indent):
       const nscolor color = useOverride ? overrideColor
         : context->GetVisitedDependentColor(
                      eCSSProperty_text_decoration_color);

>+        decorations.mUnderlines.AppendElement(
>+            nsTextFrame::LineDecoration(f, styleText->GetDecorationStyle(),
>+                                                               color, posOffset));

(3 times)
color should line up with |f|
(this is actually tabs)

>-    if (disp->mDisplay != NS_STYLE_DISPLAY_INLINE &&
>-        disp->IsInlineOutside()) {
>+
>+    if (disp->mDisplay != NS_STYLE_DISPLAY_INLINE && disp->IsInlineOutside()) {

Leave this as it was.

>-    } else {
>+    }
>       // In standards/almost-standards mode, if we're on an
>-      // absolutely-positioned element or a floating element, we're done.
>-      if (disp->IsFloating() || disp->IsAbsolutelyPositioned()) {
>-        break;
>-      }
>+    // absolutely-positioned element or a floating element, we're done.
>+    else if (disp->IsFloating() || disp->IsAbsolutelyPositioned()) {
>+      break;

Leave these as it was; I think it's clearer to have a straight if/else
for quirks vs. standards, and then have code inside each half.

>-  nsRect shadowRect =
>-    nsLayoutUtils::GetTextShadowRectsUnion(*aVisualOverflowRect, this);
>+  nsRect shadowRect = nsLayoutUtils::GetTextShadowRectsUnion(
>+                                                 *aVisualOverflowRect, this);

Leave this as it was; don't mess up blame.

>+  // CSS 2.1 (at least, our implementations of it) allows this frame to draw
>+  // its own decorations at virtually any y-offset, so we need maxima and minima
>+  // to reliably generate the rectangle for them

You should wrap this at under 80 characters.  Also, don't say
"(at least, our implementations of it)", but do mention that it's
because this frame draws decorations for its ancestors at their offsets.

>+    const nscoord selfWidth(GetSize().width);

Just call this width, and initialize with = rather than ().

>+    nscoord top(nscoord_MAX), bottom(nscoord_MIN),

Initialize with =

>+               underlineOffset, underlineSize,
>+            strikeoutOffset, strikeoutSize;

Move these into the relevant |for|.

>-  if (!(GetStateBits() & NS_FRAME_SELECTED_CONTENT) ||
>-      !CombineSelectionUnderlineRect(aPresContext, *aVisualOverflowRect))
>+  if (!(GetStateBits() & NS_FRAME_SELECTED_CONTENT)
>+      || !CombineSelectionUnderlineRect(aPresContext, *aVisualOverflowRect))

Leave this as it was.  (But you should remove |decorationRect|.)

And I think you should remove the 2-line comment right before it.

>+  for (size_t i = decorations.mUnderlines.Length(); i > 0; --i) {
>+    const LineDecoration& dec = decorations.mUnderlines[i - 1];

I think it's clearer to write (3 times):
  for (size_t i = decorations.mUnderlines.Length(); i-- != 0; ) {
    const LineDecoration& dec = decorations.mUnderlines[i];

>-  // Hide text decorations if we're currently hiding @font-face fallback text
>-  if (aProvider.GetFontGroup()->ShouldSkipDrawing())
>-    return;

I think it's better to leave this where it is.  We should skip the
drawing based on whether there's any text drawn.

>-  // XXX aFramePt is in AppUnits, shouldn't it be nsFloatPoint?

You should leave this comment rather than removing it.  (Also don't
reorder the declarations for no reason.

>+  gfxFontGroup* fontGroup;

Remove this (see below)

>+    fontGroup = GetFontGroupForFrame(dec.mFrame, nsnull);
>+
>+    // Hide text decorations if we're currently hiding @font-face fallback text
>+    if (!fontGroup || fontGroup->ShouldSkipDrawing() || !fontGroup->GetFontAt(0))
>+      break;
>+
>+    const gfxFont::Metrics metrics = GetFirstFontMetrics(fontGroup);

Given the suggestion above of leaving the ShouldSkipDrawing check above,
and that having bad rendering on out-of-memory isn't a problem (it's
better to have less code), and that you don't need to pass the defaulted
parameter, you should just simplify this to (3 times):
    const gfxFont::Metrics metrics =
      GetFirstFontMetrics(GetFontGroupForFrame(dec.mFrame));
except in the underline case you'll need:
    gfxFontGroup* fontGroup = GetFontGroupForFrame(dec.mFrame);
    const gfxFont::Metrics metrics = GetFirstFontMetrics(fontGroup);

>  * @param aBackground the background color to use, or RGBA(0,0,0,0) if no
>+ * background should be painted
>  * background should be painted

Don't double this line.

>-    selectedOffset =
>-        provider.GetStart().GetOriginalOffset() + provider.GetOriginalLength();
>+    selectedOffset = provider.GetStart().GetOriginalOffset() +
>+                                provider.GetOriginalLength();
>     // If we're at the end of a preformatted line which has a terminating
>     // linefeed, we want to reduce the offset by one to make sure that the
>     // selection is placed before the linefeed character.
>     if (GetStyleText()->NewlineIsSignificant() &&
>-        HasTerminalNewline()) {
>+        HasTerminalNewline())
>+    {

Leave these as-is.

>+#ifdef MOZ_MATHML
>   NS_ASSERTION(!(NS_REFLOW_CALC_BOUNDING_METRICS & aMetrics.mFlags),
>                "We shouldn't be passed NS_REFLOW_CALC_BOUNDING_METRICS anymore");
>+#endif

And this.

>-  UnionTextDecorationOverflow(presContext, provider, &aMetrics.VisualOverflow());
>+  // When we have text decorations, we don't need to compute their overflow now
>+  // because we're guaranteed to do it later
>+  // (see nsLineLayout::RelativePositionFrames)

So I know I suggested this... but I now realize it's wrong.

In particular, UnionTextDecorationOverflow does stuff in addition to
text-decorations, and nsLineLayout only checks HasTextDecorationLines.

So I'd suggest, instead:
 * rename UnionTextDecorationOverflow to UnionAdditionalOverflow
 * add a boolean parameter for whether to do text-decorations, and pass
   false from this caller and true from the other caller

This looks good, but review- since I want to look at the revised version
of the patch.  (review+ would mean that you could make the revisions and
then commit without my rereading it.)

I need to double-check two additional things that I haven't done yet:
 - I want to go through the position math in
   nsTextFrame::PaintTextDecorations a little more carefully, but I'm 
 - for text decoration on block of blocks, which font size should be used
   per spec, and do we follow that?

Additional issues to think about for later patches before removing
the standards mode code:
 - make sure not to set OVERRIDE_ALL outside of quirks mode
 - check floats/abspos/etc. propagation for quirks mode

Comment 73

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

FYI: our coding style guide is here:
https://developer.mozilla.org/En/Developer_Guide/Coding_Style

Comment 74

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

Hmm, looks like nsTextFrame::UnionTextDecorationOverflow() computes solid style text-decoration rect with very simple rule. Our text-decoration painting is more complicated for some bad fonts and CSS3 text-decoration-style. See nsCSSRendering::GetTextDecorationRect() and you should use this.

> nsTextFrame::PaintTextDecorations()
> for (size_t i = decorations.mUnderlines.Length(); i > 0; --i) {

You should use PRUint32 instead of size_t.

Comment 75

[Vitor Menezes]

Attachment: fix line positioning in quirks mode code and set overflow areas to match

Fix line positioning in quirks mode and correct overflow areas to match -- revision 2

Comment 76

[Vitor Menezes]

Attachment: fix line positioning in quirks mode code and set overflow areas to match

Updated to further comply with recommendations by dbaron.

Comment 77

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

Comment on attachment 540575 [details] [diff] [review]
fix line positioning in quirks mode code and set overflow areas to match

Here are comments based on going through my review comments from last time (but I haven't yet gone through the diffs between the previous patch and this one; that's next):

For a start, there are still three occurrences of hard tabs:

+                        aPresContext->FrameManager(), f))

-    nscoord fontAscent = fm->MaxAscent();
-    nscoord fontHeight = fm->MaxHeight();
+       const nscoord fontAscent = fm->MaxAscent();
+       const nscoord fontHeight = fm->MaxHeight();


>Revision 2 of patch for bug-403524

You probably want a commit message like (with the first bit on one line, since only the first line shows up in most tools):

>Fix text-decoration positioning in quirks mode and set overflow areas to match.  (Bug 403524)  r=dbaron
>
>Change the quirks mode text-decoration code (soon to be used for all
>modes) to follow CSS 2.1's rules for positioning of decoration lines.
>Decorations are now drawn at a constant vertical position established by
>the element creating the decoration, and more than one of the same type
>(underline, overline, line-through) of decoration are supported on the
>same piece of text.
>
>This means that text-decorations can now significantly overflow a text
>frame, since the vertical-alignment of the element with text-decoration
>may be substantially different from the vertical alignment of the text.
>Set overflow areas for text frames with text decorations in
>nsLineLayout::RelativePositionFrames since it must happen *after*
>vertical alignment is done, and when relative positioning data are
>consistent (nsIFrame::GetRelativeOffset matches the offset that has been
>applied).


In LineDecoration, it's probably worth swapping mColor and mStyle so
that the used portion of the structure is contiguous.  (This also
implies swapping the initializers.)

In LineDecoration::operator==, you still need to s/other/aOther/.

Same for this comment:
>Also, in the operator== and the initializer list, it's probably
>better to have one member per line; it makes more sense in terms
>of |hg annotate| after the code gets edited a bit.

In TextDecorations() this line:
>+      : mOverlines(), mUnderlines(), mStrikes()
is not needed.

In nsTextFrame::GetTextDecorations, the parameter should be aDecorations
rather than decorations.

+  // These represent the offset to f's top from our baseline in our coordinate
+  // space and the offset from our baseline to f's baseline in our coordinate
+  // space, respectively.
+  nscoord frameTopOffset = mAscent,
+          baselineOffset = 0;

This comment isn't quite true, since frameTopOffset is always
relative to |f|, but baselineOffset isn't once we've found a block.  See
my suggestions the last time around.

>     // In all modes, if we're on an inline-block or inline-table (or
>     // inline-stack, inline-box, inline-grid), we're done.
>-    const nsStyleDisplay *disp = context->GetStyleDisplay();
>+    const nsStyleDisplay *const disp = context->GetStyleDisplay();
>+
>     if (disp->mDisplay != NS_STYLE_DISPLAY_INLINE &&
>-        disp->IsInlineOutside()) {
>+        disp->IsInlineOutside())
>+    {
>       break;
>     }
> 
>     if (compatMode == eCompatibility_NavQuirks) {
>       // In quirks mode, if we're on an HTML table element, we're done.
>       if (f->GetContent()->IsHTML(nsGkAtoms::table)) {
>         break;
>       }
>     } else {
>       // In standards/almost-standards mode, if we're on an
>-      // absolutely-positioned element or a floating element, we're done.
>+    // absolutely-positioned element or a floating element, we're done.
>       if (disp->IsFloating() || disp->IsAbsolutelyPositioned()) {
>         break;
>       }

This stuff should all stay as it was; you're not making any substantive
changes to it, so you shouldn't change blame.

>+  nsFontMetrics* fm = aProvider.GetFontMetrics();
>+
>   if (IsFloatingFirstLetterChild()) {
>     // The underline/overline drawable area must be contained in the overflow
>     // rect when this is in floating first letter frame at *both* modes.
>-    nsFontMetrics* fm = aProvider.GetFontMetrics();
>-    nscoord fontAscent = fm->MaxAscent();
>-    nscoord fontHeight = fm->MaxHeight();
>-    nsRect fontRect(0, mAscent - fontAscent, GetSize().width, fontHeight);
>+       const nscoord fontAscent = fm->MaxAscent();
>+       const nscoord fontHeight = fm->MaxHeight();
>+    const nsRect fontRect(0, mAscent - fontAscent, GetSize().width, fontHeight);
>     aVisualOverflowRect->UnionRect(*aVisualOverflowRect, fontRect);
>   }

Likewise, leave this as it was, and declare a new |fm| variable
where you need it below.


Did you address the bit about handling wavy underline, etc.?


>   gfxFont::BoundingBoxType boundingBoxType = IsFloatingFirstLetterChild() ?
>                                                gfxFont::TIGHT_HINTED_OUTLINE_EXTENTS :
>                                                gfxFont::LOOSE_INK_EXTENTS;
>+
>   NS_ASSERTION(!(NS_REFLOW_CALC_BOUNDING_METRICS & aMetrics.mFlags),
>                "We shouldn't be passed NS_REFLOW_CALC_BOUNDING_METRICS anymore");

No need to touch this either.

Comment 78

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

There are also still a bunch of () initializations of nscoord in UnionAdditionalOverflow.

And if you put this bit back in the order it was, you'll get much less of a diff:

>+  const gfxFloat app = aTextPaintStyle.PresContext()->AppUnitsPerDevPixel();
>+
>+  // XXX aFramePt is in AppUnits, shouldn't it be nsFloatPoint?
>+  const nscoord baseline = aTextBaselinePt.y - mAscent;
>+  const gfxFloat ascent = gfxFloat(mAscent) / app;
>+  gfxSize size(GetRect().width / app, 0);
>+  gfxPoint pt(aFramePt.x / app, 0);
>+
>   // Hide text decorations if we're currently hiding @font-face fallback text
>   if (aProvider.GetFontGroup()->ShouldSkipDrawing())
>     return;
> 
>-  gfxFont* firstFont = aProvider.GetFontGroup()->GetFontAt(0);
>-  if (!firstFont)
>-    return; // OOM
>-  const gfxFont::Metrics& fontMetrics = firstFont->GetMetrics();
>-  gfxFloat app = aTextPaintStyle.PresContext()->AppUnitsPerDevPixel();
>-
>-  // XXX aFramePt is in AppUnits, shouldn't it be nsFloatPoint?
>-  gfxPoint pt(aFramePt.x / app, (aTextBaselinePt.y - mAscent) / app);
>-  gfxSize size(GetRect().width / app, 0);
>-  gfxFloat ascent = gfxFloat(mAscent) / app;
>-

(And my mention of wavy underline in the previous comment was a reference to comment 74.)

Comment 79

[Vitor Menezes]

(In reply to comment #78)
> There are also still a bunch of () initializations of nscoord in
> UnionAdditionalOverflow.
> 
> And if you put this bit back in the order it was, you'll get much less of a
> diff:
> ...
>

Unless I remove the consts, the diff remains despite identical ordering.

Comment 80

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

yeah, but much less != none :-)

Comment 81

[Vitor Menezes]

Attachment: fix line positioning in quirks mode code and set overflow areas to match

Should now take into account all line-decoration styles and address all suggested concerns.

Comment 82

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

Comment on attachment 540936 [details] [diff] [review]
fix line positioning in quirks mode code and set overflow areas to match

>+  void UnionAdditionalOverflow(nsPresContext* aPresContext,
>+                               PropertyProvider& aProvider,
>+                               nsRect* aVisualOverflowRect,
>+                               bool includeTextDecorations);

s/includeTextDecorations/aIncludeTextDecorations/
(Sorry, I should have caught that before.)

Also, you missed this comment from last time:

In LineDecoration::operator==, you still need to s/other/aOther/.

(and you may as well put it in the same order as the other things)

>+  // These represent the offset to f's top from our baseline in our coordinate
>+  // space and the offset from our baseline to f's baseline in our coordinate
>+  // space up to the nearest block element, respectively.

This still isn't quite right, while frameTopOffset is from this to f,
baselineOffset is from this to f or the nearest block, whichever is closer.

>     const nsStyleDisplay *disp = context->GetStyleDisplay();
>+
>     if (disp->mDisplay != NS_STYLE_DISPLAY_INLINE &&

You still didn't quite get rid of this chunk of changes.

>+        const nsRect decorationRect(
>+          nsCSSRendering::GetTextDecorationRect(aPresContext,
>+            gfxSize(gfxWidth, metrics.underlineSize),
>+            ascent, metrics.underlineOffset,
>+            NS_STYLE_TEXT_DECORATION_LINE_UNDERLINE, dec.mStyle));
>+
>+        const nscoord rectTop = decorationRect.y - dec.mBaselineOffset;
>+
>+        top = NS_MIN(rectTop, top);
>+        bottom = NS_MAX(rectTop + decorationRect.height, bottom);

In this pattern, which occurs three times with slight variation, I think
it's clearer to use the operator+ that allows adding an nsRect and an
nsPoint (it's defined in BaseRect.h), and do:

const nsRect decorationRect =
  nsCSSRendering::GetTextDecorationRect(aPresContext,
    gfxSize(gfxWidth, metrics.underlineSize),
    ascent, metrics.underlineOffset,
    NS_STYLE_TEXT_DECORATION_LINE_UNDERLINE, dec.mStyle) +
  nsPoint(0, -dec.mBaselineOffset);
top = NS_MIN(decorationRect.y, top);
bottom = NS_MAX(decorationRect.YMost(), bottom);

>-        provider.GetStart().GetOriginalOffset() + provider.GetOriginalLength();
>+      provider.GetStart().GetOriginalOffset() + provider.GetOriginalLength();

No need to touch this.


With that, and the remaining reftest fixed, I think we should be good
to go.  (Is that one reftest the only one that needs fixing, or did
you have patches in your tree that aren't in this patch?)

(Though we may also want to add a few reftests testing for the behavior
in this patch.)

Comment 83

[Vitor Menezes]

Attachment: fix line positioning in quirks mode code and set overflow areas to match

Added a reftest, fixed an existing reftest to agree as to what "correct" rendering means, and addressed latest suggestions.

Comment 84

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

Attachment: a testcase to help debug the rounding issues

This should help with the rounding issues; it also should be straightforward to turn into a reftest by making the span color:transparent and then removing the vertical-align properties in the reference.

Comment 85

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

Comment on attachment 541252 [details] [diff] [review]
fix line positioning in quirks mode code and set overflow areas to match

>Fix text-decoration positioning in quirks mode and set overflow areas to match.  (Bug 403524)  r=dbaron
>Change the quirks mode text-decoration code (soon to be used for all

somehow the blank line between these two got lost.


The added reftest files didn't get included in this patch.  You need to hg add and hg qrefresh.

>++== decoration-css21.html decoration-css21-ref.html # bug 403524
>+\ No newline at end of file

You should have the newline at end of file.  (Not having it is a 
Windows-ism; Windows uses newlines as line separators; Unix uses them as
line terminators.

Otherwise this looks good.

When you post the patch with the reftest, I'll send it to try server
( https://wiki.mozilla.org/Build:TryServer ) to make sure it passes tests on our infrastructure.


Two remaining things that should be in separate patches:
 * make the setting of OVERRIDE_ALL quirks-mode only
 * figure out the rounding issues with line positioning

Comment 86

[Vitor Menezes]

Attachment: fix line positioning in quirks mode code and set overflow areas to match

The newline issue was my fault--I just plainly didn't insert one after adding in my reftest. Dunno what happened to the line in the comment--I probably removed it and didn't think much of it.

Also, reftests are now included.

Comment 87

[Vitor Menezes]

Attachment: fix line positioning in quirks mode code and set overflow areas to match

Slightly cleaner reftests.

Comment 88

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

Comment on attachment 541529 [details] [diff] [review]
fix line positioning in quirks mode code and set overflow areas to match

One little problem with the reftest, which I'll fix locally before pushing to try; it's missing:

.sup { vertical-align: super; }

(We forgot to add that after changing it from a sup element to a class.)

I also have the patch merged with Mats's text-overflow changes; the result of both of those is here:
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/fdab3779ee15/vitor-403524
which I'll now push to try.

Comment 89

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

The only problem is:

REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/text-decoration/decoration-style-quirks.html | assertion count 9 is more than expected 0 assertions

Because there were (during this one test) 9 occurrences of:

###!!! ASSERTION: aStyle is none: 'aStyle != NS_STYLE_TEXT_DECORATION_STYLE_NONE', file ../../../layout/base/nsCSSRendering.cpp, line 3410

Comment 90

[Vitor Menezes]

Attachment: fix line positioning in quirks mode code and set overflow areas to match

Decoration accumulation code now accumulates only decorations with genuine styles, which resolves the asserts in the offending reftest.

Comment 91

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

Comment on attachment 541529 [details] [diff] [review]
fix line positioning in quirks mode code and set overflow areas to match

>-  void UnionTextDecorationOverflow(nsPresContext* aPresContext,
>-                                   PropertyProvider& aProvider,
>-                                   nsRect* aVisualOverflowRect);
>+  void UnionAdditionalOverflow(nsPresContext* aPresContext,
>+                               PropertyProvider& aProvider,
>+                               nsRect* aVisualOverflowRect,
>+                               bool aIncludeTextDecorations);

dbaron: Shouldn't we use PRBool instead of bool here because nsTextFrame is using PRBool? I saw something to discuss about PRBool vs. bool but I don't remember the detail.

>@@ -6984,17 +7070,21 @@ nsTextFrame::ReflowText(nsLineLayout& aL
> 
>   mAscent = aMetrics.ascent;
> 
>   // Handle text that runs outside its normal bounds.
>   nsRect boundingBox = RoundOut(textMetrics.mBoundingBox) + nsPoint(0, mAscent);
>   aMetrics.SetOverflowAreasToDesiredBounds();
>   aMetrics.VisualOverflow().UnionRect(aMetrics.VisualOverflow(), boundingBox);
> 
>-  UnionTextDecorationOverflow(presContext, provider, &aMetrics.VisualOverflow());
>+  // When we have text decorations, we don't need to compute their overflow now
>+  // because we're guaranteed to do it later
>+  // (see nsLineLayout::RelativePositionFrames)
>+  UnionAdditionalOverflow(presContext, provider, &aMetrics.VisualOverflow(),
>+                          false);

PR_FALSE instead of false?

> 
>   /////////////////////////////////////////////////////////////////////
>   // Clean up, update state
>   /////////////////////////////////////////////////////////////////////
> 
>   // If all our characters are discarded or collapsed, then trimmable width
>   // from the last textframe should be preserved. Otherwise the trimmable width
>   // from this textframe overrides. (Currently in CSS trimmable width can be
>@@ -7244,17 +7334,17 @@ nsTextFrame::RecomputeOverflow()
>     mTextRun->MeasureText(provider.GetStart().GetSkippedOffset(),
>                           ComputeTransformedLength(provider),
>                           gfxFont::LOOSE_INK_EXTENTS, nsnull,
>                           &provider);
> 
>   nsRect &vis = result.VisualOverflow();
>   vis.UnionRect(vis, RoundOut(textMetrics.mBoundingBox) + nsPoint(0, mAscent));
> 
>-  UnionTextDecorationOverflow(PresContext(), provider, &vis);
>+  UnionAdditionalOverflow(PresContext(), provider, &vis, true);

PR_TRUE instead of true?


>diff --git a/layout/reftests/text-decoration/decoration-css21-ref.html b/layout/reftests/text-decoration/decoration-css21-ref.html
>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/text-decoration/decoration-css21-ref.html
>@@ -0,0 +1,28 @@
>+<html>
>+	<head>

>diff --git a/layout/reftests/text-decoration/decoration-css21.html b/layout/reftests/text-decoration/decoration-css21.html
>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/text-decoration/decoration-css21.html
>@@ -0,0 +1,30 @@
>+<html>
>+	<head>

I think that you should replace these files' each horizontal tab with 2 white spaces.

>diff --git a/layout/reftests/text-decoration/decoration-style-quirks-ref.html b/layout/reftests/text-decoration/decoration-style-quirks-ref.html
>--- a/layout/reftests/text-decoration/decoration-style-quirks-ref.html
>+++ b/layout/reftests/text-decoration/decoration-style-quirks-ref.html

> <p>
>   <span style="text-decoration: underline line-through overline;
>                -moz-text-decoration-style: double;">
>     Here has double decoration lines,
>-  </span><span style="font-size: 2em;
>-                      text-decoration: underline line-through overline;
>-                      -moz-text-decoration-style: double;">
>+  <span style="font-size: 2em;">
>     here is specified as dashed decoration lines but should be
>     ignored</span><span style="text-decoration: underline line-through overline;
>                                -moz-text-decoration-style: double;">,
>     and here has double decoration lines.</span>
> </p>

You remove </span> but you don't add new </span> later.

>diff --git a/layout/reftests/text-decoration/reftest.list b/layout/reftests/text-decoration/reftest.list
>--- a/layout/reftests/text-decoration/reftest.list
>+++ b/layout/reftests/text-decoration/reftest.list
>@@ -84,8 +84,9 @@ fails == underline-block-propagation-sta
> fails-if(Android) fails-if(d2d) == underline-block-propagation-2-standards.html underline-block-propagation-2-standards-ref.html # bug 585684
> == text-decoration-zorder-1-standards.html text-decoration-zorder-1-ref.html
> fails == text-decoration-zorder-1-quirks.html text-decoration-zorder-1-ref.html # bug 403524
> == table-quirk-1.html table-quirk-1-ref.html
> == table-quirk-2.html table-quirk-2-ref.html
> == text-decoration-propagation-1-quirks.html text-decoration-propagation-1-quirks-ref.html
> fails == text-decoration-propagation-1-standards.html text-decoration-propagation-1-standards-ref.html
> == 641444-1.html 641444-1-ref.html
>+== decoration-css21.html decoration-css21-ref.html # bug 403524

Shouldn't be sorted by alphabet?

Comment 92

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

(In reply to comment #91)
> dbaron: Shouldn't we use PRBool instead of bool here because nsTextFrame is
> using PRBool? I saw something to discuss about PRBool vs. bool but I don't
> remember the detail.

> PR_FALSE instead of false?

> PR_TRUE instead of true?

I think using bool is fine as long as it doesn't cause us to convert between bool and PRBool.

> I think that you should replace these files' each horizontal tab with 2
> white spaces.

I think it's good if tests come in a variety of formats; it leads to additional test coverage.  So we shouldn't enforce too many style requirements on them.

> You remove </span> but you don't add new </span> later.

Looks like something is wrong there.

> Shouldn't be sorted by alphabet?

I don't think it matters.  Some directories sort in a logical order or by chronology instead.

Comment 93

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

This revised patch is missing the change from comment 88 (adding vertical-align:super to the .sup rule in decoration-css21.html).

It also changes the indent of struct LineDecoration relative to earlier patches; it should be the way it was (2 spaces less).

There also seems to be a good bit of churn relative to the previous patch most of which looks wrong, e.g.,:

removing the removal of "TextDecorations decorations;" at the start of GetTextDecorations; changing the indent here (2 extra spaces on the second line):

+    if (!useOverride &&
+          (NS_STYLE_TEXT_DECORATION_LINE_OVERRIDE_ALL & textDecorations))

adding extra spaces here:

-+                     ascent = gfxFloat(mAscent) / appUnitsPerDevUnit;
++                     ascent   = gfxFloat(mAscent) / appUnitsPerDevUnit;

and here:

 +      aVisualOverflowRect->UnionRect(*aVisualOverflowRect,
-+                                     nsRect(0, top, width, bottom - top));
++                                       nsRect(0, top, width, bottom - top));

Also, in the merging with the text-overflow changes, you should probably change this line:
+  gfxPoint pt(aFramePt.x / app, 0);
to this:
+  gfxPoint pt(x / app, 0);

and there's a bunch of new unnecessary whitespace churn in nsTextFrame::ReflowText and in ua.css.


The moral of the story is that you should generally read diffs of what you do.  Changing things (especially whitespace) unnecessarily imposes extra reviewing work, since I'm just going to diff your patch against the previous one.

Comment 94

[Vitor Menezes]

Attachment: fix line positioning in quirks mode code and set overflow areas to match

Fixed random whitespace changes, added a </span> in my reftests where it was needed (thank you Masayuki), updated my reftest to be a little more robust, and otherwise (to the best of my knowledge) integrated latest suggestions.

Comment 95

[Vitor Menezes]

Attachment: enable font override in quirks mode only

Add a check to propagate font overrides only when in quirks mode, plus a couple of reftests to assert quirks vs. standards behavior.

Comment 96

[Vitor Menezes]

Attachment: fix line positioning in quirks mode code and set overflow areas to match

Addressed an edge case involving vertically-aligned elements inheriting text decorations from a block, as this case requires that the decorations be aligned to the line the element is on. Also added a reftest for this case.

Comment 97

[Vitor Menezes]

Attachment: Fix rounding issues with CSS rendering

Corrects issues with horizontal rounding; vertical rounding TBD

Comment 98

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

Comment on attachment 545291 [details] [diff] [review]
Fix rounding issues with CSS rendering

I think you want to continue using NS_floor -- and you also want NS_floor for the right edge; otherwise you'll have problems occasionally when crossing from negative to positive coordinates.

I'd also name the variable "x" rather than "X".  It may also be clearer to do:
  gfxFloat x = NS_floor(aPt.x + 0.5);
  gfxFloat xMost = NS_floor(aPt.x + aLineSize.width + 0.5);
  gfxRect r(x, 0, xMost - x, 0);

You also need a commit message describing what the patch is changing.

With that, this looks good.

Comment 99

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

Comment on attachment 542695 [details] [diff] [review]
enable font override in quirks mode only

This needs a commit message to describe what you're changing and why, such as:

Set NS_STYLE_TEXT_DECORATION_LINE_OVERRIDE_ALL for font elements only in quirks mode, so that we can use the quirks mode text decoration code (which honors it) in standards mode as well without introducing this quirk.  (Bug 403524)  r=dbaron

Comment 100

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

As far as the vertical rounding issues go, a few thoughts:

nsTextFrame::PaintText (which is the most important caller of PaintTextDecorations... and the other callers are also within it, via PaintOneShadow or PaintTextWithSelection) initializes textBaselinePt with a call to nsLayoutUtils::GetSnappedBaselineY.

Presuming that that's there for a good reason (which I suspect it is), I suspect the fix for the vertical rounding issues may be to make your new text decoration offset computation code, or more likely, the code that uses those offsets, compensate for this.  (This requires a little bit of thought since you need to compensate for the adjustment at a point when you have display-relative coordinates rather than frame-relative coordinates, but it shouldn't require more than a few lines of code.  It probably also requires that you pad the overflow rect calculations by an extra half pixel since you don't know how the rounding will come out.)

Comment 101

[Vitor Menezes]

Attachment: Fix rounding issues with CSS rendering

Both horizontal & vertical errors should now be resolved

Comment 102

[Vitor Menezes]

Attachment: Fix z-ordering bug in quirks-mode code

Refactors nsTextframe a bit so that quirks-mode text fits the CSS 2.1 spec, so that the two codepaths may be merged.

Comment 103

[Vitor Menezes]

Attachment: Fix quirks-mode shadow rendering code

Fixes quirks-mode shadow rendering code so that reftests pass.

Comment 104

[Vitor Menezes]

Attachment: Merge quirks mode and standards mode codepaths

Standards-mode code for text decorations removed and using quirks mode code instead. As a result, some previously failing reftests began to succeed.

Comment 105

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

Comment on attachment 545284 [details] [diff] [review]
fix line positioning in quirks mode code and set overflow areas to match

nsLineLayout.cpp:

>+  // If the frame being reflowed has text decorations, we simulate the
>+  // propagation of those decorations to a line-level element by storing the
>+  // offset in a frame property on the child frames that are vertically-aligned
>+  // somewhere other than the baseline. This property is then used by
>+  // nsTextFrame::GetTextDecorations when the same conditions are met

"the child frames" -> "any child frames"
"are met" -> "are met." (add period)

>+      nsStyleContext *const styleContext = f->GetStyleContext();
>+      const nsStyleCoord& vAlign =
>+        styleContext->GetStyleTextReset()->mVerticalAlign;

condense this (since you can call GetStyleTextReset() on the frame
directly, which implicitly goes through the style context) to just:

      const nsStyleCoord& vAlign = f->GetStyleTextReset()->mVerticalAlign;

>+          vAlign.GetIntValue()!= NS_STYLE_VERTICAL_ALIGN_BASELINE)

space before the !=

(and also bring the { up to that line)

>+        f->Properties().Set(nsIFrame::LineBaselineOffset(), (void*)offset);

Instead of a (void*) cast, use NS_INT32_TO_PTR
(and use NS_PTR_TO_INT32 at the other end)


>         frame->FinishAndStoreOverflow(r, frame->GetSize());
>       }
>-
>       // If we have something that's not an inline but with a complex frame
>       // hierarchy inside that contains views, they need to be
>       // positioned.

Probably best to leave that blank line.

nsTextFrameThebes.cpp:

>+    //printf("Getting decorations for 0x%x...\n", f);

remove this

>+      baselineOffset = frameTopOffset -
>+        (nscoord)(f->Properties().Get(nsIFrame::LineBaselineOffset()));

NS_PTR_TO_INT32

>+
>+  const gfxFloat app = aTextPaintStyle.PresContext()->AppUnitsPerDevPixel();

No need for two blank lines above this; one is enough.

>+  // When we have text decorations, we don't need to compute their overflow no
>+  // because we're guaranteed to do it later
>+  // (see nsLineLayout::RelativePositionFrames)
>+  UnionAdditionalOverflow(presContext, provider, &aMetrics.VisualOverflow(),
>+                          false);
>-
>   /////////////////////////////////////////////////////////////////////
>   // Clean up, update state
>   /////////////////////////////////////////////////////////////////////
>-

But you probably want to leave the two blank lines here.


r=dbaron with those things fixed

Comment 106

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

Comment on attachment 547512 [details] [diff] [review]
Fix quirks-mode shadow rendering code

>From: Vitor Menezes <[e-mail]>
>
>Since nsTextFrame::DrawText now draws text decorations, we need to 
>stipulate to the text decoration code that it shouldn't use any override color.

The first line of the commit message is more important than the rest, so you don't want to have a random line break between first and second line.  What you probably want is something like:

>From: Vitor Menezes <[e-mail]>
>
>Bug 403524:  Draw correct colors for text-decorations on default-color shadows in quirks mode.
>
>Make the quirks mode text-decoration + text-shadow code draw the
>decorations for shadows that do not have a specified color using
>the same color used for the un-shadowed decorations, which matches
>standards mode behavior.  (The color of unspecified-color shadows
>is explicitly undefined in the specification.)
>
>This code will (in a later patch on this bug) be used for both
>quirks and standards modes.


r=dbaron

Comment 107

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

Comment on attachment 547524 [details] [diff] [review]
Merge quirks mode and standards mode codepaths

>From: Vitor Menezes <[e-mail]>
>
>In order to merge standards and quirks mode text-decoration code, we basically have to
>remove all the standards-mode code and the check in the quirks-mode code for 
>quirks mode, since the quirks code now implements the CSS 2.1 standard.

Again, the first line is more important than the rest.  I don't think you really need a multi-line comment here though.  Probably something like this is sufficient:

>Remove the standards-mode text-decoration code and use the quirks-mode code in all modes.  (Bug 403524)

***Additional code you need to remove***:

HasTextFrameDescendant and HasTextFrameDescendantOrInFlow, in nsHTMLContainerFrame.cpp (including the forward declaration).

>diff --git a/layout/reftests/text-decoration/reftest.list b/layout/reftests/text-decoration/reftest.list

>-fails == underline-block-propagation-standards.html underline-block-propagation-standards-ref.html # bug that decoration is drawn through non-text child (bug 428599)
>+== underline-block-propagation-standards.html underline-block-propagation-standards-ref.html # bug that decoration is drawn through non-text child (bug 428599)

remove the comment at the end of the line too

r=dbaron with that

Comment 108

[Vitor Menezes]

Attachment: fix line positioning in quirks mode code and set overflow areas to match

Comment 109

[Vitor Menezes]

Attachment: Fix quirks-mode shadow rendering code

Comment 110

[Vitor Menezes]

Attachment: Merge quirks mode and standards mode codepaths

Comment 111

[Vitor Menezes]

Attachment: Fix broken text-shadow reftests

Both of these tests expected text-decorations to render behind shadows, but the current CSS3 spec on text-shadow (http://dev.w3.org/csswg/css3-text/#text-shadow) clearly indicates that shadows should render behind the text *and* its decorations.

This was likely due to our old standards-mode code doing strange painting orders; merging that codepath with the quirks-mode code so standards-mode could implement CSS 2.1 exposed the oddity.

Also, CC'ing Michael Ventnor (author of these two tests) in the event that I am horribly mistaken.

Comment 112

[Vitor Menezes]

Attachment: Fix broken text-shadow reftests

Added commit message.

Comment 113

[Mats Palmgren (inactive)]

If this bug makes text-decorations identical in standards/quirks mode,
then we can remove layout/reftests/text-overflow/standards-decorations.html
or quirks-decorations.html

Comment 114

[Vitor Menezes]

Attachment: Fix textarea text-decoration scheme

By making the anonymous input element an inline-block, we only get decorations defined on the textarea element itself onto the input text.

With this final change, all reftests pass.

Comment 115

[Vitor Menezes]

Bug 585684 declared as dependent on this one; the textarea fix technically alleviates the concerns brought up there.

Comment 116

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to comment #113)
> If this bug makes text-decorations identical in standards/quirks mode,
> then we can remove layout/reftests/text-overflow/standards-decorations.html
> or quirks-decorations.html

I disagree. We should test both.

Comment 117

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

Comment on attachment 547511 [details] [diff] [review]
Fix z-ordering bug in quirks-mode code

>Quirks-mode code draws text, and then all decorations. We need to instead draw
>underlines, then overlines, -then- text, then line-throughs, as per CSS 2.1 aspec.
>
>This involves refactoring nsTextFrame::PaintTextDecorations and nsTextFrame::DrawText
>by merging them together, and also updating some of their callers.

You need a first line for this commit message (all on one line), perhaps:

Make z-ordering of quirks mode text decoration drawing match CSS 2.1.  (Bug 403524)

For the rest of the lines, you should wrap at less than 80 characters, and
change "aspec" -> "spec".


Don't change nsTextFrame::PaintTextSelectionDecorations since
you don't use the additional parameter.

+  const gfxFloat app = aTextStyle.PresContext()->AppUnitsPerDevPixel();
+
+  // XXX aFramePt is in AppUnits, shouldn't it be nsFloatPoint?
+  nscoord x = NSToCoordRound(aFramePt.x);
+  nscoord width = GetRect().width;
+  aClipEdges.Intersect(&x, &width);
+
+  gfxPoint pt(x / app, 0);
+  gfxSize size(width / app, 0);
+  const gfxFloat ascent = gfxFloat(mAscent) / app;
+  const gfxFloat frameTop = aFramePt.y;
+
+  nscolor lineColor;

You should move most of this inside the if (drawDecorations) code,
except that:
 * you'll need to declare pt, size, ascent, and frameTop outside the if()
 * you should just declare lineColor separately for each of the 3 uses

You should probably also rename pt -> decPt and size -> decSize.

r=dbaron with those things fixed

Comment 118

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

Comment on attachment 548664 [details] [diff] [review]
Fix broken text-shadow reftests

r=dbaron

Comment 119

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

Comment on attachment 548997 [details] [diff] [review]
Fix textarea text-decoration scheme

>I suggest making textareas inline-blocks so that their decorations are isolated

"I suggest making" -> "This patch makes"
"textareas" -> "the anonymous div inside textareas and text inputs"
"their decorations are isolated" -> "decorations on elements outside of them do not apply to the text in them"

>and since the suggested CSS2.1 UA stylesheet does so as well (see
>http://www.w3.org/TR/CSS21/sample.html)

This bit isn't relevant; remove it.

>diff --git a/layout/reftests/text-decoration/underline-block-propagation-2-standards-ref.html b/layout/reftests/text-decoration/underline-block-propagation-2-standards-ref.html

The change to this file (but not the other files) seems to me to belong in the patch to remove the standards mode code rather than this patch, since otherwise I think this test will fail between the patches.

r=dbaron with that

Comment 120

[Vitor Menezes]

Attachment: Fix z-ordering bug in quirks-mode code

Incorporated latest suggestions. Also refactored nsTextFrame::DrawText into two separate functions drawing with and without decorations.

Comment 121

[Vitor Menezes]

Attachment: Fix z-ordering bug in quirks-mode code

Forgot to qref after applying identifier changes; rectifying that....

Comment 122

[Vitor Menezes]

(In reply to comment #119)
> >diff --git a/layout/reftests/text-decoration/underline-block-propagation-2-standards-ref.html b/layout/reftests/text-decoration/underline-block-propagation-2-standards-ref.html
> 
> The change to this file (but not the other files) seems to me to belong in
> the patch to remove the standards mode code rather than this patch, since
> otherwise I think this test will fail between the patches.
> 
> r=dbaron with that

The test fails before this patch anyway, since without it the textareas have decorations inside them. After fixing textareas, that inserted span remains the only difference, as it was compensating for previous standards-mode decoration rendering order.
I can move the change to the previous patch, but the test will continue to fail.

Comment 123

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

Oh, I though this patch was underneath the patch to remove the standards mode code.

Comment 124

[Vitor Menezes]

Attachment: Fix textarea text-decoration scheme

Updated commit message. Reftest correction left as-is.

Comment 125

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

Comment on attachment 545773 [details] [diff] [review]
Fix rounding issues with CSS rendering

I'm a little worried we *might* need to use the snapped baseline here somehow, but this seems to work, so r=dbaron.

(I'd be most worried about seeing issues on either (a) Windows with D2D+DirectWrite or (b) Mac -- and the issue I'd be concerned about is varying spacing between text and underline between different lines.)

Comment 126

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

I re-line-wrapped a few of the commit messages and pushed to mozilla-inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/225a79ce27bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/88d8bfd7ef64
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e774f69980e
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3d43eea28c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/db9466903986
https://hg.mozilla.org/integration/mozilla-inbound/rev/83b2648ee442
https://hg.mozilla.org/integration/mozilla-inbound/rev/941c6bc7d728
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9dff8b867f0

Note that I don't think I'd actually reviewed https://hg.mozilla.org/integration/mozilla-inbound/rev/941c6bc7d728 before, but it looks ok at a quick glance and I don't think it needs more review than that.

Comment 127

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

Android bustage fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6912479afdd

Comment 128

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/225a79ce27bc
http://hg.mozilla.org/mozilla-central/rev/88d8bfd7ef64
http://hg.mozilla.org/mozilla-central/rev/5e774f69980e
http://hg.mozilla.org/mozilla-central/rev/d3d43eea28c8
http://hg.mozilla.org/mozilla-central/rev/db9466903986
http://hg.mozilla.org/mozilla-central/rev/83b2648ee442
http://hg.mozilla.org/mozilla-central/rev/941c6bc7d728
http://hg.mozilla.org/mozilla-central/rev/c9dff8b867f0
http://hg.mozilla.org/mozilla-central/rev/c6912479afdd

Comment 129

[j.j.]

Seems this quirk was undocumented
https://developer.mozilla.org/en/mozilla_quirks_mode_behavior

Comment 130

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

Tests checked in for this bug include <img src="http://www.mozilla.org/...">. Tests should not refer to external Web sites.

Comment 131

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

Tests fixed in https://hg.mozilla.org/integration/mozilla-inbound/rev/512ce8163e88

Comment 132

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/512ce8163e88

Comment 133

[Eric Shepherd [:sheppy]]

This is documented here:

https://developer.mozilla.org/en/mozilla_quirks_mode_behavior#Block_and_Inline_layout

Also mentioned on Firefox 8 for developers:

https://developer.mozilla.org/en/Firefox_8_for_developers#CSS