Bug 655877 (svgtext)

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

RESOLVED FIXED in mozilla21

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
2 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
Details | Diff | Splinter Review
11.99 KB, patch
roc
: review+
heycam
: checkin+
Details | Diff | Splinter Review
13.89 KB, patch
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
heycam
: checkin+
Details | Diff | Splinter Review
1.76 KB, patch
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
Robert Longson
: 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
Robert Longson
: review+
heycam
: checkin+
Details | Diff | Splinter Review
3.52 KB, patch
jwatt
: review+
heycam
: checkin+
Details | Diff | Splinter Review
16.42 KB, patch
Robert Longson
: review+
heycam
: checkin+
Details | Diff | Splinter Review
19.27 KB, patch
Robert Longson
: review+
heycam
: checkin+
Details | Diff | Splinter Review
4.00 KB, patch
Robert Longson
: review+
heycam
: checkin+
Details | Diff | Splinter Review
847 bytes, patch
Robert Longson
: review+
heycam
: checkin+
Details | Diff | Splinter Review
1002 bytes, patch
Robert Longson
: 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

6 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

6 years ago
Blocks: 317196, 292498
(Assignee)

Updated

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

Updated

6 years ago
Blocks: 371787
(Assignee)

Updated

6 years ago
Blocks: 454970
(Assignee)

Comment 1

6 years ago
Created attachment 585360 [details] [diff] [review]
WIP (v0.1)

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

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

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

Updated

5 years ago
Blocks: 716666
(Assignee)

Updated

5 years ago
Depends on: 734319
(Assignee)

Comment 2

5 years ago
Created attachment 605633 [details] [diff] [review]
WIP (v0.2)

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

Updated

5 years ago
Attachment #605633 - Attachment is patch: true
(Assignee)

Comment 3

5 years ago
Created attachment 606124 [details] [diff] [review]
WIP (v0.3)

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

Now we have non-solid fills and strokes working on text and their decorations -- all styles of decorations except wavy ones so far.
Attachment #585360 - Attachment is obsolete: true
Attachment #605633 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
Created attachment 608975 [details] [diff] [review]
WIP (v0.4)

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

Invalidations happen correctly now when selections changes, such as when press Cmd+A to select all.  Searching also works, so the green and pink search highlights get rendered.
Attachment #606124 - Attachment is obsolete: true
Awesome!

Comment 6

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

Comment 7

5 years ago
Created attachment 610396 [details] [diff] [review]
WIP (v0.5)

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

Selecting text with the mouse works now too.
Attachment #608975 - Attachment is obsolete: true
Blocks: 308338
(Assignee)

Updated

5 years ago
Depends on: 740271
(Assignee)

Comment 8

5 years ago
Created attachment 616477 [details]
WIP (v0.6)

* SVG DOM text methods (getSubStringLength etc.) support.
* Text on a path support.
* Some refactoring/cleanup.
Attachment #610396 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
Created attachment 618941 [details]
WIP (v0.7)

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

Updated

5 years ago
Blocks: 330045
(Assignee)

Comment 10

5 years ago
Created attachment 630085 [details] [diff] [review]
WIP (v0.8)

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

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

Comment 11

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

5 years ago
Depends on: 771874
(Assignee)

Updated

5 years ago
Depends on: 771876
(Assignee)

Comment 13

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

Updated

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

Comment 14

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

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

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

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

Comment 15

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

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

(Part 17 extends adds a similar callback parameter to nsTextFrame, with a few different callback functions (for different parts of the text frame being rendered), which uses this gfxFont callback.)
Attachment #640891 - Flags: review?(roc)
(Assignee)

Comment 16

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

Comment 17

5 years ago
Created attachment 640892 [details] [diff] [review]
Part 42: Construct new SVG text frames if the pref is set.

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

(Although just writing this out now I wonder whether that means aParentFrame being null always implies within SVG text, in which case we wouldn't need to do this.)
Attachment #640892 - Flags: review?(roc)
(Assignee)

Updated

5 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

5 years ago
Created attachment 640895 [details] [diff] [review]
Part 1: Move some path drawing functions from nsSVGGeometryFrame to nsSVGUtils.

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

Updated

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

5 years ago
Created attachment 642687 [details] [diff] [review]
Part 25: Give gfxTextRuns the ability to invoke a callback after painting glyphs and partial ligatures. (v1.1)

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

OK, I renamed it to DrawCallbacks and added the comment.
Attachment #640891 - Attachment is obsolete: true
Attachment #640891 - Flags: review?(roc)
Attachment #642687 - Flags: review?(roc)
Attachment #642687 - Flags: review?(roc) → review+
(Assignee)

Comment 24

5 years ago
Created attachment 643428 [details] [diff] [review]
Part 42a: Construct new SVG text frames if the pref is set.

These are the changes to handle <text><a><textPath>, which turned out a bit trickier because it's another case where you don't have access to parent frames within FindSVGData.
Attachment #643428 - Flags: review?(bzbarsky)
(Assignee)

Comment 25

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

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

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

Comment 28

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

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

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

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

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

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

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


I solved the problem of the mutually referential nsIFrame and nsStyleDisplay inlines by moving the definitions of nsStyleDisplay::IsBlockInside etc. to nsStyleStructInlines.h and #including this file towards the bottom of nsIFrame.h, before the definitions of its inline IsBlockInside etc. functions.
Attachment #640890 - Attachment is obsolete: true
Attachment #640890 - Flags: review?(roc)
Attachment #644269 - Flags: review?(roc)
(Assignee)

Comment 29

5 years ago
Created attachment 644280 [details] [diff] [review]
Part 8: Ignore letter-spacing in SVG text frames.

We do actually want to support letter-spacing eventually, but it's a bit tricky with the PropertyProviders, and we don't support it now, so best to force it to be ignored and not mess up the text layout.
Attachment #644280 - Flags: review?(roc)
(Assignee)

Comment 30

5 years ago
Created attachment 644281 [details] [diff] [review]
Part 9: Ignore word-spacing in SVG text frames.

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

Comment 31

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

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

Comment 32

5 years ago
Created attachment 644284 [details] [diff] [review]
Part 12: Ignore margins, borders and padding on SVG text frames.

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

Comment 33

5 years ago
Created attachment 644285 [details] [diff] [review]
Part 14: Ignore float in SVG text frames.

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

Comment 34

5 years ago
Created attachment 644286 [details] [diff] [review]
Part 15: Don't treat SVG text frames as being positioned.

SVG text frames are not positioned.
Attachment #644286 - Flags: review?(roc)
Attachment #644269 - Attachment is patch: true
Attachment #644280 - Attachment is patch: true
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+
Attachment #644281 - 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.
Attachment #644284 - Flags: review?(roc) → review+
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

5 years ago
Created attachment 644475 [details] [diff] [review]
Part 11a: Ensure we reflow SVG text when dominant-baseline changes.

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

Here's the patch for that.  The test I've added to Part 43 where I've got all my tests.
Attachment #644475 - Flags: review?(roc)
Attachment #644475 - Flags: review?(roc) → review+
(Assignee)

Comment 42

5 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

5 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

5 years ago
Created attachment 648189 [details] [diff] [review]
Part 14: Ignore float in SVG text frames. (v1.1)

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

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

Comment 47

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

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

Comment 48

5 years ago
Created attachment 648192 [details] [diff] [review]
Part 3: Add a frame state bit and anonymous box pseudo for SVG text frames.
Attachment #648192 - Flags: review?(roc)
(Assignee)

Updated

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

Comment 49

5 years ago
Created attachment 648193 [details] [diff] [review]
Part 4: Add FCDATA_ flag for SVG text.
Attachment #648193 - Flags: review?(roc)
(Assignee)

Comment 50

5 years ago
Created attachment 648195 [details] [diff] [review]
Part 7: Add IsFrameSVGText helper function to nsTextFrame and nsCSSFrameConstructor.
Attachment #648195 - Flags: review?(roc)
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).
(Assignee)

Comment 52

5 years ago
Created attachment 648197 [details] [diff] [review]
Part 10: Ignore text-align and text-align-end in SVG text frames.
Attachment #648197 - Flags: review?(roc)
(Assignee)

Comment 53

5 years ago
Created attachment 648199 [details] [diff] [review]
Part 13: Make :first-letter apply to <svg:text>.
Attachment #648199 - Flags: review?(roc)
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
Attachment #648192 - Flags: review?(roc) → review+
(Assignee)

Comment 55

5 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?
Attachment #648193 - Flags: review?(roc) → review+
(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.
Attachment #648199 - Flags: review?(roc) → review+
(Assignee)

Updated

5 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

5 years ago
Created attachment 648204 [details] [diff] [review]
Part 14: Ignore float in SVG text frames. (v1.2)
Attachment #648189 - Attachment is obsolete: true
Attachment #648189 - Flags: review?(roc)
Attachment #648204 - Flags: review?(roc)
(Assignee)

Updated

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

Comment 60

5 years ago
Created attachment 648205 [details] [diff] [review]
Part 15: Don't treat SVG text frames as being positioned. (v1.2)
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

5 years ago
Created attachment 648216 [details] [diff] [review]
Part 7: Add IsFrameSVGText helper function to nsIFrame. (v1.1)
Attachment #648195 - Attachment is obsolete: true
Attachment #648195 - Flags: review?(roc)
Attachment #648216 - Flags: review?(roc)
(Assignee)

Updated

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

Comment 64

5 years ago
Created attachment 648217 [details] [diff] [review]
Part 10: Ignore text-align and text-align-end in SVG text frames. (v1.1)
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.
Attachment #648217 - Flags: review?(roc) → review+
Attachment #648216 - Flags: review?(roc) → review+
(Assignee)

Comment 66

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

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

5 years ago
Attachment #648217 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/7bb6717bc95f
https://hg.mozilla.org/mozilla-central/rev/5d5b4d1a78f3
https://hg.mozilla.org/mozilla-central/rev/b5d5270fbd92
https://hg.mozilla.org/mozilla-central/rev/57377260ce4f
https://hg.mozilla.org/mozilla-central/rev/12d4dbe798eb
https://hg.mozilla.org/mozilla-central/rev/ddf80ca67833
https://hg.mozilla.org/mozilla-central/rev/19cd8d99dfbe
https://hg.mozilla.org/mozilla-central/rev/58c16e8cfdc7
https://hg.mozilla.org/mozilla-central/rev/f4f26647c555
https://hg.mozilla.org/mozilla-central/rev/4aeedcac9ecb
https://hg.mozilla.org/mozilla-central/rev/3efee683c154
https://hg.mozilla.org/mozilla-central/rev/b99801e9a0c0
https://hg.mozilla.org/mozilla-central/rev/d7749fadb594
(Assignee)

Comment 68

5 years ago
Created attachment 648620 [details] [diff] [review]
Part 17: Ensure non-SVG child elements of SVG text elements do not get frames.
Attachment #648620 - Flags: review?(bzbarsky)
(Assignee)

Comment 69

5 years ago
Created attachment 648621 [details] [diff] [review]
Part 18: Ensure even line-ending white space SVG text frames are created.
Attachment #648621 - Flags: review?(bzbarsky)
(Assignee)

Comment 70

5 years ago
Created attachment 648622 [details] [diff] [review]
Part 21: Avoid assertions when nsStyleContext::GetVisitedDependentColor is called for an SVG paint property.

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

Comment 71

5 years ago
Created attachment 648624 [details] [diff] [review]
Part 20: Make nsTextFrame QueryFrame-able.
Attachment #648624 - Flags: review?(roc)
(Assignee)

Comment 72

5 years ago
Created attachment 648625 [details] [diff] [review]
Part 22: Add function for getting the 'is SVG text using CSS frames' pref.
Attachment #648625 - Flags: review?(jwatt)
(Assignee)

Updated

5 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

5 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

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

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)
Attachment #648624 - Flags: review?(roc) → review+
(Assignee)

Comment 77

5 years ago
Created attachment 648632 [details] [diff] [review]
Part 29: Don't underline links within SVG text by default.
Attachment #648632 - Flags: 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

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

Comment 81

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/47f0da584ea3
https://hg.mozilla.org/integration/mozilla-inbound/rev/270c17de5f45
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8c6025c0881
https://hg.mozilla.org/integration/mozilla-inbound/rev/6899651a0f09
https://hg.mozilla.org/integration/mozilla-inbound/rev/05848864b5f9
(Assignee)

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Comment 82

5 years ago
Created attachment 649067 [details] [diff] [review]
Part 26: Give nsTextFrames the ability to invoke callbacks after painting different parts of the text.

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

Comment 83

5 years ago
Created attachment 649068 [details] [diff] [review]
Part 27: Ignore text-shadow in SVG text frames.

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

Comment 84

5 years ago
Created attachment 649069 [details] [diff] [review]
Part 24: Add UA style sheet rule to inherit unicode-bidi on <svg:text> to its anonymous block child.

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

Comment 85

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

(Forgot some changes.)
Attachment #649070 - Flags: review?(roc)
(Assignee)

Updated

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

Updated

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

Comment 86

5 years ago
Created attachment 649072 [details] [diff] [review]
Part 28: Paint SVG text frames using 'fill' not 'color'.
Attachment #649072 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/47f0da584ea3
https://hg.mozilla.org/mozilla-central/rev/270c17de5f45
https://hg.mozilla.org/mozilla-central/rev/d8c6025c0881
https://hg.mozilla.org/mozilla-central/rev/6899651a0f09
https://hg.mozilla.org/mozilla-central/rev/05848864b5f9
Comment 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

5 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

5 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

5 years ago
Created attachment 649520 [details] [diff] [review]
Part 27: Ignore text-shadow in SVG text frames. (v1.1)
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

5 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

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

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

So now nsTextPaintStyle can be used in a "non-resolving" mode, where the special color values like NS_SAME_AS_FOREGROUND_COLOR are not resolved down into the actual color.  We set this mode when a DrawPathCallbacks object is passed in to PaintText().
Attachment #649070 - Attachment is obsolete: true
Attachment #649070 - Flags: review?(roc)
Attachment #649524 - Flags: review?(roc)
(Assignee)

Comment 99

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

I've changed GetTextDecorations to have a non-colour-resolving mode too, so that the callbacks can be told to paint a text decoration with NS_SAME_AS_FOREGROUND_COLOR.
Attachment #649527 - Flags: review?(roc)
(Assignee)

Updated

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

Comment 103

5 years ago
Created attachment 649558 [details] [diff] [review]
Part 19: Add function to convert text decorations to a path.
Attachment #649558 - Flags: review?(roc)
(Assignee)

Updated

5 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

5 years ago
Created attachment 649572 [details] [diff] [review]
Part 32: Allow nsCharClipDisplayItems to be constructed without an nsDisplayListBuilder.

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

Comment 106

5 years ago
Created attachment 649574 [details] [diff] [review]
Part 37: Reflow SVG text even if undisplayed content is added/removed.
Attachment #649574 - Flags: review?(roc)
(Assignee)

Comment 107

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

I've taken this out so that nsSVGTextFrame2 can call it to determine the closest part of a text frame to a point (used when handling selection with the mouse), since parts of text frames get rendered in different places due to SVG glyph positioning.  I've made it a template because I have my text frame parts in SVG user units.
Attachment #649575 - Flags: review?(roc)
Attachment #649572 - Flags: review?(roc) → review+
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+

Updated

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

Updated

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

5 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

5 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

5 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

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

Comment 117

5 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

5 years ago
Created attachment 649603 [details] [diff] [review]
Part 37: Reflow SVG text even if undisplayed content is added/removed. (v1.1)
Attachment #649574 - Attachment is obsolete: true
Attachment #649574 - Flags: review?(roc)
Attachment #649603 - Flags: review?(roc)
(Assignee)

Comment 119

5 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

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

Comment 120

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

5 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

5 years ago
Created attachment 649707 [details]
Test case for baseline-shift support for super/subscript with mixed colors and fonts

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

5 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

5 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

5 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

5 years ago
Created attachment 649900 [details] [diff] [review]
Part 19: Add function to convert text decorations to a path. (v1.1)

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

Comment 130

5 years ago
Created attachment 649945 [details] [diff] [review]
Part 37: Reflow SVG text even if undisplayed content is added/removed. (v1.2)
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

5 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

5 years ago
Created attachment 649948 [details] [diff] [review]
Part 31a: Atom for new frame class nsSVGTextFrame2.

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

5 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

5 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?
Yes.
(Assignee)

Comment 140

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

Comment 142

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Updated

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

Comment 143

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/76835a9e3efb
(Assignee)

Updated

5 years ago
Attachment #649069 - Flags: checkin+

Comment 144

5 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.
https://hg.mozilla.org/mozilla-central/rev/fcde7b899e87
https://hg.mozilla.org/mozilla-central/rev/856faff0f587
https://hg.mozilla.org/mozilla-central/rev/a90b2757ccc2
https://hg.mozilla.org/mozilla-central/rev/dd0e5c739347
https://hg.mozilla.org/mozilla-central/rev/e45aca2a5d87
https://hg.mozilla.org/mozilla-central/rev/342718ea6734
https://hg.mozilla.org/mozilla-central/rev/5cd1bb53c262
https://hg.mozilla.org/mozilla-central/rev/9f477f32c157
https://hg.mozilla.org/mozilla-central/rev/76835a9e3efb
Comment 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

5 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

5 years ago
Created 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".

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

5 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

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

Comment 154

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b32dbbd0df6f
(Assignee)

Comment 155

5 years ago
Created attachment 650809 [details] [diff] [review]
Part 30: Allow PathExtentsToMaxStrokeExtents to work with nsTextFrames.

This is needed so that nsSVGTextFrame2 get bounding boxes / covered regions of nsTextFrame children.
Attachment #650809 - Flags: review?(jwatt)

Comment 156

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

5 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

5 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

5 years ago
Created attachment 650816 [details] [diff] [review]
Part 39: Get bounding boxes of SVG text using new text frames correctly.

nsSVGTextFrame2 doesn't implement nsSVGTextContainerFrame, so we need to go searching for the closest ancestor nsSVGTextFrame2 if we request a bbox for an ns{Block,Inline,Text}Frame instead.
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... :-|

Updated

5 years ago
Attachment #650789 - Flags: review?(jwatt) → review+

Updated

5 years ago
Attachment #650809 - Flags: review?(jwatt) → review+

Updated

5 years ago
Attachment #650816 - Flags: review?(jwatt) → review+
(Assignee)

Comment 160

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

Comment 162

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d65dc56b7243
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb0d0666b2bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/60f0b304f8c6
(Assignee)

Updated

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

Updated

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

Updated

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

Comment 163

5 years ago
Created attachment 651089 [details] [diff] [review]
Part 38: Ignore the non-SVG frames when propagating SVG changes through a tree.

We'll encounter NS_FRAME_IS_SVG_TEXT frames as we propagate SVG changes down the tree, so we shouldn't assert if we find them.
Attachment #651089 - Flags: review?(jwatt)

Updated

5 years ago
Attachment #651089 - Flags: review?(jwatt) → review+
https://hg.mozilla.org/mozilla-central/rev/b32dbbd0df6f
https://hg.mozilla.org/mozilla-central/rev/d65dc56b7243
https://hg.mozilla.org/mozilla-central/rev/eb0d0666b2bf
https://hg.mozilla.org/mozilla-central/rev/60f0b304f8c6

Comment 165

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

Comment 166

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/8c95a0d692fa
(Assignee)

Updated

5 years ago
Attachment #651089 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/8c95a0d692fa
(Assignee)

Comment 168

5 years ago
Created 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)

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

5 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

5 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

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

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

Comment 172

5 years ago
Created attachment 655521 [details] [diff] [review]
Part 31d: Hook SVG text scaling into CSS text frames.

This is how we will handle scaling text to handle very small or very large font sizes.  nsSVGTextFrame2 will return a scaling factor to apply to the font-size of all of its text frames.
Attachment #655521 - Flags: review?(roc)
(Assignee)

Comment 173

5 years ago
Created attachment 655523 [details] [diff] [review]
Part 33: Allow new SVG text frames in clip paths.
Attachment #655523 - Flags: review?(longsonr)
(Assignee)

Comment 174

5 years ago
Created attachment 655525 [details] [diff] [review]
Part 31c: Ensure SVG text with effects applied to them are in the right position.

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

Comment 175

5 years ago
Created attachment 655526 [details] [diff] [review]
Part 34: Implement SVG DOM text methods in terms of the new SVG text frame.

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

Updated

5 years ago
Attachment #655523 - Flags: review?(longsonr) → review+

Comment 176

5 years ago
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.
Blocks: 786031
(Assignee)

Comment 178

5 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

5 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

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

Comment 181

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

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

Comment 183

5 years ago
ask bz? or roc? 

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

Comment 184

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

Updated

5 years ago
Depends on: 786216

Updated

5 years ago
No longer depends on: 786216

Updated

5 years ago
Attachment #655525 - Flags: review?(jwatt) → review+
(Assignee)

Comment 185

5 years ago
Created attachment 656320 [details] [diff] [review]
Part 42a: Handle SVG text frame pref changes gracefully.

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

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

Robert: the changes in nsSVGUtils.cpp and nsSVGOuterSVGFrame.cpp are on top of two patches I haven't yet uploaded (they're parts 35 and 36 in http://mcc.id.au/temp/bug-655877/ if you care to look).
Attachment #656320 - Flags: review?(longsonr)
Attachment #656320 - Flags: review?(bzbarsky)
Attachment #655521 - Flags: review?(roc) → review+
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 187

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

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

Comment 189

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

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

Here's the big one.  I haven't got it working with DLBI yet, but I suspect that'll be a small patch (and would be better as a separate patch anyway).
Attachment #657565 - Flags: review?(jwatt)
(Assignee)

Comment 191

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

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

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

Comment 192

5 years ago
Created attachment 657567 [details] [diff] [review]
Part 36: Make referenced text path updates cause SVG text relayout.

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

Comment 193

5 years ago
Created attachment 657568 [details] [diff] [review]
Part 41a: Move ToCanvasBounds to nsSVGUtils.

I want to call this from both nsSVGForeignObjectFrame and nsSVGTextFrame2.
Attachment #657568 - Flags: review?(longsonr)

Comment 194

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

Updated

5 years ago
Attachment #657567 - Flags: review?(longsonr) → review+

Comment 195

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

5 years ago
Created attachment 657630 [details] [diff] [review]
Part 41b: Make SVG text selectable with the mouse.

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

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

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

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

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

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

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

and not

  x: 12000 + 60
  y: 6000

so that when selection happens the nsTextFrame thinks the point is over the "B".
Attachment #657630 - Flags: review?(roc)
Attachment #657630 - Flags: review?(jwatt)
(Assignee)

Comment 197

5 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

5 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 199

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

5 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

5 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

5 years ago
Created attachment 658703 [details] [diff] [review]
Part 31b: New frame class nsSVGTextFrame2 for SVG text using CSS block/inline/text frames for layout. (v1.1)

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

Those changes are needed due to fixes in later patches.  I also addressed a couple of Robert Longson's comments.
Attachment #658703 - Flags: review?(jwatt)
(Assignee)

Updated

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

Updated

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

Comment 209

5 years ago
Created attachment 658704 [details] [diff] [review]
Part 41b: Make SVG text selectable with the mouse. (v1.1)

Address review comments.  I made the Transform* functions on nsSVGTextFrame2 work with frame types other than nsTextFrame, in case they ever get called with an nsInlineFrame for a <tspan>, for example.
Attachment #657630 - Attachment is obsolete: true
Attachment #657630 - Flags: review?(roc)
Attachment #657630 - Flags: review?(jwatt)
Attachment #658704 - Flags: review?(roc)
Attachment #658704 - Flags: review?(jwatt)
(Assignee)

Comment 210

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

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

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

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

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

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

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

  NS_ABORT_IF_FALSE(newLen <= oldLen, ...)

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

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

That might be helpful.  Like gfxContextPathAutoSaveRestore, it would need to take a flag to say whether a push was actually needed.
Comment on attachment 658704 [details] [diff] [review]
Part 41b: Make SVG text selectable with the mouse. (v1.1)

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

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

delete one 'relative to'

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

{} around if/else bodies

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

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

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

ditto

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

More accurately, the user space for this frame is relative to the top-left of mRect.
Attachment #658704 - Flags: review?(roc) → review+
(Assignee)

Updated

5 years ago
Depends on: 788918
(Assignee)

Updated

5 years ago
Depends on: 788940
(Assignee)

Comment 212

5 years ago
Created attachment 658758 [details] [diff] [review]
Part 45: Note a crashtest that involves XBL as triggering assertions.

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

Comment 213

5 years ago
Created attachment 658759 [details] [diff] [review]
Part 46: Test fix due to subpixel AA not being used for SVG text with strokes.

Text that gets painted directly through nsTextFrames gets painted with sub-pixel antialising, but if we fall back to the path that paints the text with paths (for example when using SVG pattern fills) then we just get normal AA.  I think that's fine, but it causes a reftest failure.  This patch just forces text-layout-06-ref.svg to fall back to the non-sub-pixel AAed path, by using a transparent paint server for the text's stroke.
Attachment #658759 - Flags: review?(longsonr)

Updated

5 years ago
Attachment #658759 - Flags: review?(longsonr) → review+

Comment 214

5 years ago
Comment on attachment 658758 [details] [diff] [review]
Part 45: Note a crashtest that involves XBL as triggering assertions.

Raise a follow up to track please.
Attachment #658758 - Flags: review?(longsonr) → review+
(Assignee)

Comment 215

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1ad5d65bf44
https://hg.mozilla.org/integration/mozilla-inbound/rev/e79271d0adfe
(Assignee)

Comment 216

5 years ago
(In reply to Robert Longson from comment #214)
> Raise a follow up to track please.

Bug 788953.
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)

nsCSSKeywordList.h is out of sync with the name used elsewhere; that
means this patch doesn't even compile.  I'm presuming it's wrong and
the rest is right.

You should add the new value to property_database.js.

The !NewlineIsSignificant test in
nsCSSFrameConstructor::ConstructFramesFromItem needs adjusting, since
you need to construct text frames for your new value (even for non-SVG;
we shouldn't introduce weird broken behavior).  And nsStyleText::
CalcDifference needs adjusting to match.

Likewise I think nsTextFrame::AddInline{Min,Pref}WidthForFlow probably
need adjusting, although maybe they're ok.

roc should really review the text frame bits; r=dbaron on the rest
Attachment #654901 - Flags: review?(roc)
Attachment #654901 - Flags: review?(dbaron)
Attachment #654901 - Flags: review+
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]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrameThebes.cpp
@@ +769,5 @@
> +  for (PRInt32 i = 0; i < len; ++i) {
> +    char ch = str[i];
> +    if (ch == '\n')
> +      continue;
> +    return false;

if (ch != '\n')
  return false;
Attachment #654901 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/f1ad5d65bf44
https://hg.mozilla.org/mozilla-central/rev/e79271d0adfe
(Assignee)

Comment 220

5 years ago
Created attachment 659088 [details] [diff] [review]
Part 35: Ensure SVG text is updated when attributes on text content children change. (v1.1)

No more nsInlineFrame changes to listen to attributes.  I've bundled up nsSVGTextFrame2::AttributeChanged into nsSVGTextFrame2::MutationObserver::AttributeChanged.  Unlike in the first version of the patch, I don't call NotifyGlyphMetricsChange on nsSVGTextFrame2s on xml:space="" changes, because this will just be handled with style updates.
Attachment #657566 - Attachment is obsolete: true
Attachment #657566 - Flags: review?(roc)
Attachment #657566 - Flags: review?(jwatt)
Attachment #659088 - Flags: review?(jwatt)
(Assignee)

Comment 221

5 years ago
Created attachment 659106 [details] [diff] [review]
Part 23b: Add UA style sheet rules to map xml:space='preserve' to white-space:-moz-pre-discard-newlines on SVG text elements.

Now we don't break content we were concerned about in comment 111.
Attachment #648631 - Attachment is obsolete: true
Attachment #659106 - Flags: review?(jwatt)
(Assignee)

Comment 222

5 years ago
Created attachment 659121 [details] [diff] [review]
Part 44: Fix some bidi tests whose rendering behavior has changed a bit with the new SVG text support.

This updates bidiSVG-03 and -04 to the correct SVG bidi text layout.

I also reworked bidiMirroring.svg to (a) put one mirrored character per <text> element, to avoid differences between layout pre-and-post new SVG text layout, and (b) to use a WOFF font that has glyphs for each mirrored characters, so that mirroring would actually occur (it wouldn't if it fell back to hex character box glyphs).
Attachment #659121 - Flags: review?(smontagu)
(Assignee)

Comment 223

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d9b336d8b4b
https://hg.mozilla.org/mozilla-central/rev/5d9b336d8b4b
Blocks: 791037

Updated

5 years ago
Depends on: 791434
(Assignee)

Updated

5 years ago
Depends on: 794347
(Assignee)

Comment 225

5 years ago
Created attachment 665263 [details] [diff] [review]
Part 31b: New frame class nsSVGTextFrame2 for SVG text using CSS block/inline/text frames for layout. (v1.2)

Some bug fixes, and the remainder of Robert Longson's comments.
Attachment #658703 - Attachment is obsolete: true
Attachment #658703 - Flags: review?(jwatt)
Attachment #665263 - Flags: review?(jwatt)
(Assignee)

Comment 226

5 years ago
Created attachment 665264 [details] [diff] [review]
Part 35: Ensure SVG text is updated when attributes on text content children change. (v1.2)

Found we can't just move all the AttributeChanged logic into the mutation observer, since animated value updates directly call AttributeChanged on the frame class.
Attachment #665264 - Flags: review?(jwatt)
(Assignee)

Comment 227

5 years ago
Created attachment 665265 [details] [diff] [review]
Part 41b: Make SVG text selectable with the mouse. (v1.2)

Fixed some issues with text selection when full page zoom is active.  Didn't make any layout/{base,generic}/ changes, so carrying over r=roc for that part.
Attachment #658704 - Attachment is obsolete: true
Attachment #658704 - Flags: review?(jwatt)
Attachment #665265 - Flags: review?(jwatt)
(Assignee)

Updated

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

Comment 228

5 years ago
Created attachment 665268 [details] [diff] [review]
Part 43: Tests for new SVG text support.

Some tests.  Many of them are tests for combinations of anchoring, ltr/rtl/bidi text, and glyph positioning attributes.
Attachment #665268 - Flags: review?(jwatt)
Attachment #659121 - Flags: review?(smontagu) → review+
(Assignee)

Comment 229

5 years ago
Just noticed, this bug fix hunk in part 41b:

@@ -2753,17 +2768,17 @@ nsSVGTextFrame2::MutationObserver::ContentRemoved(
 void
 nsSVGTextFrame2::MutationObserver::AttributeChanged(
                                                 nsIDocument* aDocument,
                                                 mozilla::dom::Element* aElement,
                                                 int32_t aNameSpaceID,
                                                 nsIAtom* aAttribute,
                                                 int32_t aModType)
 {
-  if (aElement->IsSVG()) {
+  if (!aElement->IsSVG()) {
     return;
   }

should have been folded into part 35.
(Assignee)

Comment 230

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6786b5f4ee4a
(Assignee)

Comment 231

5 years ago
Backed out part 44, since it depends on the pref having landed:

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

Updated

5 years ago
Attachment #659106 - Flags: review?(jwatt) → review+
(Assignee)

Comment 232

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b313eadbbc11
https://hg.mozilla.org/mozilla-central/rev/b313eadbbc11
Flags: in-testsuite+
(Assignee)

Comment 234

5 years ago
Created attachment 668692 [details] [diff] [review]
Part 31e: Ensure gradients are positioned correctly for text in filters.

A fix for one of the last couple of reftest failures; I was passing in the wrong matrix to be used for setting up the CTM for paint servers.
Attachment #668692 - Flags: review?(jwatt)
(Assignee)

Comment 235

5 years ago
Created attachment 668693 [details] [diff] [review]
Part 43a: A few more tests.
Attachment #668693 - Flags: review?(jwatt)
(Assignee)

Comment 236

5 years ago
Created attachment 668696 [details] [diff] [review]
Part 45: Fixes for DLBI.

These were the changes I needed when rebasing on top of DLBI.  The nsSVGTextPathProperty::TargetIsValid changes were needed to avoid infinite reflow loops with layout/svg/crashtests/472782-1.svg.  r?roc for the nsFrame.cpp change; this was simpler than registering the rendering observer.
Attachment #668696 - Flags: review?(roc)
Attachment #668696 - Flags: review?(jwatt)
(Assignee)

Comment 237

5 years ago
Created attachment 668699 [details] [diff] [review]
Part 0: Add pref for new CSS-using SVG text frames, initially turned off.

Create the pref, starting disabled.
Attachment #668699 - Flags: review?(jwatt)
(Assignee)

Comment 238

5 years ago
Created attachment 668701 [details] [diff] [review]
Part LAST: Enable pref for new CSS-using SVG text frames.
Attachment #668701 - Flags: review?(jwatt)
(Assignee)

Comment 239

5 years ago
Here are try runs with all the patches applied:

* with pref off: https://tbpl.mozilla.org/?tree=Try&rev=d02ecf775677
* with pref on: https://tbpl.mozilla.org/?tree=Try&rev=9c22c9c7d2ba

In both cases,

  Bug 623454 - Permaorange - reftests/bugs/580863-1.html on WinXP

seems to pass now (not sure why, but checking the reftest image it looks legit).

With the pref on, there is one final failure that I haven't fixed yet:

REFTEST TEST-UNEXPECTED-FAIL | http://10.250.48.210:30066/tests/layout/reftests/svg/dynamic-text-06.svg | image comparison (==), max difference: 113, number of differing pixels: 1

That's the test where some large font-size text has its transform scale animated with setTimeout.  I guess it is an invalidation problem; one of the antialiased pixels is being left behind.  I don't have an Android debug environment set up, so I haven't looked into this properly.

The other issue that might prevent flipping the pref on is that OpenType SVG glyphs now do not render in SVG documents.  I'm working on that now.
Comment on attachment 668696 [details] [diff] [review]
Part 45: Fixes for DLBI.

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

The rest looks fine.

::: layout/generic/nsFrame.cpp
@@ +4785,5 @@
>  }
>  
>  static void InvalidateFrameInternal(nsIFrame *aFrame, bool aHasDisplayItem = true)
>  {
> +  if (aFrame->IsSVGText() && aFrame->GetType() != nsGkAtoms::svgTextFrame2) {

Can we avoid all frames hitting this by overriding virtual InvalidateFrame() and InvalidateFrameWithRect() methods?
(Assignee)

Comment 241

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #240)
> Can we avoid all frames hitting this by overriding virtual InvalidateFrame()
> and InvalidateFrameWithRect() methods?

Yes, we'd need to override them on ns{Block,Inline,Text}Frame.  I imagine that covers most frames, but I guess it would save us something.
(Assignee)

Comment 242

5 years ago
Created attachment 668849 [details] [diff] [review]
Part 45: Fixes for DLBI. (v1.1)
Attachment #668696 - Attachment is obsolete: true
Attachment #668696 - Flags: review?(roc)
Attachment #668696 - Flags: review?(jwatt)
Attachment #668849 - Flags: review?(roc)
Attachment #668849 - Flags: review?(jwatt)
(Assignee)

Comment 243

5 years ago
Created attachment 668890 [details] [diff] [review]
Part 46: Paint SVG glyphs with new SVG text frames.

This should make SVG glyphs render properly.  The patch:

* copied over the SetupCairoState etc. functions from nsSVGGlyphFrame to nsSVGTextFrame2, and modified them to take the nsIFrame* child that they should look for style on (instead of this), and then called SetupCairoState in PaintSVG to record the object paint values
* fixed nsSVGTextFrame2::ReflowSVG to make selected fill:none text actually render
* added another pair of callbacks for before and after an SVG glyph is painted
* renamed nsSVGGlyphFrame::SVGTextObjectPaint to mozilla::SVGTextObjectPaint to use it in nsSVGTextFrame2
* modified various methods on nsTextFrame to pass in a gfxTextObjectPaint value
* added a bool on gfxTextRunDrawCallbacks to indicate whether SVG glyphs should be painted while in gfxFont::GLYPH_PATH mode, and modified gfxFont::Draw to do that and invoke the new callbacks (an alternative would be to add a new gfxFont::DrawMode value; that might be better)
* added a null check in GlyphBufferAzure::Flush for the AzureState's pattern, as that was null when fill:none SVG text was being painted (I'm unsure whether this null check is sufficient; maybe fill:none should be treated on the SVGTextObjectPaint as if it were rgba(0,0,0,0) to avoid going down the path of filling the text with a pattern)
Attachment #668890 - Flags: review?(roc)
Comment on attachment 668849 [details] [diff] [review]
Part 45: Fixes for DLBI. (v1.1)

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

::: layout/generic/nsBlockFrame.cpp
@@ +493,5 @@
> +{
> +  if (IsSVGText()) {
> +    nsIFrame* svgTextFrame =
> +      nsLayoutUtils::GetClosestFrameOfType(GetParent(),
> +                                           nsGkAtoms::svgTextFrame2);

We don't actually need to do this for blocks, right? If a block is IsSVGText(0, then it must be a direct child of the svgTextFrame2 so we can just invalidate it directly.

But I guess it doesn't matter.
Attachment #668849 - Flags: review?(roc) → review+
Comment on attachment 668890 [details] [diff] [review]
Part 46: Paint SVG glyphs with new SVG text frames.

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

::: gfx/thebes/gfxFont.cpp
@@ +1711,5 @@
> +ForcePaintingDrawMode(gfxFont::DrawMode aDrawMode)
> +{
> +    return aDrawMode == gfxFont::GLYPH_PATH ?
> +        gfxFont::DrawMode(gfxFont::GLYPH_FILL | gfxFont::GLYPH_STROKE) :
> +        aDrawMode;

I don't really understand why you're doing this, so at least it needs to be documented.

The existing drawing code just ignores the SVG glyphs when asked to draw with GLYPH_PATH. Why aren't we continuing to do that?
(Assignee)

Comment 246

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #245)
> The existing drawing code just ignores the SVG glyphs when asked to draw
> with GLYPH_PATH. Why aren't we continuing to do that?

GLYPH_PATH will be used whenever we need to do some SVG specific painting that the normal text frames wouldn't handle (like filling with a pattern), but if we have SVG glyphs we still want them to get painted.  So we would either need to ensure that when we have only SVG glyphs, we call into nsTextFrame::PaintText without the callback object, or do the above which is to actually paint the SVG glyphs even if GLYPH_PATH were set.  I agree it does kind of subvert the meaning of "GLYPH_PATH", though.
(Assignee)

Comment 247

5 years ago
And I am not sure how easy it would be to tell that it is safe to use GLYPH_{FILL,STROKE} because we only have SVG glyphs.
Comment on attachment 668890 [details] [diff] [review]
Part 46: Paint SVG glyphs with new SVG text frames.

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

Right OK, sorry.
Attachment #668890 - Flags: review?(roc) → review+
Comment on attachment 665264 [details] [diff] [review]
Part 35: Ensure SVG text is updated when attributes on text content children change. (v1.2)

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

::: layout/svg/nsSVGTextFrame2.cpp
@@ +2780,5 @@
> +      childElementFrame->Properties().Delete(nsSVGEffects::HrefProperty());
> +      mFrame->NotifyGlyphMetricsChange();
> +    }
> +  } else {
> +    if (IsGlyphPositioningAttribute(aAttribute)) {

Check |aNameSpaceID == kNameSpaceID_None &&| here.
Attachment #665264 - Flags: review?(jwatt) → review+
Comment on attachment 668692 [details] [diff] [review]
Part 31e: Ensure gradients are positioned correctly for text in filters.

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

Roll this into the original nsSVGTextFrame2 patch please.

r=jwatt with the following issue addressed.

::: layout/svg/nsSVGTextFrame2.cpp
@@ +2982,5 @@
>      gfx->SetMatrix(runTransform);
>  
>      nsRect frameRect = frame->GetVisualOverflowRect();
>      if (ShouldRenderAsPath(aContext, frame)) {
> +      SVGTextDrawPathCallbacks callbacks(aContext, frame, initialMatrix);

I think this is wrong. You probably want the FOR_PAINTING transform multiplied by initialMatrix, and a test with a pattern containing a transformed <g> with some text in it.
Attachment #668692 - Flags: review?(jwatt) → review+