655877 - (svgtext) SVG text should use CSS text frames to gain support for various text layout features

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Cameron McCormack (:heycam)]

This would get us text selection, text-decoration, font-variant, bidi, and various other text layout features without having to reimplement in the SVG text layout code.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.1)

Start of implementation of the SVG text layout proposal at http://www.w3.org/Graphics/SVG/WG/wiki/Proposals/Text_layout using CSS text frames for rendering.  Most of the work is in nsSVGTextFrame2.cpp.

Handling of x/y/dx/dy for ltr/rtl/bidi layout is working, rotate has no effect yet.  Painting of "simple" text (single chunk, no stroke, no rotations, solid colour fill) works and also gets text decorations.

TBD:
- decorations and non-black fills for non-simple text
- rotate=""
- stroking of text
- textPath layout
- interaction for events and text selection
- lots more

Comment 2

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.2)

Painting of solid fill, no stroke text with multiple x/y/dx/dy attributes now works, including with decorations.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.3)

(Things are broken when there are skipped chars, btw, like multiple spaces in a row.)

Now we have non-solid fills and strokes working on text and their decorations -- all styles of decorations except wavy ones so far.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.4)

Compressed white space due to the white-space property is handled properly now.  We'll need to decide how to handle line breaks due to white-space:pre, and whether we can map xml:space="" behaviour to white-space to avoid having the old SVG specific behaviour as well.

Invalidations happen correctly now when selections changes, such as when press Cmd+A to select all.  Searching also works, so the green and pink search highlights get rendered.

Comment 5

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

Awesome!

Comment 6

[Marek Raida]

Yes, progress looks very promising... ;-)

Comment 7

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.5)

Fix some problems with positioning of text runs that have half a ligature in them, and allow partial ligatures to be reported back when drawing a gfxTextRun in GLYPH_PATH mode through a callback.

Selecting text with the mouse works now too.

Comment 8

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.6)

* SVG DOM text methods (getSubStringLength etc.) support.
* Text on a path support.
* Some refactoring/cleanup.

Comment 9

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.7)

* Make various properties (display:block, position:relative, float, border, ...)
  have no effect on SVG text.
* Add support for the white-space property, including multi-line text via
  white-space:pre-line.
* Map xml:space="preserve" to white-space:pre.
* Disable letter-spacing and word-spacing for now.
* Add support for dominant-baseline by mapping into an equivalent vertical-align
  value.
* Fix problem with transform used for gradients and patterns on text
  fill/stroke.

Comment 10

[Cameron McCormack (:heycam)]

Attachment: WIP (v0.8)

Done:
* Fix a bunch of reftest failures and assertions.
* Stop painting selected text as black on Mac (and allow the original fill paint
  to be used).
* Fix some problems with invalidation & covered region calculations and text
  selection by the mouse.
* Handle full page zoom.
* Text zoom is also handled.  (In the existing code, text zoom is deliberately
  undone for SVG text.  Could do this too, but I think it is fine to have text
  zoom affect SVG text too.)
* Handle style changes and DOM mutations by reflowing and invalidating properly.

Major things left to be done:
* Scale font sizes used for layout and text runs to be within a reasonable
  range, so that text still works inside very small or large coordinate systems.
* Get contenteditable working.
* Fix some problems with caret browsing.
* Abandon some of my proposed changes to text anchoring and return to following
  the spec as it currently stands.  (Already done for text on a path.)
* Rebase on top of several months' worth of activity (including DLBI and SVG
  display lists).

Comment 11

[Robert Longson [:longsonr]]

(In reply to Cameron McCormack (:heycam) from comment #10)
> * Text zoom is also handled.  (In the existing code, text zoom is
> deliberately
>   undone for SVG text.  Could do this too, but I think it is fine to have
> text
>   zoom affect SVG text too.)

I don't think we want text zoom to affect SVG text, neither do we want setting a minimum font size to affect it. We did make these changes deliberately a long time ago see bug 291785

Comment 12

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

(In reply to Cameron McCormack (:heycam) from comment #10)
> Major things left to be done:
> * Get contenteditable working.
> * Fix some problems with caret browsing.

I don't think we should do these before landing. These are not regressions.

One major thing to be done is to split up the patch for review and landing :-).

Comment 13

[Cameron McCormack (:heycam)]

Current split up patch queue is here: http://mcc.id.au/temp/bug-655877/

Comment 14

[Cameron McCormack (:heycam)]

Attachment: Part 16: Treat all values of display other than 'none' as 'inline' in SVG text frames.

This patch makes sure that values of 'display' other than "none" get treated the same as "inline" on SVG text frames.  (The NS_FRAME_IS_SVG_TEXT state bit is set on the nsSVGTextFrame2 frame and all of its nsBlockFrame/nsInlineFrame descendants.)

I searched for all of the places that looked at mDisplay on nsStyleDisplay or called the accessor functions like IsOutsideInline, and for the places where it would matter (e.g. I didn't change any of inner bits of the table frame construction code to look at SVG's sense of the display value) I called these Effective* functions.

Some issues:
  (1) I don't quite like the "Effective" name
  (2) I don't know whether it would be better to put these functions somewhere
      other than on nsIFrame (maybe nsLayoutUtils? or should they go directly
      on nsStyleDisply, taking an nsIFrame* argument to check whether it is
      an SVG text frame?)

Comment 15

[Cameron McCormack (:heycam)]

Attachment: Part 25: Give gfxTextRuns the ability to invoke a callback after painting glyphs and partial ligatures.

Because a text run might be painted in three parts -- partial ligatures at the start of the run, the main bit, partial ligatures at the end -- I need to be able to handle those three parts separately (so my nsSVGTextFrame2 can fill and stroke them).  So this adds a callback argument that, when specified, means that the glyphs will be drawn with mode gfxFont::GLYPH_PATH and then callback invoked immediately afterwards.

(Part 17 extends adds a similar callback parameter to nsTextFrame, with a few different callback functions (for different parts of the text frame being rendered), which uses this gfxFont callback.)

Comment 16

[Cameron McCormack (:heycam)]

(Part 26 uses this, sorry, not part 17.)

Comment 17

[Cameron McCormack (:heycam)]

Attachment: Part 42: Construct new SVG text frames if the pref is set.

This is the part where the new SVG text frames are constructed.  I have this ITEM_IS_WITHIN_SVG_TEXT frame construction item flag, and the additional aIsWithinSVGText argument to FindSVGData, because we need to know whether for example an <a> ought to be an nsInlineFrame (if it is within an nsSVGTextFrame2) or an nsSVGAFrame (if it is elsewhere in an SVG tree), and when inline frames are being constructed, null is passed in for aParentFrame so we can't check aParentFrame->mState for NS_FRAME_IS_SVG_TEXT.

(Although just writing this out now I wonder whether that means aParentFrame being null always implies within SVG text, in which case we wouldn't need to do this.)

Comment 18

[Cameron McCormack (:heycam)]

Attachment: Part 1: Move some path drawing functions from nsSVGGeometryFrame to nsSVGUtils.

(Just feedback for this one at this stage, I might need to change it a bit when doing my rebasing.)  I've had to move all these painting-related functions out of nsSVGGeometryFrame.  Currently, nsSVGGlyphFrame is an nsSVGGeometryFrame, but in the future we'll just have nsSVGTextFrame2, which is an container frame, and its descendent nsTextFrames.  I need to be able to call for example GetStrokeWidth for the nsTextFrames (or their parents) and they aren't nsSVGGeometryFrames.  Do you think it's OK to do this moving?

Comment 19

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

Comment on attachment 640890 [details] [diff] [review]
Part 16: Treat all values of display other than 'none' as 'inline' in SVG text frames.

Review of attachment 640890 [details] [diff] [review]:
-----------------------------------------------------------------

The methods we want most callers to call should have the most obvious names, and the more specialized methods should have less obvious names. So if possible I think we want IsBlockInside to take account of SVG-ness, and a different name for methods that deliberately don't take account of SVG-ness (e.g. IsBlockInsideStyle).

::: layout/generic/nsIFrame.h
@@ +2809,5 @@
>      VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY = 0x01
>    };
>    bool IsVisibleConsideringAncestors(PRUint32 aFlags = 0) const;
>  
> +  static bool IsEffectivelyBlockInside(const nsIFrame* aFrame, const nsStyleDisplay* aDisplay) {

Can't this just be called IsBlockInside? Ditto for BlockOutside and InlineOutside.

And I don't really like these static methods. Seems to me the non-static methods are preferred, and for the cases where we don't have a frame, nsStyleDisplay::IsBlockInside could take an optional nsIFrame* parameter which it consults.

@@ +2843,5 @@
>      }
>      return aDisplay->IsFloating();
>    }
>  
> +  static PRUint8 EffectiveDisplay(const nsIFrame* aFrame, const nsStyleDisplay* aDisplay) {

Why not just call this GetDisplay()?

@@ +2850,5 @@
> +    }
> +    return aDisplay->mDisplay;
> +  }
> +
> +  bool IsEffectivelyBlockInside() const {

Why not IsBlockInside etc?

::: layout/style/nsStyleStruct.h
@@ +1643,5 @@
>             mTransformStyle == NS_STYLE_TRANSFORM_STYLE_PRESERVE_3D ||
>             mBackfaceVisibility == NS_STYLE_BACKFACE_VISIBILITY_HIDDEN;
>    }
> +
> +  PRUint8 DisplayForSVG() const {

A bit ambiguous. GetDisplayForSVGElement() I would say

Comment 20

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

Comment on attachment 640891 [details] [diff] [review]
Part 25: Give gfxTextRuns the ability to invoke a callback after painting glyphs and partial ligatures.

Review of attachment 640891 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.h
@@ +2463,5 @@
> +     * Callback for Draw() to use when drawing text with mode
> +     * gfxFont::GLYPH_PATH.
> +     */
> +    struct GlyphPathCallback {
> +        virtual void GlyphPath() = 0;

I think this needs a better name and a comment explaining when it's called. "This gets called after each path has been emitted into the context, possibly with some clipping applied to the context. This can be called multiple times as we draw partial ligatures with different clip regions."? Call it "NotifyPathEmitted"? Also, name the struct DrawCallbacks in case we want to add more?

Comment 21

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

Comment on attachment 640892 [details] [diff] [review]
Part 42: Construct new SVG text frames if the pref is set.

Review of attachment 640892 [details] [diff] [review]:
-----------------------------------------------------------------

More Boris's territory.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 640892 [details] [diff] [review]
Part 42: Construct new SVG text frames if the pref is set.

Please add comments at each point where you null-check aParentFrame to the effect that if it's null here then it will end up being an inline, and hence our GetType() check would return false anyway.

It looks to me like your new code doesn't handle <textPath> as a child of <a> which is itself a descendant of <text> with only <a> in between, whereas the old code did.  That needs to be fixed, and a test added, ideally.

It's adding a comment at the definition of sTSpanData explaining why it's not using ConstructInline (e.g. because it wants different behavior for generated content, if nothing else).

r=me with those addressed.

Comment 23

[Cameron McCormack (:heycam)]

Attachment: Part 25: Give gfxTextRuns the ability to invoke a callback after painting glyphs and partial ligatures. (v1.1)

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> ::: gfx/thebes/gfxFont.h
> @@ +2463,5 @@
> > +     * Callback for Draw() to use when drawing text with mode
> > +     * gfxFont::GLYPH_PATH.
> > +     */
> > +    struct GlyphPathCallback {
> > +        virtual void GlyphPath() = 0;
> 
> I think this needs a better name and a comment explaining when it's called.
> "This gets called after each path has been emitted into the context,
> possibly with some clipping applied to the context. This can be called
> multiple times as we draw partial ligatures with different clip regions."?
> Call it "NotifyPathEmitted"? Also, name the struct DrawCallbacks in case we
> want to add more?

OK, I renamed it to DrawCallbacks and added the comment.

Comment 24

[Cameron McCormack (:heycam)]

Attachment: Part 42a: Construct new SVG text frames if the pref is set.

These are the changes to handle <text><a><textPath>, which turned out a bit trickier because it's another case where you don't have access to parent frames within FindSVGData.

Comment 25

[Cameron McCormack (:heycam)]

Attachment: Part 42a: Construct new SVG text frames if the pref is set. (v1.1)

With the tests included this time.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 643437 [details] [diff] [review]
Part 42a: Construct new SVG text frames if the pref is set. (v1.1)

r=me

Comment 27

[Cameron McCormack (:heycam)]

I'll keep this in sync with the patches attached to the bug.

Comment 28

[Cameron McCormack (:heycam)]

Attachment: Part 16: Treat all values of display other than 'none' as 'inline' in SVG text frames. (v1.1)

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> The methods we want most callers to call should have the most obvious names,
> and the more specialized methods should have less obvious names. So if
> possible I think we want IsBlockInside to take account of SVG-ness, and a
> different name for methods that deliberately don't take account of SVG-ness
> (e.g. IsBlockInsideStyle).

OK, I have used this naming style.  (There are a bunch of extra edits to the call sites of the now named IsBlockInsideStyle etc. just because of the rename.)

> And I don't really like these static methods. Seems to me the non-static
> methods are preferred, and for the cases where we don't have a frame,
> nsStyleDisplay::IsBlockInside could take an optional nsIFrame* parameter
> which it consults.

I've done this, except I didn't make the argument optional, so that it is clearer that you should always call IsBlockInsideStyle if you only want it to look at mDisplay and IsBlockInside if you want to use the frame's SVG-ness.

> ::: layout/style/nsStyleStruct.h
> @@ +1643,5 @@
> >             mTransformStyle == NS_STYLE_TRANSFORM_STYLE_PRESERVE_3D ||
> >             mBackfaceVisibility == NS_STYLE_BACKFACE_VISIBILITY_HIDDEN;
> >    }
> > +
> > +  PRUint8 DisplayForSVG() const {
> 
> A bit ambiguous. GetDisplayForSVGElement() I would say

I've turned this just into GetDisplay(nsIFrame*), in line with the naming of IsBlockInside etc.


I solved the problem of the mutually referential nsIFrame and nsStyleDisplay inlines by moving the definitions of nsStyleDisplay::IsBlockInside etc. to nsStyleStructInlines.h and #including this file towards the bottom of nsIFrame.h, before the definitions of its inline IsBlockInside etc. functions.

Comment 29

[Cameron McCormack (:heycam)]

Attachment: Part 8: Ignore letter-spacing in SVG text frames.

We do actually want to support letter-spacing eventually, but it's a bit tricky with the PropertyProviders, and we don't support it now, so best to force it to be ignored and not mess up the text layout.

Comment 30

[Cameron McCormack (:heycam)]

Attachment: Part 9: Ignore word-spacing in SVG text frames.

Same for word-spacing.

Comment 31

[Cameron McCormack (:heycam)]

Attachment: Part 11: Ignore vertical-align and map dominant-baseline to vertical-align in SVG text frames.

The current SVG text code supports SVG 1.1's dominant-baseline property (one of the split out vertical alignment properties that XSL defined) to control vertical alignment.  Eventually we want to support vertical-align on SVG text itself, but for now we just map dominant-baseline to vertical-align.

Comment 32

[Cameron McCormack (:heycam)]

Attachment: Part 12: Ignore margins, borders and padding on SVG text frames.

We don't want margin, padding and borders to apply to SVG text.

Comment 33

[Cameron McCormack (:heycam)]

Attachment: Part 14: Ignore float in SVG text frames.

We also don't want floats.

Comment 34

[Cameron McCormack (:heycam)]

Attachment: Part 15: Don't treat SVG text frames as being positioned.

SVG text frames are not positioned.

Comment 35

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

Comment on attachment 644269 [details] [diff] [review]
Part 16: Treat all values of display other than 'none' as 'inline' in SVG text frames. (v1.1)

Review of attachment 644269 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that,

::: layout/generic/nsFrame.cpp
@@ +3392,5 @@
>  
>    // Figure out whether the offsets should be over, after, or before the frame
>    nsRect rect(nsPoint(0, 0), aFrame->GetSize());
>  
> +  bool isSVGText = aFrame->GetStateBits() & NS_FRAME_IS_SVG_TEXT;

you're not using isSVGText in this patch. Move it to another patch?

::: layout/style/nsStyleStructInlines.h
@@ +59,5 @@
> +nsStyleDisplay::IsBlockInside(const nsIFrame* aFrame) const
> +{
> +  if (aFrame && (aFrame->GetStateBits() & NS_FRAME_IS_SVG_TEXT)) {
> +    return aFrame->GetType() == nsGkAtoms::blockFrame;
> +  }

Let's not handle null in these methods.

Comment 36

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

Comment on attachment 644280 [details] [diff] [review]
Part 8: Ignore letter-spacing in SVG text frames.

Review of attachment 644280 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/base/src/nsSVGGlyphFrame.cpp
@@ +1589,5 @@
>        gfxPlatform::GetPlatform()->CreateFontGroup(font.name, &fontStyle, presContext->GetUserFontSet());
>  
>      PRUint32 flags = gfxTextRunFactory::TEXT_NEED_BOUNDING_BOX |
>        GetTextRunFlags(text.Length()) |
> +      nsLayoutUtils::GetTextRunFlagsForStyle(GetStyleContext(), GetStyleFont(), 0);

nsnull instead of 0

Comment 37

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

Comment on attachment 644283 [details] [diff] [review]
Part 11: Ignore vertical-align and map dominant-baseline to vertical-align in SVG text frames.

Review of attachment 644283 [details] [diff] [review]:
-----------------------------------------------------------------

r+ if you make the CalcDifference change in a separate patch.

::: layout/generic/nsFrame.cpp
@@ +7354,5 @@
> +          frame->GetType() == nsGkAtoms::svgTextFrame) {
> +        break;
> +      }
> +    }
> +    return ConvertSVGDominantBaselineToVerticalAlign(dominantBaseline);

I think nsStyleSVGReset::CalcDifference needs to be modified so that a change in mDominantBaseline forces reflow of the entire frame subtree using nsChangeHint_NeedDirtyReflow.

Comment 38

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

You should have a test for dynamic changes to dominant-baseline if there isn't already one.

Comment 39

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

Comment on attachment 644285 [details] [diff] [review]
Part 14: Ignore float in SVG text frames.

Review of attachment 644285 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsBidiPresUtils.cpp
@@ +508,5 @@
>    // Have to special case floating first letter frames because the continuation
>    // doesn't go in the first letter frame. The continuation goes with the rest
>    // of the text that the first letter frame was made out of.
>    if (parent->GetType() == nsGkAtoms::letterFrame &&
> +      parent->GetStyleDisplay()->IsFloatingStyle()) {

Might as well call IsFloating(parent) here. Sure, it doesn't matter, but it's the right thing to do since we have the frame.

::: layout/generic/nsBlockFrame.cpp
@@ +5647,5 @@
>  {
>    NS_PRECONDITION(aPresContext && aChild, "null pointer");
>  
>    if ((aChild->GetStateBits() & NS_FRAME_OUT_OF_FLOW) &&
> +      aChild->GetStyleDisplay()->IsFloatingStyle()) {

Why not IsFloating()? In general there are a bunch of uses of IsFloatingStyle() that could be IsFloating(), I think, and therefore should be.

::: layout/generic/nsFrame.cpp
@@ +2067,5 @@
>      || child->IsTransformed()
>      || nsSVGIntegrationUtils::UsingEffectsForFrame(child);
>  
>    bool isPositioned = disp->IsPositioned();
> +  if (isVisuallyAtomic || isPositioned || disp->IsFloatingStyle() ||

E.g. why is this IsFloatingStyle() and not IsFloating()?

Comment 40

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

Comment on attachment 644286 [details] [diff] [review]
Part 15: Don't treat SVG text frames as being positioned.

Review of attachment 644286 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +4938,5 @@
>  nsIFrame::GetRelativeOffset(const nsStyleDisplay* aDisplay) const
>  {
> +  if (!aDisplay ||
> +      (NS_STYLE_POSITION_RELATIVE == aDisplay->mPosition &&
> +       (mState & NS_FRAME_IS_SVG_TEXT))) {

Missing ! I think.

Worth adding nsIFrame::IsPositioned and nsIFrame::IsRelativelyPositioned?

Comment 41

[Cameron McCormack (:heycam)]

Attachment: Part 11a: Ensure we reflow SVG text when dominant-baseline changes.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #37)
> r+ if you make the CalcDifference change in a separate patch.

Here's the patch for that.  The test I've added to Part 43 where I've got all my tests.

Comment 42

[Cameron McCormack (:heycam)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #39)
> ::: layout/generic/nsBlockFrame.cpp
> @@ +5647,5 @@
> >  {
> >    NS_PRECONDITION(aPresContext && aChild, "null pointer");
> >  
> >    if ((aChild->GetStateBits() & NS_FRAME_OUT_OF_FLOW) &&
> > +      aChild->GetStyleDisplay()->IsFloatingStyle()) {
> 
> Why not IsFloating()? In general there are a bunch of uses of
> IsFloatingStyle() that could be IsFloating(), I think, and therefore should
> be.

For cases where there is already an nsStyleDisplay in a local variable (or maybe passed in to the function), should I prefer to call disp->IsFloating(frame) rather than frame->IsFloating()?

And is it much of a performance win to pass around these style structs instead of grabbing them off the frame with GetStyleDisplay() as needed?

Comment 43

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

(In reply to Cameron McCormack (:heycam) from comment #42)
> For cases where there is already an nsStyleDisplay in a local variable (or
> maybe passed in to the function), should I prefer to call
> disp->IsFloating(frame) rather than frame->IsFloating()?

I think the former, but in many cases it doesn't matter.

> And is it much of a performance win to pass around these style structs
> instead of grabbing them off the frame with GetStyleDisplay() as needed?

In many cases grabbing a style struct is not as cheap as I would like and has shown up on profiles. "How much of a performance win" is hard to say. I think it's OK to pass around style structs on known hot paths, but otherwise we shouldn't.

Comment 44

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #39)
> Might as well call IsFloating(parent) here. Sure, it doesn't matter, but
> it's the right thing to do since we have the frame.

BTW the reason I prefer avoiding IsFloatingStyle() because in most of the places you're currently using it, we're relying on some reasoning that proves the frame can't be an SVG-text frame. It's simpler to not rely on that reasoning. It also means that people modifying or copying code into contexts applying to SVG-text frames are less likely to introduce bugs.

Comment 45

[Cameron McCormack (:heycam)]

I think that's good reasoning, and yes the reason why I had many IsFloatingStyles in my original patch is because I noticed that we were in for example table frame code, where we'd never end up with SVG text.

Comment 46

[Cameron McCormack (:heycam)]

Attachment: Part 14: Ignore float in SVG text frames. (v1.1)

I've removed most uses of IsFloatingStyle.

Comment 47

[Cameron McCormack (:heycam)]

Attachment: Part 15: Don't treat SVG text frames as being positioned. (v1.1)

I've added nsStyleDisplay::IsPositioned() etc.

Comment 48

[Cameron McCormack (:heycam)]

Attachment: Part 3: Add a frame state bit and anonymous box pseudo for SVG text frames.

Comment 49

[Cameron McCormack (:heycam)]

Attachment: Part 4: Add FCDATA_ flag for SVG text.

Comment 50

[Cameron McCormack (:heycam)]

Attachment: Part 7: Add IsFrameSVGText helper function to nsTextFrame and nsCSSFrameConstructor.

Comment 51

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

Comment on attachment 648189 [details] [diff] [review]
Part 14: Ignore float in SVG text frames. (v1.1)

Review of attachment 648189 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +510,5 @@
>      if (IsFontSizeInflationContainer(this, disp)) {
>        AddStateBits(NS_FRAME_FONT_INFLATION_CONTAINER);
>        if (!GetParent() ||
>            // I'd use NS_FRAME_OUT_OF_FLOW, but it's not set yet.
> +          IsFloating() || disp->IsAbsolutelyPositioned()) {

Let's call disp->IsFloating(this) for speed

@@ +2067,5 @@
>      || child->IsTransformed()
>      || nsSVGIntegrationUtils::UsingEffectsForFrame(child);
>  
>    bool isPositioned = disp->IsPositioned();
> +  if (isVisuallyAtomic || isPositioned || child->IsFloating() ||

Let's call disp->IsFloating(child) for speed

@@ +2175,5 @@
>        rv = aLists.PositionedDescendants()->AppendNewToTop(new (aBuilder)
>            nsDisplayWrapList(aBuilder, child, &list));
>        NS_ENSURE_SUCCESS(rv, rv);
>      }
> +  } else if (child->IsFloating()) {

Let's call disp->IsFloating(child) for speed

@@ +9275,5 @@
>      if (aFrame->GetPrevInFlow())
>        printf(" prev-in-flow");
>      if (disp->IsAbsolutelyPositioned())
>        printf(" abspos");
> +    if (disp->IsFloatingStyle())

Shouldn't this be aFrame->IsFloating?

::: layout/generic/nsHTMLReflowState.cpp
@@ +618,5 @@
>        //      see bug 154892; need to revisit later
>        if (frame->GetPrevInFlow())
>          frameType = NS_CSS_FRAME_TYPE_BLOCK;
>      }
> +    else if (frame->IsFloating()) {

Let's call disp->IsFloating(frame) for speed

::: layout/generic/nsIFrame.h
@@ +2478,5 @@
>    bool IsPseudoStackingContextFromStyle() {
>      const nsStyleDisplay* disp = GetStyleDisplay();
> +    return disp->mOpacity != 1.0f ||
> +           disp->IsPositioned() ||
> +           IsFloating();

Let's call disp->IsFloating(this) for speed

::: layout/style/nsStyleStructInlines.h
@@ +57,5 @@
>  
> +bool
> +nsStyleDisplay::IsFloating(const nsIFrame* aFrame) const
> +{
> +  if (aFrame && (aFrame->GetStateBits() & NS_FRAME_IS_SVG_TEXT)) {

I think we shouldn't check aFrame for null here. That's wasteful. Callers should be responsible for that if necessary (it hardly ever will be necessary).

Comment 52

[Cameron McCormack (:heycam)]

Attachment: Part 10: Ignore text-align and text-align-end in SVG text frames.

Comment 53

[Cameron McCormack (:heycam)]

Attachment: Part 13: Make :first-letter apply to <svg:text>.

Comment 54

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

Comment on attachment 648190 [details] [diff] [review]
Part 15: Don't treat SVG text frames as being positioned. (v1.1)

Review of attachment 648190 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSFrameConstructor.cpp
@@ +3674,5 @@
>        // Now figure out whether newFrame or blockFrame should be the
>        // absolute container.  It should be the latter if it's
>        // positioned, otherwise the former.
>        const nsStyleDisplay* blockDisplay = blockContext->GetStyleDisplay();
> +      if (blockFrame->IsPositioned()) {

Use blockDisplay->IsPositioned(blockFrame)

@@ +3709,5 @@
>  
>      if (bits & FCDATA_FORCE_NULL_ABSPOS_CONTAINER) {
>        aState.PushAbsoluteContainingBlock(nsnull, absoluteSaveState);
>      } else if (!(bits & FCDATA_SKIP_ABSPOS_PUSH) &&
> +               maybeAbsoluteContainingBlock->IsPositioned()) {

Use maybeAbsoluteContainingBlockDisplay->IsPositioned(maybeAbsoluteContainingBlock)

::: layout/generic/nsFrame.cpp
@@ +510,5 @@
>      if (IsFontSizeInflationContainer(this, disp)) {
>        AddStateBits(NS_FRAME_FONT_INFLATION_CONTAINER);
>        if (!GetParent() ||
>            // I'd use NS_FRAME_OUT_OF_FLOW, but it's not set yet.
> +          IsFloating() || IsAbsolutelyPositioned()) {

Use disp->IsAbsolutelyPositioned(this)

@@ +9274,5 @@
>      if (aFrame->GetStateBits() & NS_FRAME_OUT_OF_FLOW)
>        printf(" out-of-flow");
>      if (aFrame->GetPrevInFlow())
>        printf(" prev-in-flow");
> +    if (disp->IsAbsolutelyPositionedStyle())

Shouldn't this be aFrame->IsAbsolutelyPositioned()?

::: layout/generic/nsHTMLReflowState.cpp
@@ +611,5 @@
>                 "Unexpected position style");
>    NS_ASSERTION(frame->GetStyleDisplay()->IsFloatingStyle() ==
>                   disp->IsFloatingStyle(), "Unexpected float style");
>    if (frame->GetStateBits() & NS_FRAME_OUT_OF_FLOW) {
> +    if (frame->IsAbsolutelyPositioned()) {

Use disp->IsAbsolutelyPositiond(frame). This code is very hot.

@@ +1861,5 @@
>  
>      // Compute our offsets if the element is relatively positioned.  We need
>      // the correct containing block width and height here, which is why we need
>      // to do it after all the quirks-n-such above.
> +    if (frame->IsRelativelyPositioned()) {

Use mDisplayDisplay->IsRelativelyPositioned(frame)

::: layout/generic/nsIFrame.h
@@ +2476,5 @@
>     * element.
>     */
>    bool IsPseudoStackingContextFromStyle() {
> +    return GetStyleDisplay()->mOpacity != 1.0f ||
> +           IsPositioned() ||

Use disp->IsPositioned(this)

::: layout/style/nsStyleStructInlines.h
@@ +66,5 @@
>  
> +bool
> +nsStyleDisplay::IsPositioned(const nsIFrame* aFrame) const
> +{
> +  if (aFrame && (aFrame->GetStateBits() & NS_FRAME_IS_SVG_TEXT)) {

Remove null check

@@ +75,5 @@
> +
> +bool
> +nsStyleDisplay::IsRelativelyPositioned(const nsIFrame* aFrame) const
> +{
> +  if (aFrame && (aFrame->GetStateBits() & NS_FRAME_IS_SVG_TEXT)) {

Remove null check

@@ +84,5 @@
> +
> +bool
> +nsStyleDisplay::IsAbsolutelyPositioned(const nsIFrame* aFrame) const
> +{
> +  if (aFrame && (aFrame->GetStateBits() & NS_FRAME_IS_SVG_TEXT)) {

Remove null check

Comment 55

[Cameron McCormack (:heycam)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #51)
> @@ +9275,5 @@
> >      if (aFrame->GetPrevInFlow())
> >        printf(" prev-in-flow");
> >      if (disp->IsAbsolutelyPositioned())
> >        printf(" abspos");
> > +    if (disp->IsFloatingStyle())
> 
> Shouldn't this be aFrame->IsFloating?

It wasn't clear to me whether this should be printing out information just based on style or what the actual behaviour is.  I guess you're right, no point saying it's floating in the debug info if it's not actually going to float.

> ::: layout/style/nsStyleStructInlines.h
> @@ +57,5 @@
> >  
> > +bool
> > +nsStyleDisplay::IsFloating(const nsIFrame* aFrame) const
> > +{
> > +  if (aFrame && (aFrame->GetStateBits() & NS_FRAME_IS_SVG_TEXT)) {
> 
> I think we shouldn't check aFrame for null here. That's wasteful. Callers
> should be responsible for that if necessary (it hardly ever will be
> necessary).

Right, I'll do that for the position-related functions too.

Comment 56

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

Comment on attachment 648195 [details] [diff] [review]
Part 7: Add IsFrameSVGText helper function to nsTextFrame and nsCSSFrameConstructor.

Review of attachment 648195 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrameThebes.cpp
@@ +1442,5 @@
>  
> +static bool
> +IsFrameSVGText(const nsIFrame* aFrame)
> +{
> +  return aFrame->GetStateBits() & NS_FRAME_IS_SVG_TEXT;

Why not just add this to nsIFrame?

Comment 57

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #56)
> Why not just add this to nsIFrame?

Then you can call it in a lot of places, it would be nice.

Comment 58

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

Comment on attachment 648197 [details] [diff] [review]
Part 10: Ignore text-align and text-align-end in SVG text frames.

Review of attachment 648197 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsBlockFrame.cpp
@@ +1587,2 @@
>  {
> +  return (aIsSVGText ||

slightly better to pass an nsIFrame and check its state here, I think.

Comment 59

[Cameron McCormack (:heycam)]

Attachment: Part 14: Ignore float in SVG text frames. (v1.2)

Comment 60

[Cameron McCormack (:heycam)]

Attachment: Part 15: Don't treat SVG text frames as being positioned. (v1.2)

Comment 61

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

Comment on attachment 648204 [details] [diff] [review]
Part 14: Ignore float in SVG text frames. (v1.2)

Review of attachment 648204 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that

::: layout/generic/nsHTMLReflowState.cpp
@@ +618,5 @@
>        //      see bug 154892; need to revisit later
>        if (frame->GetPrevInFlow())
>          frameType = NS_CSS_FRAME_TYPE_BLOCK;
>      }
> +    else if (frame->IsFloating()) {

disp->IsFloating(frame) (you missed this comment)

::: layout/style/nsStyleStructInlines.h
@@ +60,5 @@
> +{
> +  if (aFrame->GetStateBits() & NS_FRAME_IS_SVG_TEXT) {
> +    return false;
> +  }
> +  return IsFloatingStyle();

Actually I think this would be slightly better written as
  return IsFloatingStyle() && !(aFrame->GetStateBits() & NS_FRAME_IS_SVG_TEXT);
just because IsFloatingStyle() will normally be false, so evaluation should be slightly quicker this way.

Comment 62

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

Comment on attachment 648205 [details] [diff] [review]
Part 15: Don't treat SVG text frames as being positioned. (v1.2)

Review of attachment 648205 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that

::: layout/style/nsStyleStructInlines.h
@@ +69,5 @@
> +{
> +  if ((aFrame->GetStateBits() & NS_FRAME_IS_SVG_TEXT)) {
> +    return false;
> +  }
> +  return IsPositionedStyle();

As in part 14, I think it's better to test Is...PositionedStyle first. Plus you can write this as a boolean expression.

Comment 63

[Cameron McCormack (:heycam)]

Attachment: Part 7: Add IsFrameSVGText helper function to nsIFrame. (v1.1)

Comment 64

[Cameron McCormack (:heycam)]

Attachment: Part 10: Ignore text-align and text-align-end in SVG text frames. (v1.1)

Comment 65

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

Comment on attachment 648216 [details] [diff] [review]
Part 7: Add IsFrameSVGText helper function to nsIFrame. (v1.1)

Review of attachment 648216 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that

::: layout/generic/nsIFrame.h
@@ +2809,5 @@
>      VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY = 0x01
>    };
>    bool IsVisibleConsideringAncestors(PRUint32 aFlags = 0) const;
>  
> +  bool IsFrameSVGText() const { return mState & NS_FRAME_IS_SVG_TEXT; }

IsSVGText. "Frame" is redundant.

Comment 66

[Cameron McCormack (:heycam)]

Parts 3, 4, 7, 8, 9, 10, 11, 11a, 12, 13, 14, 15, 16:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7bb6717bc95f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5b4d1a78f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5d5270fbd92
https://hg.mozilla.org/integration/mozilla-inbound/rev/57377260ce4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/12d4dbe798eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddf80ca67833
https://hg.mozilla.org/integration/mozilla-inbound/rev/19cd8d99dfbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/58c16e8cfdc7
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4f26647c555
https://hg.mozilla.org/integration/mozilla-inbound/rev/4aeedcac9ecb
https://hg.mozilla.org/integration/mozilla-inbound/rev/3efee683c154
https://hg.mozilla.org/integration/mozilla-inbound/rev/b99801e9a0c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7749fadb594

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7bb6717bc95f
https://hg.mozilla.org/mozilla-central/rev/5d5b4d1a78f3
https://hg.mozilla.org/mozilla-central/rev/b5d5270fbd92
https://hg.mozilla.org/mozilla-central/rev/57377260ce4f
https://hg.mozilla.org/mozilla-central/rev/12d4dbe798eb
https://hg.mozilla.org/mozilla-central/rev/ddf80ca67833
https://hg.mozilla.org/mozilla-central/rev/19cd8d99dfbe
https://hg.mozilla.org/mozilla-central/rev/58c16e8cfdc7
https://hg.mozilla.org/mozilla-central/rev/f4f26647c555
https://hg.mozilla.org/mozilla-central/rev/4aeedcac9ecb
https://hg.mozilla.org/mozilla-central/rev/3efee683c154
https://hg.mozilla.org/mozilla-central/rev/b99801e9a0c0
https://hg.mozilla.org/mozilla-central/rev/d7749fadb594

Comment 68

[Cameron McCormack (:heycam)]

Attachment: Part 17: Ensure non-SVG child elements of SVG text elements do not get frames.

Comment 69

[Cameron McCormack (:heycam)]

Attachment: Part 18: Ensure even line-ending white space SVG text frames are created.

Comment 70

[Cameron McCormack (:heycam)]

Attachment: Part 21: Avoid assertions when nsStyleContext::GetVisitedDependentColor is called for an SVG paint property.

This is to allow the frame's normal painting code path to look at 'fill' instead of 'color'.

Comment 71

[Cameron McCormack (:heycam)]

Attachment: Part 20: Make nsTextFrame QueryFrame-able.

Comment 72

[Cameron McCormack (:heycam)]

Attachment: Part 22: Add function for getting the 'is SVG text using CSS frames' pref.

Comment 73

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 648621 [details] [diff] [review]
Part 18: Ensure even line-ending white space SVG text frames are created.

r=me

Comment 74

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 648620 [details] [diff] [review]
Part 17: Ensure non-SVG child elements of SVG text elements do not get frames.

r=me.  I assume that there' a test that fails without this?  Similar for part 18, by the way.

Comment 75

[Cameron McCormack (:heycam)]

I've got a test for the invalid child (in part 43 where I've got all my tests), but not for the line-ending white space one.  I'll make one.

Comment 76

[Cameron McCormack (:heycam)]

Attachment: Part 23: Add UA style sheet rules to map xml:space='preserve' to white-space:pre on SVG text elements.

My current plan is to treat xml:space="preserve" just like white-space="pre", but I am having second thoughts.  It would mean that markup like

<text x="100" y="100" xml:space="preserve">
  abc
  def
</text>

would change from being rendered as:

  |  abc  def

to:

  |  abc
  |  def

where the "a" is placed at (100,100) in both cases.

There isn't a value of white-space that corresponds nicely to SVG's xml:space="preserve" rules.  And I don't want to keep duplicated special SVG white space handling code.  Do you think instead I should be adding say a -moz-something value for white-space that means "don't collapse spaces but don't insert line breaks either", and mapping xml:space="preserve" to that?

Comment 77

[Cameron McCormack (:heycam)]

Attachment: Part 29: Don't underline links within SVG text by default.

Comment 78

[David Baron :dbaron:]

Comment on attachment 648622 [details] [diff] [review]
Part 21: Avoid assertions when nsStyleContext::GetVisitedDependentColor is called for an SVG paint property.

Given that nsStyleAnimation::Value has a nontrivial copy-constructor, I'd rather you declare |val| in ExtractColor and ExtractColorLenient and make ExtractAnimationValue take an nsStyleAnimation::Value& and return void.

r=dbaron with that

Comment 79

[David Baron :dbaron:]

Comment on attachment 648632 [details] [diff] [review]
Part 29: Don't underline links within SVG text by default.

Though it doesn't matter now, I'd rather you put the :not(svg|a) after the :-moz-any-link in case we change things so it does matter.

(Would it make sense to use :not(svg|*) rather than not(svg|a)?)

r=dbaron with that

Comment 80

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] from comment #79)
> (Would it make sense to use :not(svg|*) rather than not(svg|a)?)

I don't think any element other than <svg:a> is going to be :-moz-any-link, but it would work just as well to use "*".

Comment 81

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47f0da584ea3
https://hg.mozilla.org/integration/mozilla-inbound/rev/270c17de5f45
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8c6025c0881
https://hg.mozilla.org/integration/mozilla-inbound/rev/6899651a0f09
https://hg.mozilla.org/integration/mozilla-inbound/rev/05848864b5f9

Comment 82

[Cameron McCormack (:heycam)]

Attachment: Part 26: Give nsTextFrames the ability to invoke callbacks after painting different parts of the text.

This adds all the callbacks necessary for SVG text frames to be notified to paint the different parts of an nsTextFrame.  (I mistakenly included nsTextFrame::PathCallback in the Part 25 patch; I've got it in here now with it and its member functions renamed and commented.)

Comment 83

[Cameron McCormack (:heycam)]

Attachment: Part 27: Ignore text-shadow in SVG text frames.

Not clear whether we want text-shadow to work with SVG text, so disable it for now.  (It already wouldn't work when the "drawing text as paths" code path is used.)

Comment 84

[Cameron McCormack (:heycam)]

Attachment: Part 24: Add UA style sheet rule to inherit unicode-bidi on <svg:text> to its anonymous block child.

We want to treat <text> as a block for the purpose of unicode-bidi, but the real block is the anonymous block child.  So we inherit the value of unicode-bidi into it.  (This uses up one of the 64 bidi embedding levels, but I don't think that's a real concern.)

Comment 85

[Cameron McCormack (:heycam)]

Attachment: Part 26: Give nsTextFrames the ability to invoke callbacks after painting different parts of the text. (v1.1)

(Forgot some changes.)

Comment 86

[Cameron McCormack (:heycam)]

Attachment: Part 28: Paint SVG text frames using 'fill' not 'color'.

Comment 87

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/47f0da584ea3
https://hg.mozilla.org/mozilla-central/rev/270c17de5f45
https://hg.mozilla.org/mozilla-central/rev/d8c6025c0881
https://hg.mozilla.org/mozilla-central/rev/6899651a0f09
https://hg.mozilla.org/mozilla-central/rev/05848864b5f9

Comment 88

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

Comment on attachment 649068 [details] [diff] [review]
Part 27: Ignore text-shadow in SVG text frames.

Review of attachment 649068 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +1998,5 @@
>                                         nsIFrame* aFrame,
>                                         PRUint32 aFlags)
>  {
>    const nsStyleText* textStyle = aFrame->GetStyleText();
> +  if (!textStyle->mTextShadow || aFrame->IsSVGText())

Let's have a helper function nsStyleText::HasTextShadow(nsIFrame*)

Comment 89

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

Comment on attachment 649072 [details] [diff] [review]
Part 28: Paint SVG text frames using 'fill' not 'color'.

Review of attachment 649072 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrameThebes.cpp
@@ +3414,5 @@
> +        return NS_RGBA(0, 0, 0, 0);
> +      case eStyleSVGPaintType_Color:
> +        return nsLayoutUtils::GetColor(mFrame, eCSSProperty_fill);
> +      default:
> +        return NS_RGBA(0, 0, 0, 255);

Should we have an NS_ERROR here to indicate that this should never be reached?

Comment 90

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

Comment on attachment 649070 [details] [diff] [review]
Part 26: Give nsTextFrames the ability to invoke callbacks after painting different parts of the text. (v1.1)

Review of attachment 649070 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrame.h
@@ +278,5 @@
> +   * Object with various callbacks for PaintText() to invoke for different parts
> +   * of the frame's text rendering, when we're generating paths rather than
> +   * painting.
> +   */
> +  struct DrawCallbacks : gfxTextRun::DrawCallbacks

Should we call this DrawPathCallbacks to indicate that we always draw paths when it's used?

@@ +330,5 @@
> +    /**
> +     * Called just after a path corresponding to a selection decoration line
> +     * has been emitted to the gfxContext.
> +     */
> +    virtual void NotifySelectionDecorationLinePathEmitted() { }

Can you have a summary comment describing the possible sequences/orderings of callbacks? Something like a regular expression :-)

Comment 91

[Cameron McCormack (:heycam)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #89)
> ::: layout/generic/nsTextFrameThebes.cpp
> @@ +3414,5 @@
> > +        return NS_RGBA(0, 0, 0, 0);
> > +      case eStyleSVGPaintType_Color:
> > +        return nsLayoutUtils::GetColor(mFrame, eCSSProperty_fill);
> > +      default:
> > +        return NS_RGBA(0, 0, 0, 255);
> 
> Should we have an NS_ERROR here to indicate that this should never be
> reached?

It seems there are a couple of places where GetTextColor() will be called even though we're painting the text with the path callbacks.  The main one is in nsTextFrame::PaintText, and that could easily be moved inside the `if (!aDrawCallbacks)` block.  One that is more troublesome is nsTextPaintStyle::InitSelectionStyle.  On my platform, IME decorations are being painted with the "40% of the foreground" colour, and I haven't got any special handling for that case when the text is being painted with a paint server fill.  Returning solid black is at least painting something (a grey underline).  Do you think we can ignore that case for now and allow the selection decoration to paint with that fallback black colour?

Comment 92

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

We could, but isn't it easy to fix?

Comment 93

[Cameron McCormack (:heycam)]

Just needs some changes to nsSelectionStyle to record that we are using NS_SAME_AS_FOREGROUND_COLOR or NS_40PERCENT_FOREGROUND_COLOR so we can pass that information in to NotifySelectionDecorationLinePathEmitted, and not call GetTextColor from within nsTextPaintStyle::InitSelectionStyle when we're painting with paths (which nsTextPaintStyle then has to know about).  So, not hard. :)

Comment 94

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

Now that you've figured it out, you might as well do it :-)

Comment 95

[Cameron McCormack (:heycam)]

Attachment: Part 27: Ignore text-shadow in SVG text frames. (v1.1)

Comment 96

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

Comment on attachment 649520 [details] [diff] [review]
Part 27: Ignore text-shadow in SVG text frames. (v1.1)

Review of attachment 649520 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrameThebes.cpp
@@ +5269,5 @@
>                             &foreground, &background);
>      gfxPoint textBaselinePt(aFramePt.x + xOffset, aTextBaselinePt.y);
>  
>      // Draw shadows, if any
> +    if (textStyle->mTextShadow && !IsSVGText()) {

Use your new function!

@@ +5631,5 @@
>  
>    nscolor foregroundColor = textPaintStyle.GetTextColor();
>    if (!aCallbacks) {
>      const nsStyleText* textStyle = GetStyleText();
> +    if (textStyle->mTextShadow && !IsSVGText()) {

Use your new function!

Comment 97

[Cameron McCormack (:heycam)]

Oops, actually I saw those when I generated the patch and fixed them to use the function, but forgot to regenerate the patch...

Comment 98

[Cameron McCormack (:heycam)]

Attachment: Part 26: Give nsTextFrames the ability to invoke callbacks after painting different parts of the text. (v1.2)

I've changed this to pass in the colour that the different parts of the text frame will be painted with into the callback functions, rather than just setting it on the context.  That's clearer, and it also lets us pass in the special selection style color constants (like NS_SAME_AS_FOREGROUND_COLOR and NS_40PERCENT_FOREGROUND_COLOR).  NS_SAME_AS_FOREGROUND_COLOR will be interpreted by nsSVGTextFrame2's callbacks as meaning "fill with 'fill' and then stroke with 'stroke'", whereas any other color value will be interpreted as meaning "fill with the specified color and don't stroke".

So now nsTextPaintStyle can be used in a "non-resolving" mode, where the special color values like NS_SAME_AS_FOREGROUND_COLOR are not resolved down into the actual color.  We set this mode when a DrawPathCallbacks object is passed in to PaintText().

Comment 99

[Cameron McCormack (:heycam)]

Attachment: Part 28: Paint SVG text frames using 'fill' not 'color'. (v1.1)

I've changed GetTextDecorations to have a non-colour-resolving mode too, so that the callbacks can be told to paint a text decoration with NS_SAME_AS_FOREGROUND_COLOR.

Comment 100

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

Comment on attachment 649524 [details] [diff] [review]
Part 26: Give nsTextFrames the ability to invoke callbacks after painting different parts of the text. (v1.2)

Review of attachment 649524 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrameThebes.cpp
@@ +268,5 @@
>   * These live for just the duration of one paint operation.
>   */
>  class nsTextPaintStyle {
>  public:
> +  nsTextPaintStyle(nsTextFrame* aFrame, bool aResolveColors);

Use named flag instead of bool parameter.

Comment 101

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #100)
> Use named flag instead of bool parameter.

Better still, instead of setting it in the constructor, just have a method SetResolveColors(bool).

Comment 102

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

Comment on attachment 649527 [details] [diff] [review]
Part 28: Paint SVG text frames using 'fill' not 'color'. (v1.1)

Review of attachment 649527 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrame.h
@@ +597,5 @@
>      }
>    };
>    void GetTextDecorations(nsPresContext* aPresContext,
> +                          TextDecorations& aDecorations,
> +                          bool aResolveColors);

This does need a flags word with named flag :-)

Comment 103

[Cameron McCormack (:heycam)]

Attachment: Part 19: Add function to convert text decorations to a path.

Comment 104

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

Comment on attachment 649558 [details] [diff] [review]
Part 19: Add function to convert text decorations to a path.

Review of attachment 649558 [details] [diff] [review]:
-----------------------------------------------------------------

Actually the ideal thing here would be to have generic functionality for converting a stroke to its path outline, and just use that with our existing text-decoration-line code. (We need that functionality anyway to implement proposed 2D canvas extensions.)

Comment 105

[Cameron McCormack (:heycam)]

Attachment: Part 32: Allow nsCharClipDisplayItems to be constructed without an nsDisplayListBuilder.

We need to pass in an nsCharClipDisplayItem to nsTextFrame::PaintText, so we need to be able to create one outside of nsDisplayListBuilder.

Comment 106

[Cameron McCormack (:heycam)]

Attachment: Part 37: Reflow SVG text even if undisplayed content is added/removed.

Comment 107

[Cameron McCormack (:heycam)]

Attachment: Part 40: Factor out 'distance from point to rect' calculation from text frame selection routine.

I've taken this out so that nsSVGTextFrame2 can call it to determine the closest part of a text frame to a point (used when handling selection with the mouse), since parts of text frames get rendered in different places due to SVG glyph positioning.  I've made it a template because I have my text frame parts in SVG user units.

Comment 108

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

Comment on attachment 649574 [details] [diff] [review]
Part 37: Reflow SVG text even if undisplayed content is added/removed.

Review of attachment 649574 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSFrameConstructor.cpp
@@ +6671,5 @@
> +    // reason as when removing frame-less content from SVG text: cached
> +    // information that matches up text frames to content nodes needs
> +    // to be updated.
> +    if (parentFrame->GetType() == nsGkAtoms::svgTextFrame2) {
> +      parentFrame = parentFrame->GetFirstPrincipalChild();

Why is this needed? shouldn't we have found the anonymous child already?

@@ +7281,5 @@
> +    // reason as when removing frame-less content from SVG text: cached
> +    // information that matches up text frames to content nodes needs
> +    // to be updated.
> +    if (parentFrame->GetType() == nsGkAtoms::svgTextFrame2) {
> +      parentFrame = parentFrame->GetFirstPrincipalChild();

As above.

@@ +7355,5 @@
> +    for (nsIContent* parent = aContainer; parent; parent = parent->GetParent()) {
> +      nsIFrame* parentFrame = parent->GetPrimaryFrame();
> +      if (parentFrame && parentFrame->IsSVGText()) {
> +        if (parentFrame->GetType() == nsGkAtoms::svgTextFrame2) {
> +          parentFrame = parentFrame->GetFirstPrincipalChild();

As above

Comment 109

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

Comment on attachment 649575 [details] [diff] [review]
Part 40: Factor out 'distance from point to rect' calculation from text frame selection routine.

Review of attachment 649575 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.h
@@ +1702,5 @@
> +  }
> +
> +  if (xDistance <= aClosestXDistance) {
> +    if (xDistance < aClosestXDistance)
> +      aClosestYDistance = std::numeric_limits<CoordType>::max();

{}

@@ +1711,5 @@
> +    CoordType yDistance;
> +    if (fromTop >= 0 && fromBottom <= 0)
> +      yDistance = 0;
> +    else
> +      yDistance = NS_MIN(abs(fromTop), abs(fromBottom));

{}s

Comment 110

[Jonathan Watt [:jwatt]]

Comment on attachment 648631 [details] [diff] [review]
Part 23: Add UA style sheet rules to map xml:space='preserve' to white-space:pre on SVG text elements.

Looks fine to me, but I don't know what the possible perf implications of selectors like this are. Boris, does this look okay to you?

Comment 111

[Jonathan Watt [:jwatt]]

Comment on attachment 648631 [details] [diff] [review]
Part 23: Add UA style sheet rules to map xml:space='preserve' to white-space:pre on SVG text elements.

Did the reviews before reading the comments, sorry.

(In reply to Cameron McCormack (:heycam) from comment #76)
> Created attachment 648631 [details] [diff] [review]
> Part 23: Add UA style sheet rules to map xml:space='preserve' to
> white-space:pre on SVG text elements.
> 
> My current plan is to treat xml:space="preserve" just like
> white-space="pre", but I am having second thoughts.  It would mean that
> markup like
> 
> <text x="100" y="100" xml:space="preserve">
>   abc
>   def
> </text>
> 
> would change from being rendered as:
> 
>   |  abc  def
> 
> to:
> 
>   |  abc
>   |  def
> 
> where the "a" is placed at (100,100) in both cases.

Hmm, yeah, I think that would be bad.

> There isn't a value of white-space that corresponds nicely to SVG's
> xml:space="preserve" rules.  And I don't want to keep duplicated special SVG
> white space handling code.  Do you think instead I should be adding say a
> -moz-something value for white-space that means "don't collapse spaces but
> don't insert line breaks either", and mapping xml:space="preserve" to that?

I think adding a -moz-xml-space-preserve or something that sounds like the sensible way to handle this. bz and dbaron will probably have some thoughts on that I'd imagine though.

Comment 112

[Cameron McCormack (:heycam)]

(In reply to Jonathan Watt [:jwatt] from comment #111)
> I think adding a -moz-xml-space-preserve or something that sounds like the
> sensible way to handle this. bz and dbaron will probably have some thoughts
> on that I'd imagine though.

That would be the simplest, implementation wise.  Speaking with roc this morning, he thought it might be worth taking the risk of just mapping it to white-space:pre, since there's not likely to be much content out there with multiple lines of xml:space="preserve" text.

I think it would be preferable to have the spec require a particular mapping of xml:space="preserve" to white-space, rather than having to describe the special SVG white space handling on top of the output we get from CSS white-space.  So if we do do this, it might be worth seeing if we can get a standardised version of -moz-xml-space-preserve into css4-text.

Comment 113

[Cameron McCormack (:heycam)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #108)
> ::: layout/base/nsCSSFrameConstructor.cpp
> @@ +6671,5 @@
> > +    // reason as when removing frame-less content from SVG text: cached
> > +    // information that matches up text frames to content nodes needs
> > +    // to be updated.
> > +    if (parentFrame->GetType() == nsGkAtoms::svgTextFrame2) {
> > +      parentFrame = parentFrame->GetFirstPrincipalChild();
> 
> Why is this needed? shouldn't we have found the anonymous child already?

> @@ +7355,5 @@
> > +    for (nsIContent* parent = aContainer; parent; parent = parent->GetParent()) {
> > +      nsIFrame* parentFrame = parent->GetPrimaryFrame();
> > +      if (parentFrame && parentFrame->IsSVGText()) {
> > +        if (parentFrame->GetType() == nsGkAtoms::svgTextFrame2) {
> > +          parentFrame = parentFrame->GetFirstPrincipalChild();
> 
> As above

Not if we have for example <text><tspan display="none">..</tspan></text> and we remove content under the tspan, because we look up the content tree for the first node that has a frame, which will skip the anonymous child.

Looking at this code now, I am not sure why I need to reflow the anonymous child rather than the nsSVGTextFrame2.  But my test case is failing without that part.

Comment 114

[Cameron McCormack (:heycam)]

(The "break" in that loop also seems to be misplaced; it should be inside the `if (parentFrame && parentFrame->IsSVGText())` block.

Comment 115

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

(In reply to Cameron McCormack (:heycam) from comment #113)
> Looking at this code now, I am not sure why I need to reflow the anonymous
> child rather than the nsSVGTextFrame2.

Is it because the nsSVGTextFrame2 doesn't actually support Reflow() itself?

In any case, you need to document this in the code.

Comment 116

[Cameron McCormack (:heycam)]

It does have a Reflow() method.  I'll work it out and add it to the comments.

Comment 117

[Cameron McCormack (:heycam)]

I think it is because I am posting an eTreeChange reflow request, the anonymous frame is a reflow root, and my nsSVGTextFrame2, when it needs update glyph positions, will only reflow the anonymous frame if it is marked dirty or with dirty children.  Does that make sense?

Comment 118

[Cameron McCormack (:heycam)]

Attachment: Part 37: Reflow SVG text even if undisplayed content is added/removed. (v1.1)

Comment 119

[Cameron McCormack (:heycam)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #104)
> Actually the ideal thing here would be to have generic functionality for
> converting a stroke to its path outline, and just use that with our existing
> text-decoration-line code. (We need that functionality anyway to implement
> proposed 2D canvas extensions.)

That would be ideal.  Should we wait for that, or just switch this new nsCSSRendering::DecorationLineToPath to it when it's ready?

Comment 120

[Robert Longson [:longsonr]]

(In reply to Cameron McCormack (:heycam) from comment #112)

When <text> has embedded <tspan> elements folks do seem to like their newlines. E.g. http://code.google.com/p/svg-edit/issues/detail?id=797 which has the following comment... Unfortunately, this type of code is commonly produced by Inkscape.

Will this display correctly?

Comment 121

[Helder "Lthere" Magalhães]

(In reply to Cameron McCormack (:heycam) from comment #112)
> [...] it might be worth taking the risk of just mapping it to
> white-space:pre, since there's not likely to be much content out there with
> multiple lines of xml:space="preserve" text.

We use this quite often (no public test cases, sorry) due to the white-space becoming much more predictable (think "editor with multi-line text support).

(In reply to Robert Longson from comment #120)
> (In reply to Cameron McCormack (:heycam) from comment #112)
> 
> When <text> has embedded <tspan> elements folks do seem to like their
> newlines. [...]  Will this display correctly?

We also use this, in combination with the above, for implementing line breaks. Things like:
<text>
  line 1
  <tspan dy="1.1em">line 2</tspan>
</text>

I don't think <pre>-like will make this kind of content layout as previously as it will (apparently) double the vertical spacing expected. But I might be wrong, as I really didn't dig up the implementation...

Comment 122

[David Mathog]

Will this work resolve the baseline-shift issue  (Bug 308338), which is still unresolved after 7 years?  If yes, is than an estimate of when these changes will make it into production code?

Firefox's lack of super/subscript support for SVG is currently a huge problem for Scientific and Engineering illustrations.

Comment 123

[David Mathog]

Attachment: Test case for baseline-shift support for super/subscript with mixed colors and fonts

This example demonstrates the minimal support required for baseline-shift to be useful in Scientific/Engineering applications.  The absolute offset of the super/sub script characters is not crucial, but the resulting text must rotate, scale, and accept font styles like "italic", as shown in the example.

Comment 124

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

(In reply to Cameron McCormack (:heycam) from comment #119)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment
> #104)
> > Actually the ideal thing here would be to have generic functionality for
> > converting a stroke to its path outline, and just use that with our existing
> > text-decoration-line code. (We need that functionality anyway to implement
> > proposed 2D canvas extensions.)
> 
> That would be ideal.  Should we wait for that, or just switch this new
> nsCSSRendering::DecorationLineToPath to it when it's ready?

How about for now we have your dedicated function but just support solid underlines? That's 99% of text-decoration anyway. Hopefully Jeff will landed stroke_to_path and then we can unify the functions and support all decoration types.

Comment 125

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

(In reply to Cameron McCormack (:heycam) from comment #117)
> I think it is because I am posting an eTreeChange reflow request, the
> anonymous frame is a reflow root, and my nsSVGTextFrame2, when it needs
> update glyph positions, will only reflow the anonymous frame if it is marked
> dirty or with dirty children.  Does that make sense?

Isn't it the nsSVGTextFrame2 that should be a reflow root, so that reflows from the anonymous child bubble up to it and trigger updating of glyph positions?

Comment 126

[Cameron McCormack (:heycam)]

(In reply to Robert Longson from comment #120)
> When <text> has embedded <tspan> elements folks do seem to like their
> newlines. E.g. http://code.google.com/p/svg-edit/issues/detail?id=797 which
> has the following comment... Unfortunately, this type of code is commonly
> produced by Inkscape.
> 
> Will this display correctly?

No, it will display differently.  I think then we need to do something to keep this rendering the same.

Comment 127

[Cameron McCormack (:heycam)]

(In reply to Helder "Lthere" Magalhães from comment #121)
> We also use this, in combination with the above, for implementing line
> breaks. Things like:
> <text>
>   line 1
>   <tspan dy="1.1em">line 2</tspan>
> </text>
> 
> I don't think <pre>-like will make this kind of content layout as previously
> as it will (apparently) double the vertical spacing expected. But I might be
> wrong, as I really didn't dig up the implementation...

If you have xml:space="preserve" on the <text>, yes you will get more vertical space than you expected.

Comment 128

[Cameron McCormack (:heycam)]

(In reply to David Mathog from comment #122)
> Will this work resolve the baseline-shift issue  (Bug 308338), which is
> still unresolved after 7 years?  If yes, is than an estimate of when these
> changes will make it into production code?
> 
> Firefox's lack of super/subscript support for SVG is currently a huge
> problem for Scientific and Engineering illustrations.

It will not make baseline-shift work, but it will make it easier to implement.  With these patches, internally dominant-baseline (the alignment property we do currently support for SVG text) is mapped into an appropriate vertical-align value when performing CSS text layout.  We could do the same thing for baseline-shift.

Comment 129

[Cameron McCormack (:heycam)]

Attachment: Part 19: Add function to convert text decorations to a path. (v1.1)

Now with only solid decorations.

Comment 130

[Cameron McCormack (:heycam)]

Attachment: Part 37: Reflow SVG text even if undisplayed content is added/removed. (v1.2)

Comment 131

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

Comment on attachment 649945 [details] [diff] [review]
Part 37: Reflow SVG text even if undisplayed content is added/removed. (v1.2)

Review of attachment 649945 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSFrameConstructor.cpp
@@ +7277,5 @@
> +    // need to trigger a reflow of the SVG text.  This is for the same
> +    // reason as when removing frame-less content from SVG text: cached
> +    // information that matches up text frames to content nodes needs
> +    // to be updated.
> +    parentFrame->InvalidateOverflowRect();

This shouldn't be necessary. If reflowing the nsSVGTextFrame2 causes the character offsets to be updated, that itself should trigger invalidation.

@@ +7280,5 @@
> +    // to be updated.
> +    parentFrame->InvalidateOverflowRect();
> +    mPresShell->FrameNeedsReflow(parentFrame,
> +                                 nsIPresShell::eTreeChange,
> +                                 NS_FRAME_IS_DIRTY);

Since we only need to reflow the frame and we don't actually need to treat anything as dirty for the purposes of invalidating cached layout data, we can pass NS_FRAME_HAS_DIRTY_CHILD here.

Should probably also use eResize. We want to do the minimum amount work that ensures nsSVGTextFrame2 gets reflowed.

@@ +7346,5 @@
> +    // some cached information that helps map between SVG text frames and
> +    // content nodes.  We do a reflow since that's easier and shouldn't happen
> +    // too often to be too wasteful.
> +    for (nsIContent* parent = aContainer; parent; parent = parent->GetParent()) {
> +      nsIFrame* parentFrame = GetFrameFor(parent);

Looping all the way up to the root seems wasteful for the usual case where no frame IsSVGText(). Why do we have to go up all the way to the root?

Comment 132

[Cameron McCormack (:heycam)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #131)
> ::: layout/base/nsCSSFrameConstructor.cpp
> @@ +7277,5 @@
> > +    // need to trigger a reflow of the SVG text.  This is for the same
> > +    // reason as when removing frame-less content from SVG text: cached
> > +    // information that matches up text frames to content nodes needs
> > +    // to be updated.
> > +    parentFrame->InvalidateOverflowRect();
> 
> This shouldn't be necessary. If reflowing the nsSVGTextFrame2 causes the
> character offsets to be updated, that itself should trigger invalidation.

I tested and it's no longer necessary (I must have had a bug in my invalidation/bounds update code before).

> @@ +7280,5 @@
> > +    // to be updated.
> > +    parentFrame->InvalidateOverflowRect();
> > +    mPresShell->FrameNeedsReflow(parentFrame,
> > +                                 nsIPresShell::eTreeChange,
> > +                                 NS_FRAME_IS_DIRTY);
> 
> Since we only need to reflow the frame and we don't actually need to treat
> anything as dirty for the purposes of invalidating cached layout data, we
> can pass NS_FRAME_HAS_DIRTY_CHILD here.
> 
> Should probably also use eResize. We want to do the minimum amount work that
> ensures nsSVGTextFrame2 gets reflowed.

OK.

> @@ +7346,5 @@
> > +    // some cached information that helps map between SVG text frames and
> > +    // content nodes.  We do a reflow since that's easier and shouldn't happen
> > +    // too often to be too wasteful.
> > +    for (nsIContent* parent = aContainer; parent; parent = parent->GetParent()) {
> > +      nsIFrame* parentFrame = GetFrameFor(parent);
> 
> Looping all the way up to the root seems wasteful for the usual case where
> no frame IsSVGText(). Why do we have to go up all the way to the root?

That's a good point.  Instead we should be finding the frame for the first ancestor content that has one, calling FrameNeedsReflow if it's SVG text, and the breaking out either way.

Comment 133

[Cameron McCormack (:heycam)]

Attachment: Part 31a: Atom for new frame class nsSVGTextFrame2.

Separating this out will let me land some other patches before the actual frame class lands.

Comment 134

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

> > Looping all the way up to the root seems wasteful for the usual case where
> > no frame IsSVGText(). Why do we have to go up all the way to the root?
> 
> That's a good point.  Instead we should be finding the frame for the first
> ancestor content that has one, calling FrameNeedsReflow if it's SVG text,
> and the breaking out either way.

That would be better, but can we avoid looping up to the first ancestor with a frame? We only need to do this if aContainer is an SVG text-related element, right?

Comment 135

[Cameron McCormack (:heycam)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #134)
> That would be better, but can we avoid looping up to the first ancestor with
> a frame? We only need to do this if aContainer is an SVG text-related
> element, right?

The cached count of undisplayed characters in the DOM includes non-SVG elements too, e.g. if you have <text x="10 20 30"><foo xmlns="something">ab</foo>c</text> then the "c" is going to be at x=30.  If you removed the "ab" node then I think we still want it to find the <text> and reflow its anonymous frame.

Comment 136

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

Your ContentAppended/ContentRangeInserted changes aren't actually correct then, if you append/insert in a subtree where the immediate parent element has no frame? We'll exit early in ContentAppended before we reach your code.

Comment 137

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

I wonder whether having the SVG <text> element use nsStubMutationObserver to watch its subtree would be better than trying to do this in nsCSSFrameConstructor.

BTW, how is it possible to compute the character offset map during reflow, when the overflow area of your frames must depend on that map?

Comment 138

[Cameron McCormack (:heycam)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #136)
> Your ContentAppended/ContentRangeInserted changes aren't actually correct
> then, if you append/insert in a subtree where the immediate parent element
> has no frame? We'll exit early in ContentAppended before we reach your code.

Indeed you're right, my test was missing that case.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #137)
> I wonder whether having the SVG <text> element use nsStubMutationObserver to
> watch its subtree would be better than trying to do this in
> nsCSSFrameConstructor.

I can try that.  I'm guessing we do want to avoid traversing up the tree when removing a non-displayed subtree from something that is also non-displayed, since that should be a really cheap operation.

> BTW, how is it possible to compute the character offset map during reflow,
> when the overflow area of your frames must depend on that map?

My call flow is:

  UpdateBounds()
    -> UpdateGlyphPositioning()
         -> DoReflow()
              -> kid->Reflow()
              -> RecordCorrespondence()  // update the map
         -> DoGlyphPositioning()         // depends on the map
    ... compute the overflow rect...     // also depends on the map
    -> FinishAndStoreOverflow()

So the overflow area of the nsSVGTextFrame2 depends on the map, but I don't mess with the overflow rects of the descendants of the anonymous frame.  Does that make sense?

Comment 139

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

Yes.

Comment 140

[Cameron McCormack (:heycam)]

Comment on attachment 649945 [details] [diff] [review]
Part 37: Reflow SVG text even if undisplayed content is added/removed. (v1.2)

I've switched to using nsStubMutationObserver, so this part is no longer necessary.

Now I am calling FrameNeedsReflow unconditionally from inside the ContentAppended/ContentInserted/ContentRemoved callbacks.  Is that going to be wasteful?  Should I be checking that it is a non-displayed subtree and only calling FrameNeedsReflow then?

Comment 141

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

I think if it's simpler to just always call FrameNeedsReflow, we should do that.

Comment 142

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fcde7b899e87
https://hg.mozilla.org/integration/mozilla-inbound/rev/856faff0f587
https://hg.mozilla.org/integration/mozilla-inbound/rev/a90b2757ccc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd0e5c739347
https://hg.mozilla.org/integration/mozilla-inbound/rev/e45aca2a5d87
https://hg.mozilla.org/integration/mozilla-inbound/rev/342718ea6734
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cd1bb53c262
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f477f32c157

Comment 143

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76835a9e3efb

Comment 144

[David Mathog]

(In reply to Cameron McCormack (:heycam) from comment #128)
> (In reply to David Mathog from comment #122)
> > Will this work resolve the baseline-shift issue  (Bug 308338), which is
> > still unresolved after 7 years?  If yes, is than an estimate of when these
> > changes will make it into production code?
>
> It will not make baseline-shift work, but it will make it easier to
> implement.  With these patches, internally dominant-baseline (the alignment
> property we do currently support for SVG text) is mapped into an appropriate
> vertical-align value when performing CSS text layout.  We could do the same
> thing for baseline-shift.

Time frame?  

I started working on implementing baseline-shift, in a manner very similar to dominant-baseline, and then discovered this thread, so stopped work.  No point modifying the current code if my changes will be immediately broken by these planned changes.

Comment 145

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fcde7b899e87
https://hg.mozilla.org/mozilla-central/rev/856faff0f587
https://hg.mozilla.org/mozilla-central/rev/a90b2757ccc2
https://hg.mozilla.org/mozilla-central/rev/dd0e5c739347
https://hg.mozilla.org/mozilla-central/rev/e45aca2a5d87
https://hg.mozilla.org/mozilla-central/rev/342718ea6734
https://hg.mozilla.org/mozilla-central/rev/5cd1bb53c262
https://hg.mozilla.org/mozilla-central/rev/9f477f32c157
https://hg.mozilla.org/mozilla-central/rev/76835a9e3efb

Comment 146

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 648631 [details] [diff] [review]
Part 23: Add UA style sheet rules to map xml:space='preserve' to white-space:pre on SVG text elements.

The selector here would only be evaluated for SVG elements, so would probably be OK in general....

Comment 147

[Cameron McCormack (:heycam)]

(In reply to David Mathog from comment #144)
> Time frame?  

First I would like to know if implementing baseline-shift is what we want to do.  SVG (in its wisdom) uses these various split-out alignment properties that XSL defined, rather than vertical-align.  This bug won't do it, but we should allow vertical-align to directly control the alignment of SVG text too, and work out how dominant-baseline, baseline-shift, etc. and vertical-align interact.  In css3-linebox, dominant-baseline and so on are defined as properties for which vertical-align is a shorthand.  But I don't know whether we plan to implement those split out properties any time soon for general CSS content, or how stable that part of the spec is.

I thought implementing baseline-shift by itself would be straightforward, although in combination with dominant-baseline it would not be, since there's no vertical-align value we can map to internally that means for example "hanging + 10px", which is what "dominant-baseline: hanging; baseline-shift: 10px" would mean).

Comment 148

[David Baron :dbaron:]

See bug 308338 comment 28 for my opinion (which I think I still agree with) on what we should do about baseline-shift.  (And the comments preceding it for some rationale.)

Comment 149

[Cameron McCormack (:heycam)]

Attachment: Part 23a: Add white-space:-moz-pre-one-line value with white space collapsing behavior like SVG's xml:space="preserve".

I think we need to handle xml:space="preserve" as it's currently defined in the SVG spec, then.  This patch adds a -moz-pre-one-line value for white-space that implements this behaviour.  (I can worry about whether we should add this to css4-text, or if the SVG spec should define this special newline discarding behaviour on top of CSS white space processing, later when I get to rewriting the SVG 2 Text chapter.)

Comment 150

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

Comment on attachment 650459 [details] [diff] [review]
Part 23a: Add white-space:-moz-pre-one-line value with white space collapsing behavior like SVG's xml:space="preserve".

Review of attachment 650459 [details] [diff] [review]:
-----------------------------------------------------------------

Seems fine to me, although I'm not sure about the name of the constant. I think NS_STYLE_WHITESPACE_PRE_DISCARD_NEWLINES might be better. Up to dbaron.

::: layout/generic/nsTextFrameUtils.cpp
@@ +67,5 @@
>        PRUnichar ch = *aText++;
>        if (IsDiscardable(ch, &flags)) {
>          aSkipChars->SkipChar();
>        } else {
> +        if (ch == '\n' && aCompression == DISCARD_NEWLINE) {

See below

@@ +174,5 @@
>          aSkipChars->SkipChar();
>        } else {
> +        if (ch == '\n' && aCompression == DISCARD_NEWLINE) {
> +          aSkipChars->SkipChar();
> +          continue;

Instead of using "continue", just || this condition after IsDiscardable.

Comment 151

[David Baron :dbaron:]

Comment on attachment 650459 [details] [diff] [review]
Part 23a: Add white-space:-moz-pre-one-line value with white space collapsing behavior like SVG's xml:space="preserve".

Asking fantasai for feedback on this, if only so that she's aware of it.

Comment 152

[David Mathog]

(In reply to David Baron [:dbaron] from comment #148)
> See bug 308338 comment 28 for my opinion (which I think I still agree with)
> on what we should do about baseline-shift.  (And the comments preceding it
> for some rationale.)

Honestly I don't care how it is implemented, just that it is, and preferably sooner rather than later.  

Chrome supposedly does baseline-shift OK.  Perhaps have a gander at webkit and see how they went about it?

Comment 153

[Cameron McCormack (:heycam)]

Attachment: Part 1: Move some path drawing functions from nsSVGGeometryFrame to nsSVGUtils. (v1.1)

This one's ready for review now.  The diff is split up to make it easier to review the changes after moving; e.g. the first part that touches nsSVGUtils.cpp just moves all the functions over verbatim, and the second part fixes the functions up.

Comment 154

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b32dbbd0df6f

Comment 155

[Cameron McCormack (:heycam)]

Attachment: Part 30: Allow PathExtentsToMaxStrokeExtents to work with nsTextFrames.

This is needed so that nsSVGTextFrame2 get bounding boxes / covered regions of nsTextFrame children.

Comment 156

[Robert Longson [:longsonr]]

(In reply to Cameron McCormack (:heycam) from comment #153)
> Created attachment 650789 [details] [diff] [review]
> Part 1: Move some path drawing functions from nsSVGGeometryFrame to
> nsSVGUtils. (v1.1)
> 
> This one's ready for review now.  The diff is split up to make it easier to
> review the changes after moving; e.g. the first part that touches
> nsSVGUtils.cpp just moves all the functions over verbatim, and the second
> part fixes the functions up.

Some of these have already been moved by bug 619964. I think you need to refresh this one.

Comment 157

[Cameron McCormack (:heycam)]

I rebased it this morning and did notice that e.g. SetupCairoFill had already moved; I think the patch is up to date.

Comment 158

[Cameron McCormack (:heycam)]

Attachment: Part 39: Get bounding boxes of SVG text using new text frames correctly.

nsSVGTextFrame2 doesn't implement nsSVGTextContainerFrame, so we need to go searching for the closest ancestor nsSVGTextFrame2 if we request a bbox for an ns{Block,Inline,Text}Frame instead.

Comment 159

[Helder "Lthere" Magalhães]

I was trying to play with this but don't seem to get this code enabled in nightly builds. According to comment #72, I've created a new boolean preference "svg.text.css-frames.enabled" set to true, but nothing happened. Am I missing something?

(In reply to Cameron McCormack (:heycam) from comment #127)
> (In reply to Helder "Lthere" Magalhães from comment #121)
> > I don't think <pre>-like will make this kind of content layout as previously
> > as it will (apparently) double the vertical spacing expected. But I might be
> > wrong, as I really didn't dig up the implementation...
> 
> If you have xml:space="preserve" on the <text>, yes you will get more
> vertical space than you expected.

Oh-oh... :-|

Comment 160

[Cameron McCormack (:heycam)]

(In reply to Helder "Lthere" Magalhães from comment #159)
> I was trying to play with this but don't seem to get this code enabled in
> nightly builds. According to comment #72, I've created a new boolean
> preference "svg.text.css-frames.enabled" set to true, but nothing happened.
> Am I missing something?

You'll have to wait for all the parts to be reviewed and landed. :)

> (In reply to Cameron McCormack (:heycam) from comment #127)
> > (In reply to Helder "Lthere" Magalhães from comment #121)
> > If you have xml:space="preserve" on the <text>, yes you will get more
> > vertical space than you expected.
> 
> Oh-oh... :-|

Don't worry, the part 23a patch should keep the original xml:space="preserve" layout behaviour.

Comment 161

[Helder "Lthere" Magalhães]

(In reply to Cameron McCormack (:heycam) from comment #160)
> (In reply to Helder "Lthere" Magalhães from comment #159)
> > [...]
> > Am I missing something?
> 
> You'll have to wait for all the parts to be reviewed and landed. :)

Oops, sorry for the rush/noise. ;-)


> > (In reply to Cameron McCormack (:heycam) from comment #127)
> > > (In reply to Helder "Lthere" Magalhães from comment #121)
> > Oh-oh... :-|
> 
> Don't worry, the part 23a patch should keep the original
> xml:space="preserve" layout behaviour.

Great, didn't notice it. Look forward to see this landing! :-)

Comment 162

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d65dc56b7243
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb0d0666b2bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/60f0b304f8c6

Comment 163

[Cameron McCormack (:heycam)]

Attachment: Part 38: Ignore the non-SVG frames when propagating SVG changes through a tree.

We'll encounter NS_FRAME_IS_SVG_TEXT frames as we propagate SVG changes down the tree, so we shouldn't assert if we find them.

Comment 164

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b32dbbd0df6f
https://hg.mozilla.org/mozilla-central/rev/d65dc56b7243
https://hg.mozilla.org/mozilla-central/rev/eb0d0666b2bf
https://hg.mozilla.org/mozilla-central/rev/60f0b304f8c6

Comment 165

[fantasai]

Comment on attachment 650459 [details] [diff] [review]
Part 23a: Add white-space:-moz-pre-one-line value with white space collapsing behavior like SVG's xml:space="preserve".

Wrt comment 151, filed http://www.w3.org/Style/CSS/Tracker/issues/274 against CSS Text. Not sure it's needed outside the UA stylesheet, though.

Comment 166

[Cameron McCormack (:heycam)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/8c95a0d692fa

Comment 167

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8c95a0d692fa

Comment 168

[Cameron McCormack (:heycam)]

Attachment: Part 23a: Add white-space:-moz-pre-discard-newlines value with white space collapsing behavior like SVG's xml:space="preserve". (v1.1)

I've gone with the property value renaming that roc suggested.

Comment 169

[Jesse Ruderman]

Comment on attachment 654901 [details] [diff] [review]
Part 23a: Add white-space:-moz-pre-discard-newlines value with white space collapsing behavior like SVG's xml:space="preserve". (v1.1)

Review of attachment 654901 [details] [diff] [review]:
-----------------------------------------------------------------

Please add the new value to layout/style/test/property_database.js so the DOM fuzzer can pick it up.

Comment 170

[Cameron McCormack (:heycam)]

(In reply to Jesse Ruderman from comment #169)
> Please add the new value to layout/style/test/property_database.js so the
> DOM fuzzer can pick it up.

OK.

Comment 171

[Cameron McCormack (:heycam)]

Attachment: Part 12a: Don't paint borders on SVG text frames when using display lists.

Since switching to painting SVG with display lists, we need this check here too.

Comment 172

[Cameron McCormack (:heycam)]

Attachment: Part 31d: Hook SVG text scaling into CSS text frames.

This is how we will handle scaling text to handle very small or very large font sizes.  nsSVGTextFrame2 will return a scaling factor to apply to the font-size of all of its text frames.

Comment 173

[Cameron McCormack (:heycam)]

Attachment: Part 33: Allow new SVG text frames in clip paths.

Comment 174

[Cameron McCormack (:heycam)]

Attachment: Part 31c: Ensure SVG text with effects applied to them are in the right position.

nsSVGTextFrame2s will handle their positioning themselves, like SVG geometry frames.

Comment 175

[Cameron McCormack (:heycam)]

Attachment: Part 34: Implement SVG DOM text methods in terms of the new SVG text frame.

This makes all of the SVG DOM methods on nsSVGText{Content,}Element switch their implementations to call into the nsSVGTextContainerFrame or nsSVGTextFrame2, depending on whether the "use CSS text frames" pref is set.  The nsSVGTextFrame2 methods all take an nsIContent* argument for the particular <text>, <tspan>, etc. element that it was called on, since it is handling the text layout for all of its descendants.  (See patch 31b at http://mcc.id.au/temp/bug-655877/ for the not yet cleaned up nsSVGTextFrame2, if you need to.)

Comment 176

[Robert Longson [:longsonr]]

Comment on attachment 655526 [details] [diff] [review]
Part 34: Implement SVG DOM text methods in terms of the new SVG text frame.

> 
>   nsSVGTextContainerFrame* GetTextContainerFrame() {
>     return do_QueryFrame(GetPrimaryFrame(Flush_Layout));
>   }
>+
>+  nsSVGTextFrame2* GetSVGTextFrame() {
>+    nsIFrame* frame = GetPrimaryFrame(Flush_Layout);
>+    while (frame) {
>+      nsSVGTextFrame2* textFrame = do_QueryFrame(frame);
>+      if (textFrame) {
>+        return textFrame;
>+      }
>+      frame = frame->GetParent();
>+    }
>+    return nsnull;

nsnull is now nullptr. There's a script in bug 626472.

>   nsSVGTextContainerFrame* GetTextContainerFrame() {
>     return do_QueryFrame(GetPrimaryFrame(Flush_Layout));
>   }
> 
>+  nsSVGTextFrame2* GetSVGTextFrame() {
>+    nsIFrame* frame = GetPrimaryFrame(Flush_Layout);
>+    while (frame) {
>+      nsSVGTextFrame2* textFrame = do_QueryFrame(frame);
>+      if (textFrame) {
>+        return textFrame;
>+      }
>+      frame = frame->GetParent();
>+    }
>+    return nsnull;
>+  }

This seems inefficient, caused by too much cut and paste probably having seen the SVGTextContainer
implementation above. If you have an SVGTextElement then its frame either doesn't exist or if it does
and we're in your brave new world then it should be an nsSVGTextFrame2.
Trawling through its parents if you don't find it directly won't help (unlike the nsSVGTextContainerFrame
case above where you're loking for the text frame parent of a tspan frame).

Same nsnull comment as above applies.

Is there some mechanism to recreate all text frames if we switch the pref between nsSVGTextFrame
and nsSVGTextFrame2? I'm not expecting one in this patch of course.

Comment 177

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

Comment on attachment 655517 [details] [diff] [review]
Part 12a: Don't paint borders on SVG text frames when using display lists.

Review of attachment 655517 [details] [diff] [review]:
-----------------------------------------------------------------

Why do we need to do this? I would have thought display list construction could not normally reach SVG text frames.

Comment 178

[Cameron McCormack (:heycam)]

Comment on attachment 655517 [details] [diff] [review]
Part 12a: Don't paint borders on SVG text frames when using display lists.

I think you're right this isn't necessary.  (I was getting into here for some reason but that must have been a bug I've since fixed.)

Comment 179

[Cameron McCormack (:heycam)]

(In reply to Robert Longson from comment #176)
> >   nsSVGTextContainerFrame* GetTextContainerFrame() {
> >     return do_QueryFrame(GetPrimaryFrame(Flush_Layout));
> >   }
> > 
> >+  nsSVGTextFrame2* GetSVGTextFrame() {
> >+    nsIFrame* frame = GetPrimaryFrame(Flush_Layout);
> >+    while (frame) {
> >+      nsSVGTextFrame2* textFrame = do_QueryFrame(frame);
> >+      if (textFrame) {
> >+        return textFrame;
> >+      }
> >+      frame = frame->GetParent();
> >+    }
> >+    return nsnull;
> >+  }
> 
> This seems inefficient, caused by too much cut and paste probably having
> seen the SVGTextContainer
> implementation above. If you have an SVGTextElement then its frame either
> doesn't exist or if it does
> and we're in your brave new world then it should be an nsSVGTextFrame2.
> Trawling through its parents if you don't find it directly won't help
> (unlike the nsSVGTextContainerFrame
> case above where you're loking for the text frame parent of a tspan frame).

Right, of course, we don't need the loop there.

> Same nsnull comment as above applies.
> 
> Is there some mechanism to recreate all text frames if we switch the pref
> between nsSVGTextFrame
> and nsSVGTextFrame2? I'm not expecting one in this patch of course.

I don't have one, but that's a good point.  Changing the pref while an SVG document with text is loaded would probably be a bad idea (i.e. it would probably crash due to assumptions about the types/presence of frames).  What would be the right way to handle this?  Should I just instead make all of the entry points that switch their behaviour based on the pref resilient against not finding the right kind of frame?  We could make these SVG DOM methods just check the type of frame rather than the pref.

Comment 180

[Cameron McCormack (:heycam)]

Although that might be tricky.  If you had for example:

  <text><tspan/></text>

with frames constructed when the pref was on, and then you flip the pref off and touch something on the tspan, you'd need to be careful that you rebuild frames of the right type.

Comment 181

[Robert Longson [:longsonr]]

Rebuild all frames when the pref switches would be best. Something equivalent to switching to display:none and back on the root frame.

Comment 182

[Cameron McCormack (:heycam)]

I would need to do this on every open window (pres shell?), yes?  Not really sure how to do that.

Comment 183

[Robert Longson [:longsonr]]

ask bz? or roc? 

or store the pref as a property on the outer SVG frame when it's created and use that over the pref.

Comment 184

[Robert Longson [:longsonr]]

Or if not a property, a state bit that children inherit like NONDISPLAY_CHILD.

Comment 185

[Cameron McCormack (:heycam)]

Attachment: Part 42a: Handle SVG text frame pref changes gracefully.

r?bz for the frame constructor changes, r?longsonr for the SVG parts.

This replaces most NS_SVGTextCSSFramesEnabled() calls with a check of the relevant frame's state bits to see if it's NS_FRAME_IS_SVG_TEXT.  Thus if you construct <text><tspan>...</tspan></text> with the pref on, then you will get new-style frames, and if you flip the pref and then say modify the text content of the <tspan>, you will still get new-style frames.  The only time NS_SVGTextCSSFramesEnabled() is called to find the current value of the pref is in nsCSSFrameConstructor::FindSVGData for a top-level <text> element.

Robert: the changes in nsSVGUtils.cpp and nsSVGOuterSVGFrame.cpp are on top of two patches I haven't yet uploaded (they're parts 35 and 36 in http://mcc.id.au/temp/bug-655877/ if you care to look).

Comment 186

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 656320 [details] [diff] [review]
Part 42a: Handle SVG text frame pref changes gracefully.

r=me on the frame constructor bits.

Comment 187

[Robert Longson [:longsonr]]

Comment on attachment 656320 [details] [diff] [review]
Part 42a: Handle SVG text frame pref changes gracefully.

Which patch defines NS_FRAME_IS_SVG_TEXT and does the inheriting into frames? I couldn't find it in 35 or 36 in the patch queue.

Comment 188

[Cameron McCormack (:heycam)]

(In reply to Robert Longson from comment #187)
> Which patch defines NS_FRAME_IS_SVG_TEXT and does the inheriting into
> frames? I couldn't find it in 35 or 36 in the patch queue.

Part 3 (already landed) defines the state bit and propagates it to children, and the nsSVGTextFrame2 constructor in part 31b sets it initially.

Comment 189

[Robert Longson [:longsonr]]

(In reply to Cameron McCormack (:heycam) from comment #188)
> 
> Part 3 (already landed) defines the state bit and propagates it to children,
> and the nsSVGTextFrame2 constructor in part 31b sets it initially.

Ahh. Very good.

Comment 190

[Cameron McCormack (:heycam)]

Attachment: Part 31b: New frame class nsSVGTextFrame2 for SVG text using CSS block/inline/text frames for layout.

Here's the big one.  I haven't got it working with DLBI yet, but I suspect that'll be a small patch (and would be better as a separate patch anyway).

Comment 191

[Cameron McCormack (:heycam)]

Attachment: Part 35: Ensure SVG text is updated when attributes on text content children change.

r?roc for the nsInlineFrame parts, r?jwatt for the SVG parts.

roc: Let me know if you think it makes sense to use a mutation events listener (now that we have one registered for element insertion/removal) for the attribute changes here, instead of modfiying nsInlineFrame itself.

Comment 192

[Cameron McCormack (:heycam)]

Attachment: Part 36: Make referenced text path updates cause SVG text relayout.

Note that the "if (NS_SVGTextFramesEnabled())" check gets changed to "mFrame->IsSVGText()" by the later patch part 42a.

Comment 193

[Cameron McCormack (:heycam)]

Attachment: Part 41a: Move ToCanvasBounds to nsSVGUtils.

I want to call this from both nsSVGForeignObjectFrame and nsSVGTextFrame2.

Comment 194

[Robert Longson [:longsonr]]

Comment on attachment 657568 [details] [diff] [review]
Part 41a: Move ToCanvasBounds to nsSVGUtils.

Seems OK. Doesn't save much over just calling nsLayoutUtils directly though.

Comment 195

[Robert Longson [:longsonr]]

Comment on attachment 657565 [details] [diff] [review]
Part 31b: New frame class nsSVGTextFrame2 for SVG text using CSS block/inline/text frames for layout.

>+/**
>+ * Returns the closest ancestor-or-self node that is not an SVG <a>
>+ * element.
>+ */
>+static nsIContent*
>+GetFirstNonAAncestor(nsIContent* aContent)
>+{
>+  while (aContent &&
>+         aContent->IsSVG() && 
>+         aContent->Tag() == nsGkAtoms::a) {
>+    aContent = aContent->GetParent();

Should this be aContent->GetFlattenedTreeParent() instead? And in fact throughout
where you use GetParent()

>+  if (aContent->Tag() == nsGkAtoms::textPath) {
>+    nsIContent* parent = GetFirstNonAAncestor(aContent->GetParent());
>+    return parent &&
>+           parent->IsSVG() &&
>+           parent->Tag() == nsGkAtoms::text;

better as parent->IsSVG(nsGkAtoms::text)

>+  }
>+
>+  return aContent->Tag() == nsGkAtoms::tspan ||
>+         aContent->Tag() == nsGkAtoms::textPath ||
>+         aContent->Tag() == nsGkAtoms::altGlyph ||
>+         aContent->Tag() == nsGkAtoms::a;

Could see if aContent implements nsIDOMSVGTextContentElement instead although that
wouldn't catch an <a>, it wouldn't need rewriting for tref.

>+
>+  // Measure that range.
>+  gfxTextRun::Metrics metrics = textRun->MeasureText(offset, length, gfxFont::LOOSE_INK_EXTENTS, nullptr, nullptr);

Nit: long line, fold somewhere.

>+  gfxPoint mPosition;
>+  double mAngle;
>+
>+  // not displayed due to falling off the end of a <textPath>
>+  bool mHidden: 1;
>+
>+  // skipped in positioning attributes due to being collapsed-away white space
>+  bool mUnaddressable: 1;
>+
>+  // a preceding character is what positioning attributes address
>+  bool mClusterOrLigatureGroupMiddle: 1;
>+
>+  // rendering is split here since an explicit position or rotation was given
>+  bool mRunBoundary: 1;
>+
>+  // an anchored chunk begins here
>+  bool mStartOfChunk: 1;
>+

Not sure using bits gets us anywhere as this will round up to 32 bits in size.

I've just skimmed this really so far.

Comment 196

[Cameron McCormack (:heycam)]

Attachment: Part 41b: Make SVG text selectable with the mouse.

r?roc for the layout/base/ and layout/generic/ changes, r?jwatt for the layout/svg/ changes (since he's reviewing the nsSVGTextFrame2 class patch).

We make the nsFrame methods that do text selection call in to the SVG frames to transform a point to pretend that it is over the corresponding part of the nsTextFrame when it is over one of the rendered runs of the text.  For example with:

  <svg>
    <text x="100 200" y="100">ABC</text>
  </svg>

the mRect of the <text> is going to be something like:

  x: 6000
  y: 6000 - ascentInAppUnits("ABC")
  width: 6000 + advanceInAppUnits("BC")
  height: ascentInAppUnits("ABC") + descentInAppUnits("ABC")

But when the mouse is at say (201,100) user units, we need to pretend that the point in app units relative to the <svg> is

  x: 6000 + advanceInAppUnits("A") + 60
  y: 6000

and not

  x: 12000 + 60
  y: 6000

so that when selection happens the nsTextFrame thinks the point is over the "B".

Comment 197

[Cameron McCormack (:heycam)]

(In reply to Robert Longson from comment #195)
> Should this be aContent->GetFlattenedTreeParent() instead? And in fact
> throughout
> where you use GetParent()

Handling cases where a child text element had a binding applied seemed tricky, and roc thought it was OK not to handle that for now.

> I've just skimmed this really so far.

Feel free to give more detailed review if you want.

Comment 198

[Cameron McCormack (:heycam)]

(In reply to Robert Longson from comment #195)
> >+  }
> >+
> >+  return aContent->Tag() == nsGkAtoms::tspan ||
> >+         aContent->Tag() == nsGkAtoms::textPath ||
> >+         aContent->Tag() == nsGkAtoms::altGlyph ||
> >+         aContent->Tag() == nsGkAtoms::a;
> 
> Could see if aContent implements nsIDOMSVGTextContentElement instead
> although that
> wouldn't catch an <a>, it wouldn't need rewriting for tref.

Yeah, that works.  (I noticed the textPath check above is redundant.)

> Not sure using bits gets us anywhere as this will round up to 32 bits in
> size.

Right, I must've miscalculated.

Comment 199

[Robert Longson [:longsonr]]

Comment on attachment 657565 [details] [diff] [review]
Part 31b: New frame class nsSVGTextFrame2 for SVG text using CSS block/inline/text frames for layout.

>+static nsIContent*
>+GetFirstNonAAncestor(nsIContent* aContent)
>+{
>+  while (aContent &&
>+         aContent->IsSVG() && 
>+         aContent->Tag() == nsGkAtoms::a) {

aContent->IsSVG(nsGkAtoms::a)

>+IsTextContentElement(nsIContent* aContent)

Isn't the logic in this method basically in nsCSSFrameConstructor too. Can't it be put somewhere common and shared. If this idea is too complicated then that's OK.

>+  /**
>+   * Returns a rectangle taht covers the fill and/or stroke of the rendered run
>+   * in the <text> element's user space.

s/taht/that/

>+  /**
>+   * The point in user space that the text is positioned at.
>+   *
>+   * The x coordinate is the left edge of a LTR run of text or the right edge of
>+   * an RTL run.  The y coordinate is the baseline of the text.
>+   */
>+  gfxPoint mPt;

Nit: would rather see this called mPosition 

>+
>+  /**
>+   * Whether the iterator is currently within mSubtree.
>+   */
>+  bool mWithinSubtree;
>+
>+  /**
>+   * Whether the iterator has already iterated through mSubtree.
>+   */
>+  bool mPastSubtree;

Could these two be an enum? eWithinSubtree, ePastSubtree, eBeforeSubtree?

>+          // Unaccumulate the current frame's position.

Unaccumulate isn't a word.

>+void
>+TextFrameIterator::PushBaseline(nsIFrame* aNextFrame)
>+{
>+  uint8_t baseline = aNextFrame->GetStyleSVGReset()->mDominantBaseline;
>+  if (baseline != NS_STYLE_DOMINANT_BASELINE_AUTO) {
>+    mBaselines.AppendElement(baseline);
>+  } else {
>+    mBaselines.AppendElement(mBaselines[mBaselines.Length() - 1]);
>+  }

What happens if mBaselines.Length() == 0 and we're auto? Can that happen? If not
assert, otherwise do something sensible.

if (baseline == NS_STYLE_DOMINANT_BASELINE_AUTO) {
  assert that mBaselines.Length() > 0
  baseline = mBaselines[mBaselines.Length() - 1];
}
mBaselines.AppendElement(baseline);

>+TextFrameIterator::PopBaseline()

  assert that mBaselines.Length() > 0

>+  mBaselines.TruncateLength(mBaselines.Length() - 1);

>+ * An instance of this calss is passed to nsTextFrame::PaintText if painting

s/calss/class/

>+
>+  // XXX This is copied from nsSVGGlyphFrame::Render, but cairo doesn't actually
>+  // seem to do anything with the antialias mode.  So we can perhaps remove it.
>+  switch (mFrame->GetStyleSVG()->mTextRendering) {
>+  case NS_STYLE_TEXT_RENDERING_OPTIMIZESPEED:
>+    gfx->SetAntialiasMode(gfxContext::MODE_ALIASED);
>+    break;
>+  default:
>+    gfx->SetAntialiasMode(gfxContext::MODE_COVERAGE);
>+    break;

"Or make SetAntialiasMode set cairo text antialiasing too." could be added to the comment

>+void
>+SVGTextDrawPathCallbacks::FillAndStroke()
>+{
>+  bool pushedGroup = false;

I wonder if we should create a gfxContextAutoPushPopGroup in thebes\gfzContext.h?

Comment 200

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

Comment on attachment 657630 [details] [diff] [review]
Part 41b: Make SVG text selectable with the mouse.

Review of attachment 657630 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsIFrame.h
@@ +2884,5 @@
> +   * Transforms a point from the coordinate space of the ancestor of aFrame
> +   * that manages aFrame for pointer events into the coordinate space for
> +   * aFrame.  When aFrame is NS_FRAME_IS_SVG_TEXT, the ancestor
> +   * nsSVGOuterSVGFrame will transform the point so that it takes into account
> +   * any SVG text layout.  Otherwise, nothing is done.

I would prefer not to have this implicit "ancestor of aFrame that manages aFrame for pointer events".

Instead of having this and nsLayoutUtils::TransformFramePoint, could we make nsLayoutUtils::GetTransformToAncestor aware of the SVG text frame setup and use that?

::: layout/svg/base/src/nsSVGTextFrame2.cpp
@@ +3008,5 @@
> +    }
> +
> +    gfxMatrix m = GetCanvasTM(FOR_HIT_TESTING).Invert();
> +    m.Multiply(run.GetTransformFromRunUserSpaceToUserSpace(presContext).
> +                   Invert());

Seems like you can save an Invert here by premultiplying and then inverting the result?

@@ +4469,5 @@
> +{
> +  NS_ASSERTION(GetFirstPrincipalChild(), "must have a child frame");
> +
> +  nsTextFrame* textFrame = do_QueryFrame(aFrame);
> +  if (!textFrame) {

The documentation says aFrame should be a text frame ... should we make the caller do this check and pass nsTextFrame*?

@@ +4497,5 @@
> +
> +  // Find the closest rendered run for the frame.
> +  nscoord dx = nscoord_MAX;
> +  nscoord dy = nscoord_MAX;
> +  while (run.mFrame == textFrame && run.mFrame) {

How can the first condition be true and the second false?

::: layout/svg/base/src/nsSVGTextFrame2.h
@@ +221,5 @@
> +   */
> +  virtual void GetCloserFrameForSelection(nsPoint aPoint,
> +                                          nsIFrame*& aFrame,
> +                                          nscoord& aXDistance,
> +                                          nscoord& aYDistance);

Use nsPoint instead of two nscoords. And return either the nsIFrame* or nsPoint directly.

@@ +223,5 @@
> +                                          nsIFrame*& aFrame,
> +                                          nscoord& aXDistance,
> +                                          nscoord& aYDistance);
> +
> +

Only need 1 blank line here

@@ +279,5 @@
>    double GetFontSizeScaleFactor() const;
>  
> +  /**
> +   * Transforms a point from outer SVG frame coordinate space to
> +   * the coordinate space for descendent text frame aFrame.

Document the return value too

Comment 201

[Cameron McCormack (:heycam)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #200)
> ::: layout/generic/nsIFrame.h
> @@ +2884,5 @@
> > +   * Transforms a point from the coordinate space of the ancestor of aFrame
> > +   * that manages aFrame for pointer events into the coordinate space for
> > +   * aFrame.  When aFrame is NS_FRAME_IS_SVG_TEXT, the ancestor
> > +   * nsSVGOuterSVGFrame will transform the point so that it takes into account
> > +   * any SVG text layout.  Otherwise, nothing is done.
> 
> I would prefer not to have this implicit "ancestor of aFrame that manages
> aFrame for pointer events".
> 
> Instead of having this and nsLayoutUtils::TransformFramePoint, could we make
> nsLayoutUtils::GetTransformToAncestor aware of the SVG text frame setup and
> use that?

There is no single transform that GetTransformToAncestor could return, because it depends on the actual point that will be transformed.  We could make GetTransformToAncestor take a point to influence what transform is returned, but then I think this is similar to what I have currently.

> @@ +4497,5 @@
> > +
> > +  // Find the closest rendered run for the frame.
> > +  nscoord dx = nscoord_MAX;
> > +  nscoord dy = nscoord_MAX;
> > +  while (run.mFrame == textFrame && run.mFrame) {
> 
> How can the first condition be true and the second false?

It can't; I'll remove the second.

> ::: layout/svg/base/src/nsSVGTextFrame2.h
> @@ +221,5 @@
> > +   */
> > +  virtual void GetCloserFrameForSelection(nsPoint aPoint,
> > +                                          nsIFrame*& aFrame,
> > +                                          nscoord& aXDistance,
> > +                                          nscoord& aYDistance);
> 
> Use nsPoint instead of two nscoords. And return either the nsIFrame* or
> nsPoint directly.

The two nscoords aren't really a point; they're separate distance measurements in the two dimensions.  Do you think it would still make sense to bundle them into an nsPoint?

I can make aFrame the return value.  It means the caller would have to make an additional check (that null wasn't returned, instead of just allowing a local variable to get the updated frame), though.


Yes to all other comments.

Comment 202

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

Comment on attachment 657566 [details] [diff] [review]
Part 35: Ensure SVG text is updated when attributes on text content children change.

Review of attachment 657566 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, I think it would be slightly nicer to use the mutation listener here than to modify nsInlineFrame.

Comment 203

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

(In reply to Cameron McCormack (:heycam) from comment #201)
> There is no single transform that GetTransformToAncestor could return,
> because it depends on the actual point that will be transformed.  We could
> make GetTransformToAncestor take a point to influence what transform is
> returned, but then I think this is similar to what I have currently.

True...

> > Use nsPoint instead of two nscoords. And return either the nsIFrame* or
> > nsPoint directly.
> 
> The two nscoords aren't really a point; they're separate distance
> measurements in the two dimensions.  Do you think it would still make sense
> to bundle them into an nsPoint?

No, I guess not.

> I can make aFrame the return value.  It means the caller would have to make
> an additional check (that null wasn't returned, instead of just allowing a
> local variable to get the updated frame), though.

I see. This method is a bit weird and probably shouldn't be called "Get". Maybe even define a struct with frame/x-distance/y-distance and just take a pointer to that? Say
  virtual void FindCloserFrameForSelection(nsPoint aPoint, FrameWithDistance* aCurrentBestFrame);

Comment 204

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

Comment on attachment 657630 [details] [diff] [review]
Part 41b: Make SVG text selectable with the mouse.

Review of attachment 657630 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +1146,4 @@
>    }
>  
> +  if (intermediate != aFrame) {
> +    intermediate->TransformFramePoint(aFrame, pt);

Since GetFrameToHandlePointTransformation already knows about nsSVGTextFrame2, wouldn't it be simpler to make TransforMFramePoint a method of nsSVGTextFrame2 only, and fold this transformation call into (suitably renamed) GetFrameToHandlePointTransformation?

@@ +1176,5 @@
>  }
>  
> +nsPoint
> +nsLayoutUtils::TransformFramePoint(nsPoint aPoint, nsIFrame* aFrom,
> +                                   nsIFrame* aTo)

I would really like the existing nsLayoutUtils Transform methods (TransformAncestorRectToFrame, TransformFrameRectToAncestor, and TransformRootPointToFrame) to work correctly with SVG text frames, in so far as that's possible, instead of introducing a new API here.

Comment 205

[Cameron McCormack (:heycam)]

We could handle all of the SVG text frame point transformations within the static TransformGfx{PointFrom,Rect{From,To}}Ancestor functions, by checking aFrame->IsSVGText() and then calling in to the renamed GetFrameToHandlePointTransformation, which would call directly into nsSVGTextFrame2.  And then make GetEventCoordinatesRelativeTo always call into TransformRootPointToFrame for SVG text frames.

That's fine for TransformRootPointToFrame.  For TransformAncestorRectToFrame/TransformFrameRectToAncestor: what would we do if the rectangle overlaps multiple runs for the nsTextFrame?

Comment 206

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

(In reply to Cameron McCormack (:heycam) from comment #205)
> That's fine for TransformRootPointToFrame.  For
> TransformAncestorRectToFrame/TransformFrameRectToAncestor: what would we do
> if the rectangle overlaps multiple runs for the nsTextFrame?

Pick the run containing the top-left.

Comment 207

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

Better still, transform each intersection of the rectangle with each run, and take the union of the result. The main place where this matters is getClientRects/getBoundingClientRect.

Comment 208

[Cameron McCormack (:heycam)]

Attachment: Part 31b: New frame class nsSVGTextFrame2 for SVG text using CSS block/inline/text frames for layout. (v1.1)

Updated to:
* changed TextRenderedRun::GetTransformFromRunUserSpaceToFrameAppUnits to
  GetTransformFromRunUserSpaceToFrameUserSpace
* rename TextRenderedRun::GetFrameUserSpaceRect to GetRunUserSpaceRect
* add a GetFrameUserSpaceRect that actually does that
* add the ability to TextFrameIterator and TextRenderedRunIterator to limit
  their iterations to a frame subtree (rather than a content subtree)

Those changes are needed due to fixes in later patches.  I also addressed a couple of Robert Longson's comments.

Comment 209

[Cameron McCormack (:heycam)]

Attachment: Part 41b: Make SVG text selectable with the mouse. (v1.1)

Address review comments.  I made the Transform* functions on nsSVGTextFrame2 work with frame types other than nsTextFrame, in case they ever get called with an nsInlineFrame for a <tspan>, for example.

Comment 210

[Cameron McCormack (:heycam)]

(In reply to Robert Longson from comment #199)
> >+IsTextContentElement(nsIContent* aContent)
> 
> Isn't the logic in this method basically in nsCSSFrameConstructor too. Can't
> it be put somewhere common and shared. If this idea is too complicated then
> that's OK.

Might be a bit tricky, I think I'll leave it.

> >+void
> >+TextFrameIterator::PushBaseline(nsIFrame* aNextFrame)
> >+{
> >+  uint8_t baseline = aNextFrame->GetStyleSVGReset()->mDominantBaseline;
> >+  if (baseline != NS_STYLE_DOMINANT_BASELINE_AUTO) {
> >+    mBaselines.AppendElement(baseline);
> >+  } else {
> >+    mBaselines.AppendElement(mBaselines[mBaselines.Length() - 1]);
> >+  }
> 
> What happens if mBaselines.Length() == 0 and we're auto? Can that happen? If
> not
> assert, otherwise do something sensible.

It can't happen because Init() pushes a value into the array at the beginning of iteration.  It pushes whatever the dominant-baseline value for mRootFrame (the nsSVGTextFrame2) is.  That might be auto too, but GetBaselinePosition will eventually just treat that as alphabetic.

> if (baseline == NS_STYLE_DOMINANT_BASELINE_AUTO) {
>   assert that mBaselines.Length() > 0
>   baseline = mBaselines[mBaselines.Length() - 1];
> }
> mBaselines.AppendElement(baseline);

I've left the code as is.  (We'll get an assertion from nsTArray.)
 
> >+TextFrameIterator::PopBaseline()
> 
>   assert that mBaselines.Length() > 0

I think we'll get an assertion from nsTArray there too, because it takes an unsigned integer and TruncateLength already does

  NS_ABORT_IF_FALSE(newLen <= oldLen, ...)

But that's on the boundary of being obvious, so I added your suggested assertion to PopBaseline.

> >+void
> >+SVGTextDrawPathCallbacks::FillAndStroke()
> >+{
> >+  bool pushedGroup = false;
> 
> I wonder if we should create a gfxContextAutoPushPopGroup in
> thebes\gfzContext.h?

That might be helpful.  Like gfxContextPathAutoSaveRestore, it would need to take a flag to say whether a push was actually needed.

Comment 211

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

Comment on attachment 658704 [details] [diff] [review]
Part 41b: Make SVG text selectable with the mouse. (v1.1)

Review of attachment 658704 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.h
@@ +537,5 @@
> +    return TransformAncestorPointToFrame(aFrame, aPoint, nullptr);
> +  }
> +
> +  /**
> +   * Transform aPoint relative to relative to aAncestor down to the

delete one 'relative to'

::: layout/generic/nsFrame.cpp
@@ +3585,5 @@
> +    if (closest.frame->IsSVGText())
> +      pt = nsLayoutUtils::TransformAncestorPointToFrame(closest.frame,
> +                                                        aPoint, this);
> +    else
> +      pt = aPoint - closest.frame->GetOffsetTo(this);

{} around if/else bodies

::: layout/svg/base/src/nsSVGTextFrame2.cpp
@@ +4566,5 @@
> +  nsPresContext* presContext = PresContext();
> +
> +  // Add in the mRect offset to aPoint, as that won't have been taken into
> +  // account when transforming the point from the ancestor frame down
> +  // to this one.

Don't you mean "as that *will* have been taken into account"?

@@ +4640,5 @@
> +  nsPresContext* presContext = PresContext();
> +
> +  // Add in the mRect offset to aRect, as that won't have been taken into
> +  // account when transforming the rect from the ancestor frame down
> +  // to this one.

ditto

@@ +4740,5 @@
> +    }
> +  }
> +
> +  // Subtract the mRect offset from the result, as our caller,
> +  // nsLayoutUtils::TransformFrameRectToAncestor, will add it back.

More accurately, the user space for this frame is relative to the top-left of mRect.

Comment 212

[Cameron McCormack (:heycam)]

Attachment: Part 45: Note a crashtest that involves XBL as triggering assertions.

This crashtest uses odd combinations of XBL and SVG text.  The new code asserts, but should be safe.  Let's just annotate the assertions for now.  (I've marked it 0-5 assertions so that we don't get test failures when the "use CSS frames for SVG text" pref is not switched on.)