Bug 655877 (svgtext)

SVG text should use CSS text frames to gain support for various text layout features

RESOLVED FIXED in mozilla21

Status

()

RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

(Blocks: 3 bugs)

Trunk
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(59 attachments, 39 obsolete attachments)

17.75 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
11.99 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
13.89 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
44.39 KB, patch
roc
: review+
heycam
: review+
Details | Diff | Splinter Review
9.77 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
3.77 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
7.83 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
4.67 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
1.83 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
2.90 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
1.31 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
1.22 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
33.84 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
42.17 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
1.00 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
7.77 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
1.83 KB, patch
bzbarsky
: review+
heycam
: checkin+
Details | Diff | Splinter Review
1.76 KB, patch
bzbarsky
: review+
heycam
: checkin+
Details | Diff | Splinter Review
3.94 KB, patch
dbaron
: review+
heycam
: checkin+
Details | Diff | Splinter Review
2.40 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
2.67 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
2.89 KB, patch
dbaron
: review+
heycam
: checkin+
Details | Diff | Splinter Review
580 bytes, patch
jwatt
: review+
heycam
: checkin+
Details | Diff | Splinter Review
6.15 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
41.82 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
9.16 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
1.18 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
5.80 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
26.85 KB, image/svg+xml
Details
4.75 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
1.08 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
40.64 KB, patch
jwatt
: review+
heycam
: checkin+
Details | Diff | Splinter Review
4.45 KB, patch
jwatt
: review+
heycam
: checkin+
Details | Diff | Splinter Review
2.07 KB, patch
jwatt
: review+
heycam
: checkin+
Details | Diff | Splinter Review
1.32 KB, patch
longsonr
: review+
heycam
: checkin+
Details | Diff | Splinter Review
17.89 KB, patch
dbaron
: review+
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
1.15 KB, patch
Details | Diff | Splinter Review
1.60 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
1.21 KB, patch
longsonr
: review+
heycam
: checkin+
Details | Diff | Splinter Review
3.52 KB, patch
jwatt
: review+
heycam
: checkin+
Details | Diff | Splinter Review
16.42 KB, patch
longsonr
: review+
heycam
: checkin+
Details | Diff | Splinter Review
19.27 KB, patch
bzbarsky
: review+
longsonr
: review+
heycam
: checkin+
Details | Diff | Splinter Review
4.00 KB, patch
longsonr
: review+
heycam
: checkin+
Details | Diff | Splinter Review
847 bytes, patch
longsonr
: review+
heycam
: checkin+
Details | Diff | Splinter Review
1002 bytes, patch
longsonr
: review+
heycam
: checkin+
Details | Diff | Splinter Review
1.11 KB, patch
jwatt
: review+
heycam
: checkin+
Details | Diff | Splinter Review
30.34 KB, patch
smontagu
: review+
heycam
: checkin+
Details | Diff | Splinter Review
6.56 KB, patch
jwatt
: review+
heycam
: checkin+
Details | Diff | Splinter Review
36.56 KB, patch
jwatt
: review+
heycam
: checkin+
Details | Diff | Splinter Review
181.60 KB, patch
jwatt
: review+
heycam
: checkin+
Details | Diff | Splinter Review
2.73 KB, patch
jwatt
: review+
heycam
: checkin+
Details | Diff | Splinter Review
7.51 KB, patch
jwatt
: review+
heycam
: checkin+
Details | Diff | Splinter Review
1009 bytes, patch
jwatt
: review+
heycam
: checkin+
Details | Diff | Splinter Review
1.01 KB, patch
jwatt
: review+
heycam
: checkin+
Details | Diff | Splinter Review
83.89 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
13.61 KB, patch
jwatt
: review+
heycam
: checkin+
Details | Diff | Splinter Review
3.75 KB, patch
jwatt
: review+
heycam
: checkin+
Details | Diff | Splinter Review
2.41 KB, patch
jwatt
: review+
heycam
: checkin+
Details | Diff | Splinter Review
166.55 KB, patch
jwatt
: review+
heycam
: checkin+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
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.
(Assignee)

Updated

8 years ago
Blocks: 317196, 292498
(Assignee)

Updated

8 years ago
Assignee: nobody → cam
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
Blocks: 371787
(Assignee)

Updated

7 years ago
Blocks: 454970
(Assignee)

Comment 1

7 years ago
Posted patch WIP (v0.1) (obsolete) — Splinter Review
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
(Assignee)

Updated

7 years ago
Blocks: 716666
(Assignee)

Updated

7 years ago
Depends on: 734319
(Assignee)

Comment 2

7 years ago
Posted patch WIP (v0.2) (obsolete) — Splinter Review
Painting of solid fill, no stroke text with multiple x/y/dx/dy attributes now works, including with decorations.
Attachment #605633 - Attachment is patch: true
(Assignee)

Comment 3

7 years ago
Posted patch WIP (v0.3) (obsolete) — Splinter Review
(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.
Attachment #585360 - Attachment is obsolete: true
Attachment #605633 - Attachment is obsolete: true
(Assignee)

Comment 4

7 years ago
Posted patch WIP (v0.4) (obsolete) — Splinter Review
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.
Attachment #606124 - Attachment is obsolete: true

Comment 6

7 years ago
Yes, progress looks very promising... ;-)
(Assignee)

Comment 7

7 years ago
Posted patch WIP (v0.5) (obsolete) — Splinter Review
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.
Attachment #608975 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Depends on: 740271
(Assignee)

Comment 8

7 years ago
Posted file WIP (v0.6) (obsolete) —
* SVG DOM text methods (getSubStringLength etc.) support.
* Text on a path support.
* Some refactoring/cleanup.
Attachment #610396 - Attachment is obsolete: true
(Assignee)

Comment 9

7 years ago
Posted file WIP (v0.7) (obsolete) —
* 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.
Attachment #616477 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Blocks: 330045
(Assignee)

Comment 10

7 years ago
Posted patch WIP (v0.8) (obsolete) — Splinter Review
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).
Attachment #618941 - Attachment is obsolete: true
(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
(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 :-).
(Assignee)

Updated

7 years ago
Depends on: 771874
(Assignee)

Updated

7 years ago
Depends on: 771876
(Assignee)

Comment 13

7 years ago
Current split up patch queue is here: http://mcc.id.au/temp/bug-655877/
(Assignee)

Updated

7 years ago
Attachment #630085 - Attachment is obsolete: true
(Assignee)

Comment 14

7 years ago
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?)
Attachment #640890 - Flags: review?(roc)
(Assignee)

Comment 15

7 years ago
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.)
Attachment #640891 - Flags: review?(roc)
(Assignee)

Comment 16

7 years ago
(Part 26 uses this, sorry, not part 17.)
(Assignee)

Comment 17

7 years ago
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.)
Attachment #640892 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Attachment #640892 - Attachment description: Construct new SVG text frames if the pref is set. → Part 42: Construct new SVG text frames if the pref is set.
(Assignee)

Comment 18

7 years ago
(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?
Attachment #640895 - Flags: feedback?(jwatt)
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 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 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.
Attachment #640892 - Flags: review?(roc) → review?(bzbarsky)
Attachment #640895 - Flags: feedback?(jwatt) → feedback+
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.
Attachment #640892 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 23

7 years ago
(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.
Attachment #640891 - Attachment is obsolete: true
Attachment #640891 - Flags: review?(roc)
Attachment #642687 - Flags: review?(roc)
(Assignee)

Comment 24

7 years ago
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.
Attachment #643428 - Flags: review?(bzbarsky)
(Assignee)

Comment 25

7 years ago
With the tests included this time.
Attachment #643428 - Attachment is obsolete: true
Attachment #643428 - Flags: review?(bzbarsky)
Attachment #643437 - Flags: review?(bzbarsky)
Comment on attachment 643437 [details] [diff] [review]
Part 42a: Construct new SVG text frames if the pref is set. (v1.1)

r=me
Attachment #643437 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 27

7 years ago
I'll keep this in sync with the patches attached to the bug.
(Assignee)

Comment 28

7 years ago
(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.
Attachment #640890 - Attachment is obsolete: true
Attachment #640890 - Flags: review?(roc)
Attachment #644269 - Flags: review?(roc)
(Assignee)

Comment 29

7 years ago
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.
Attachment #644280 - Flags: review?(roc)
(Assignee)

Comment 30

7 years ago
Same for word-spacing.
Attachment #644281 - Flags: review?(roc)
(Assignee)

Comment 31

7 years ago
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.
Attachment #644283 - Flags: review?(roc)
(Assignee)

Comment 32

7 years ago
We don't want margin, padding and borders to apply to SVG text.
Attachment #644284 - Flags: review?(roc)
(Assignee)

Comment 33

7 years ago
We also don't want floats.
Attachment #644285 - Flags: review?(roc)
(Assignee)

Comment 34

7 years ago
SVG text frames are not positioned.
Attachment #644286 - Flags: review?(roc)
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.
Attachment #644269 - Flags: review?(roc) → review+
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
Attachment #644280 - Flags: review?(roc) → review+
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.
Attachment #644283 - Flags: review?(roc) → review+
You should have a test for dynamic changes to dominant-baseline if there isn't already one.
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 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?
(Assignee)

Comment 41

7 years ago
(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.
Attachment #644475 - Flags: review?(roc)
(Assignee)

Comment 42

7 years ago
(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?
(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.
(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.
(Assignee)

Comment 45

7 years ago
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.
(Assignee)

Comment 46

7 years ago
I've removed most uses of IsFloatingStyle.
Attachment #644285 - Attachment is obsolete: true
Attachment #644285 - Flags: review?(roc)
Attachment #648189 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Attachment #648189 - Attachment is patch: true
(Assignee)

Comment 47

7 years ago
I've added nsStyleDisplay::IsPositioned() etc.
Attachment #644286 - Attachment is obsolete: true
Attachment #644286 - Flags: review?(roc)
Attachment #648190 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Attachment #648192 - Attachment description: Add a frame state bit and anonymous box pseudo for SVG text frames. → Part 3: Add a frame state bit and anonymous box pseudo for SVG text frames.
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 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
(Assignee)

Comment 55

7 years ago
(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 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?
(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 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.
(Assignee)

Updated

7 years ago
Attachment #648199 - Attachment description: Make :first-letter apply to <svg:text>. → Part 13: Make :first-letter apply to <svg:text>.
(Assignee)

Comment 59

7 years ago
Attachment #648189 - Attachment is obsolete: true
Attachment #648189 - Flags: review?(roc)
Attachment #648204 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Attachment #648204 - Attachment is patch: true
(Assignee)

Comment 60

7 years ago
Attachment #648190 - Attachment is obsolete: true
Attachment #648190 - Flags: review?(roc)
Attachment #648205 - Flags: review?(roc)
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.
Attachment #648204 - Flags: review?(roc) → review+
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.
Attachment #648205 - Flags: review?(roc) → review+
(Assignee)

Comment 63

7 years ago
Attachment #648195 - Attachment is obsolete: true
Attachment #648195 - Flags: review?(roc)
Attachment #648216 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Attachment #643437 - Attachment is patch: true
(Assignee)

Comment 64

7 years ago
Attachment #648197 - Attachment is obsolete: true
Attachment #648197 - Flags: review?(roc)
Attachment #648217 - Flags: review?(roc)
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.
(Assignee)

Updated

7 years ago
Attachment #640892 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #640892 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #644269 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #644280 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #644281 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #644283 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #644284 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #644475 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #648192 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #648193 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #648199 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #648204 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #648205 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #648216 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #648217 - Flags: checkin+
(Assignee)

Comment 70

7 years ago
This is to allow the frame's normal painting code path to look at 'fill' instead of 'color'.
Attachment #648622 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Attachment #648624 - Attachment description: Make nsTextFrame QueryFrame-able. → Part 20: Make nsTextFrame QueryFrame-able.
Comment on attachment 648621 [details] [diff] [review]
Part 18: Ensure even line-ending white space SVG text frames are created.

r=me
Attachment #648621 - Flags: review?(bzbarsky) → review+
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.
Attachment #648620 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 75

7 years ago
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.
(Assignee)

Comment 76

7 years ago
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?
Attachment #648631 - Flags: review?(jwatt)
Attachment #648622 - Flags: review?(roc) → review?(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
Attachment #648622 - Flags: review?(dbaron) → review+
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
Attachment #648632 - Flags: review?(dbaron) → review+
(Assignee)

Comment 80

7 years ago
(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 "*".
(Assignee)

Updated

7 years ago
Attachment #648620 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #648621 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #648622 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #648624 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #648632 - Flags: checkin+
(Assignee)

Comment 82

7 years ago
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.)
Attachment #649067 - Flags: review?(roc)
(Assignee)

Comment 83

7 years ago
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.)
Attachment #649068 - Flags: review?(roc)
(Assignee)

Comment 84

7 years ago
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.)
Attachment #649069 - Flags: review?(jwatt)
(Assignee)

Updated

7 years ago
Attachment #649067 - Attachment is obsolete: true
Attachment #649067 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Attachment #649069 - Attachment description: Add UA style sheet rule to inherit unicode-bidi on <svg:text> to its anonymous block child. → Part 24: Add UA style sheet rule to inherit unicode-bidi on <svg:text> to its anonymous block child.
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 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 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 :-)
(Assignee)

Comment 91

7 years ago
(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?
We could, but isn't it easy to fix?
(Assignee)

Comment 93

7 years ago
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. :)
Now that you've figured it out, you might as well do it :-)
(Assignee)

Comment 95

7 years ago
Attachment #649068 - Attachment is obsolete: true
Attachment #649068 - Flags: review?(roc)
Attachment #649520 - Flags: review?(roc)
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!
Attachment #649520 - Flags: review?(roc) → review+
(Assignee)

Comment 97

7 years ago
Oops, actually I saw those when I generated the patch and fixed them to use the function, but forgot to regenerate the patch...
(Assignee)

Comment 98

7 years ago
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().
Attachment #649070 - Attachment is obsolete: true
Attachment #649070 - Flags: review?(roc)
Attachment #649524 - Flags: review?(roc)
(Assignee)

Comment 99

7 years ago
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.
Attachment #649527 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Attachment #649072 - Flags: review?(roc)
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.
Attachment #649524 - Flags: review?(roc) → review+
(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 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 :-)
Attachment #649527 - Flags: review?(roc) → review+
(Assignee)

Updated

7 years ago
Attachment #649072 - Attachment is obsolete: true
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.)
(Assignee)

Comment 105

7 years ago
We need to pass in an nsCharClipDisplayItem to nsTextFrame::PaintText, so we need to be able to create one outside of nsDisplayListBuilder.
Attachment #649572 - Flags: review?(roc)
(Assignee)

Comment 107

7 years ago
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.
Attachment #649575 - Flags: review?(roc)
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 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
Attachment #649575 - Flags: review?(roc) → review+
Attachment #648625 - Flags: review?(jwatt) → review+
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?
Attachment #648631 - Flags: review?(jwatt)
Attachment #648631 - Flags: review?(bzbarsky)
Attachment #648631 - Flags: review+
Attachment #649069 - Flags: review?(jwatt) → review+
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.
Attachment #648631 - Flags: review?(bzbarsky)
Attachment #648631 - Flags: review?
Attachment #648631 - Flags: review+
(Assignee)

Comment 112

7 years ago
(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.
(Assignee)

Comment 113

7 years ago
(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.
(Assignee)

Comment 114

7 years ago
(The "break" in that loop also seems to be misplaced; it should be inside the `if (parentFrame && parentFrame->IsSVGText())` block.
(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.
(Assignee)

Comment 116

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

Comment 117

7 years ago
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?
(Assignee)

Comment 118

7 years ago
Attachment #649574 - Attachment is obsolete: true
Attachment #649574 - Flags: review?(roc)
Attachment #649603 - Flags: review?(roc)
(Assignee)

Comment 119

7 years ago
(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?
(Assignee)

Updated

7 years ago
Attachment #649603 - Attachment description: 37: Reflow SVG text even if undisplayed content is added/removed. (v1.1) → Part 37: Reflow SVG text even if undisplayed content is added/removed. (v1.1)
(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?
(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

7 years ago
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

7 years ago
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.
(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.
(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?
(Assignee)

Comment 126

7 years ago
(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.
(Assignee)

Comment 127

7 years ago
(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.
(Assignee)

Comment 128

7 years ago
(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.
(Assignee)

Comment 129

7 years ago
Now with only solid decorations.
Attachment #649558 - Attachment is obsolete: true
Attachment #649558 - Flags: review?(roc)
Attachment #649900 - Flags: review?(roc)
(Assignee)

Comment 130

7 years ago
Attachment #649603 - Attachment is obsolete: true
Attachment #649603 - Flags: review?(roc)
Attachment #649945 - Flags: review?(roc)
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?
(Assignee)

Comment 132

7 years ago
(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.
(Assignee)

Comment 133

7 years ago
Separating this out will let me land some other patches before the actual frame class lands.
Attachment #649948 - Flags: review?(roc)
> > 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?
(Assignee)

Comment 135

7 years ago
(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.
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.
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?
(Assignee)

Comment 138

7 years ago
(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?
(Assignee)

Comment 140

7 years ago
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?
Attachment #649945 - Attachment is obsolete: true
Attachment #649945 - Flags: review?(roc)
I think if it's simpler to just always call FrameNeedsReflow, we should do that.
(Assignee)

Updated

7 years ago
Attachment #649900 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #642687 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #649524 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #649520 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #649527 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #649948 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #649572 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #649575 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #649069 - Flags: checkin+

Comment 144

7 years ago
(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 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....
Attachment #648631 - Flags: review?
(Assignee)

Comment 147

7 years ago
(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).
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.)
(Assignee)

Comment 149

7 years ago
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.)
Attachment #650459 - Flags: review?(roc)
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.
Attachment #650459 - Flags: review?(roc) → review?(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.
Attachment #650459 - Flags: feedback?(fantasai.bugs)

Comment 152

7 years ago
(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?
(Assignee)

Comment 153

7 years ago
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.
Attachment #640895 - Attachment is obsolete: true
Attachment #650789 - Flags: review?(jwatt)
(Assignee)

Comment 155

7 years ago
This is needed so that nsSVGTextFrame2 get bounding boxes / covered regions of nsTextFrame children.
Attachment #650809 - Flags: review?(jwatt)
(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.
(Assignee)

Comment 157

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

Updated

7 years ago
Attachment #650809 - Attachment description: Bug 655877 - Part 30: Allow PathExtentsToMaxStrokeExtents to work with nsTextFrames. → Part 30: Allow PathExtentsToMaxStrokeExtents to work with nsTextFrames.
(Assignee)

Comment 158

7 years ago
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.
Attachment #650816 - Flags: review?(jwatt)
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... :-|
Attachment #650789 - Flags: review?(jwatt) → review+
Attachment #650809 - Flags: review?(jwatt) → review+
Attachment #650816 - Flags: review?(jwatt) → review+
(Assignee)

Comment 160

7 years ago
(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.
(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! :-)
(Assignee)

Updated

7 years ago
Attachment #650789 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #650809 - Flags: checkin+
(Assignee)

Updated

7 years ago
Attachment #650816 - Flags: checkin+
(Assignee)

Comment 163

7 years ago
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.
Attachment #651089 - Flags: review?(jwatt)
Attachment #651089 - Flags: review?(jwatt) → review+

Comment 165

7 years ago
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.
Attachment #650459 - Flags: feedback?(fantasai.bugs)
(Assignee)

Updated

7 years ago
Attachment #651089 - Flags: checkin+
(Assignee)

Comment 168

7 years ago
I've gone with the property value renaming that roc suggested.
Attachment #650459 - Attachment is obsolete: true
Attachment #650459 - Flags: review?(dbaron)
Attachment #654901 - Flags: review?(dbaron)

Comment 169

7 years ago
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.
(Assignee)

Comment 170

7 years ago
(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.
(Assignee)

Comment 171

7 years ago
Since switching to painting SVG with display lists, we need this check here too.
Attachment #655517 - Flags: review?(roc)
(Assignee)

Comment 172

7 years ago
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.
Attachment #655521 - Flags: review?(roc)
(Assignee)

Comment 174

7 years ago
nsSVGTextFrame2s will handle their positioning themselves, like SVG geometry frames.
Attachment #655525 - Flags: review?(jwatt)
(Assignee)

Comment 175

7 years ago
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.)
Attachment #655526 - Flags: review?(longsonr)
Attachment #655523 - Flags: review?(longsonr) → review+
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.
Attachment #655526 - Flags: review?(longsonr) → review+
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.
(Assignee)

Comment 178

7 years ago
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.)
Attachment #655517 - Flags: review?(roc)
(Assignee)

Comment 179

7 years ago
(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.
(Assignee)

Comment 180

7 years ago
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.
Rebuild all frames when the pref switches would be best. Something equivalent to switching to display:none and back on the root frame.
(Assignee)

Comment 182

7 years ago
I would need to do this on every open window (pres shell?), yes?  Not really sure how to do that.
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.
Or if not a property, a state bit that children inherit like NONDISPLAY_CHILD.

Updated

7 years ago
Depends on: 786216
No longer depends on: 786216
Attachment #655525 - Flags: review?(jwatt) → review+
(Assignee)

Comment 185

7 years ago
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).
Attachment #656320 - Flags: review?(longsonr)
Attachment #656320 - Flags: review?(bzbarsky)
Comment on attachment 656320 [details] [diff] [review]
Part 42a: Handle SVG text frame pref changes gracefully.

r=me on the frame constructor bits.
Attachment #656320 - Flags: review?(bzbarsky) → review+
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.
Attachment #656320 - Flags: review?(longsonr) → review+
(Assignee)

Comment 188

7 years ago
(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.
(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.
(Assignee)

Comment 190

7 years ago
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).
Attachment #657565 - Flags: review?(jwatt)
(Assignee)

Comment 191

7 years ago
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.
Attachment #657566 - Flags: review?(roc)
Attachment #657566 - Flags: review?(jwatt)
(Assignee)

Comment 192

7 years ago
Note that the "if (NS_SVGTextFramesEnabled())" check gets changed to "mFrame->IsSVGText()" by the later patch part 42a.
Attachment #657567 - Flags: review?(longsonr)
(Assignee)

Comment 193

7 years ago
I want to call this from both nsSVGForeignObjectFrame and nsSVGTextFrame2.
Attachment #657568 - Flags: review?(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.
Attachment #657568 - Flags: review?(longsonr) → review+
Attachment #657567 - Flags: review?(longsonr) → review+
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.
(Assignee)

Comment 196

7 years ago
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".
Attachment #657630 - Flags: review?(roc)
Attachment #657630 - Flags: review?(jwatt)
(Assignee)

Comment 197

7 years ago
(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.
(Assignee)

Comment 198

7 years ago
(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 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 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
(Assignee)

Comment 201

7 years ago
(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 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.
(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 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.
(Assignee)

Comment 205

7 years ago
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?
(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.
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.
(Assignee)

Comment 208

7 years ago
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.
Attachment #658703 - Flags: review?(jwatt)
(Assignee)

Updated

7 years ago
Attachment #658703 - Attachment is patch: true
(Assignee)

Updated

7 years ago
Attachment #657565 - Attachment is obsolete: true
Attachment #657565 - Flags: review?(jwatt)
(Assignee)

Comment 209

7 years ago