Closed
Bug 655877
(svgtext)
Opened 13 years ago
Closed 11 years ago
SVG text should use CSS text frames to gain support for various text layout features
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: heycam, Assigned: heycam)
References
(Blocks 1 open bug, )
Details
Attachments
(59 files, 39 obsolete files)
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•13 years ago
|
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → cam
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•12 years ago
|
||
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 | ||
Comment 2•12 years ago
|
||
Painting of solid fill, no stroke text with multiple x/y/dx/dy attributes now works, including with decorations.
Updated•12 years ago
|
Attachment #605633 -
Attachment is patch: true
Assignee | ||
Comment 3•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
Yes, progress looks very promising... ;-)
Assignee | ||
Comment 7•12 years ago
|
||
Fix some problems with positioning of text runs that have half a ligature in them, and allow partial ligatures to be reported back when drawing a gfxTextRun in GLYPH_PATH mode through a callback. Selecting text with the mouse works now too.
Attachment #608975 -
Attachment is obsolete: true
Assignee | ||
Comment 8•12 years ago
|
||
* SVG DOM text methods (getSubStringLength etc.) support. * Text on a path support. * Some refactoring/cleanup.
Attachment #610396 -
Attachment is obsolete: true
Assignee | ||
Comment 9•12 years ago
|
||
* 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 | ||
Comment 10•12 years ago
|
||
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•12 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 | ||
Comment 13•12 years ago
|
||
Current split up patch queue is here: http://mcc.id.au/temp/bug-655877/
Assignee | ||
Updated•12 years ago
|
Attachment #630085 -
Attachment is obsolete: true
Assignee | ||
Comment 14•12 years ago
|
||
This patch makes sure that values of 'display' other than "none" get treated the same as "inline" on SVG text frames. (The NS_FRAME_IS_SVG_TEXT state bit is set on the nsSVGTextFrame2 frame and all of its nsBlockFrame/nsInlineFrame descendants.) I searched for all of the places that looked at mDisplay on nsStyleDisplay or called the accessor functions like IsOutsideInline, and for the places where it would matter (e.g. I didn't change any of inner bits of the table frame construction code to look at SVG's sense of the display value) I called these Effective* functions. Some issues: (1) I don't quite like the "Effective" name (2) I don't know whether it would be better to put these functions somewhere other than on nsIFrame (maybe nsLayoutUtils? or should they go directly on nsStyleDisply, taking an nsIFrame* argument to check whether it is an SVG text frame?)
Attachment #640890 -
Flags: review?(roc)
Assignee | ||
Comment 15•12 years ago
|
||
Because a text run might be painted in three parts -- partial ligatures at the start of the run, the main bit, partial ligatures at the end -- I need to be able to handle those three parts separately (so my nsSVGTextFrame2 can fill and stroke them). So this adds a callback argument that, when specified, means that the glyphs will be drawn with mode gfxFont::GLYPH_PATH and then callback invoked immediately afterwards. (Part 17 extends adds a similar callback parameter to nsTextFrame, with a few different callback functions (for different parts of the text frame being rendered), which uses this gfxFont callback.)
Attachment #640891 -
Flags: review?(roc)
Assignee | ||
Comment 16•12 years ago
|
||
(Part 26 uses this, sorry, not part 17.)
Assignee | ||
Comment 17•12 years ago
|
||
This is the part where the new SVG text frames are constructed. I have this ITEM_IS_WITHIN_SVG_TEXT frame construction item flag, and the additional aIsWithinSVGText argument to FindSVGData, because we need to know whether for example an <a> ought to be an nsInlineFrame (if it is within an nsSVGTextFrame2) or an nsSVGAFrame (if it is elsewhere in an SVG tree), and when inline frames are being constructed, null is passed in for aParentFrame so we can't check aParentFrame->mState for NS_FRAME_IS_SVG_TEXT. (Although just writing this out now I wonder whether that means aParentFrame being null always implies within SVG text, in which case we wouldn't need to do this.)
Attachment #640892 -
Flags: review?(roc)
Assignee | ||
Updated•12 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•12 years ago
|
||
(Just feedback for this one at this stage, I might need to change it a bit when doing my rebasing.) I've had to move all these painting-related functions out of nsSVGGeometryFrame. Currently, nsSVGGlyphFrame is an nsSVGGeometryFrame, but in the future we'll just have nsSVGTextFrame2, which is an container frame, and its descendent nsTextFrames. I need to be able to call for example GetStrokeWidth for the nsTextFrames (or their parents) and they aren't nsSVGGeometryFrames. Do you think it's OK to do this moving?
Attachment #640895 -
Flags: feedback?(jwatt)
Comment on attachment 640890 [details] [diff] [review] Part 16: Treat all values of display other than 'none' as 'inline' in SVG text frames. Review of attachment 640890 [details] [diff] [review]: ----------------------------------------------------------------- The methods we want most callers to call should have the most obvious names, and the more specialized methods should have less obvious names. So if possible I think we want IsBlockInside to take account of SVG-ness, and a different name for methods that deliberately don't take account of SVG-ness (e.g. IsBlockInsideStyle). ::: layout/generic/nsIFrame.h @@ +2809,5 @@ > VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY = 0x01 > }; > bool IsVisibleConsideringAncestors(PRUint32 aFlags = 0) const; > > + static bool IsEffectivelyBlockInside(const nsIFrame* aFrame, const nsStyleDisplay* aDisplay) { Can't this just be called IsBlockInside? Ditto for BlockOutside and InlineOutside. And I don't really like these static methods. Seems to me the non-static methods are preferred, and for the cases where we don't have a frame, nsStyleDisplay::IsBlockInside could take an optional nsIFrame* parameter which it consults. @@ +2843,5 @@ > } > return aDisplay->IsFloating(); > } > > + static PRUint8 EffectiveDisplay(const nsIFrame* aFrame, const nsStyleDisplay* aDisplay) { Why not just call this GetDisplay()? @@ +2850,5 @@ > + } > + return aDisplay->mDisplay; > + } > + > + bool IsEffectivelyBlockInside() const { Why not IsBlockInside etc? ::: layout/style/nsStyleStruct.h @@ +1643,5 @@ > mTransformStyle == NS_STYLE_TRANSFORM_STYLE_PRESERVE_3D || > mBackfaceVisibility == NS_STYLE_BACKFACE_VISIBILITY_HIDDEN; > } > + > + PRUint8 DisplayForSVG() const { A bit ambiguous. GetDisplayForSVGElement() I would say
Comment on attachment 640891 [details] [diff] [review] Part 25: Give gfxTextRuns the ability to invoke a callback after painting glyphs and partial ligatures. Review of attachment 640891 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxFont.h @@ +2463,5 @@ > + * Callback for Draw() to use when drawing text with mode > + * gfxFont::GLYPH_PATH. > + */ > + struct GlyphPathCallback { > + virtual void GlyphPath() = 0; I think this needs a better name and a comment explaining when it's called. "This gets called after each path has been emitted into the context, possibly with some clipping applied to the context. This can be called multiple times as we draw partial ligatures with different clip regions."? Call it "NotifyPathEmitted"? Also, name the struct DrawCallbacks in case we want to add more?
Comment on attachment 640892 [details] [diff] [review] Part 42: Construct new SVG text frames if the pref is set. Review of attachment 640892 [details] [diff] [review]: ----------------------------------------------------------------- More Boris's territory.
Attachment #640892 -
Flags: review?(roc) → review?(bzbarsky)
Updated•12 years ago
|
Attachment #640895 -
Flags: feedback?(jwatt) → feedback+
Comment 22•12 years ago
|
||
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•12 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20) > ::: gfx/thebes/gfxFont.h > @@ +2463,5 @@ > > + * Callback for Draw() to use when drawing text with mode > > + * gfxFont::GLYPH_PATH. > > + */ > > + struct GlyphPathCallback { > > + virtual void GlyphPath() = 0; > > I think this needs a better name and a comment explaining when it's called. > "This gets called after each path has been emitted into the context, > possibly with some clipping applied to the context. This can be called > multiple times as we draw partial ligatures with different clip regions."? > Call it "NotifyPathEmitted"? Also, name the struct DrawCallbacks in case we > want to add more? OK, I renamed it to DrawCallbacks and added the comment.
Attachment #640891 -
Attachment is obsolete: true
Attachment #640891 -
Flags: review?(roc)
Attachment #642687 -
Flags: review?(roc)
Attachment #642687 -
Flags: review?(roc) → review+
Assignee | ||
Comment 24•12 years ago
|
||
These are the changes to handle <text><a><textPath>, which turned out a bit trickier because it's another case where you don't have access to parent frames within FindSVGData.
Attachment #643428 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 25•12 years ago
|
||
With the tests included this time.
Attachment #643428 -
Attachment is obsolete: true
Attachment #643428 -
Flags: review?(bzbarsky)
Attachment #643437 -
Flags: review?(bzbarsky)
Comment 26•12 years ago
|
||
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•12 years ago
|
||
I'll keep this in sync with the patches attached to the bug.
Assignee | ||
Comment 28•12 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19) > The methods we want most callers to call should have the most obvious names, > and the more specialized methods should have less obvious names. So if > possible I think we want IsBlockInside to take account of SVG-ness, and a > different name for methods that deliberately don't take account of SVG-ness > (e.g. IsBlockInsideStyle). OK, I have used this naming style. (There are a bunch of extra edits to the call sites of the now named IsBlockInsideStyle etc. just because of the rename.) > And I don't really like these static methods. Seems to me the non-static > methods are preferred, and for the cases where we don't have a frame, > nsStyleDisplay::IsBlockInside could take an optional nsIFrame* parameter > which it consults. I've done this, except I didn't make the argument optional, so that it is clearer that you should always call IsBlockInsideStyle if you only want it to look at mDisplay and IsBlockInside if you want to use the frame's SVG-ness. > ::: layout/style/nsStyleStruct.h > @@ +1643,5 @@ > > mTransformStyle == NS_STYLE_TRANSFORM_STYLE_PRESERVE_3D || > > mBackfaceVisibility == NS_STYLE_BACKFACE_VISIBILITY_HIDDEN; > > } > > + > > + PRUint8 DisplayForSVG() const { > > A bit ambiguous. GetDisplayForSVGElement() I would say I've turned this just into GetDisplay(nsIFrame*), in line with the naming of IsBlockInside etc. I solved the problem of the mutually referential nsIFrame and nsStyleDisplay inlines by moving the definitions of nsStyleDisplay::IsBlockInside etc. to nsStyleStructInlines.h and #including this file towards the bottom of nsIFrame.h, before the definitions of its inline IsBlockInside etc. functions.
Attachment #640890 -
Attachment is obsolete: true
Attachment #640890 -
Flags: review?(roc)
Attachment #644269 -
Flags: review?(roc)
Assignee | ||
Comment 29•12 years ago
|
||
We do actually want to support letter-spacing eventually, but it's a bit tricky with the PropertyProviders, and we don't support it now, so best to force it to be ignored and not mess up the text layout.
Attachment #644280 -
Flags: review?(roc)
Assignee | ||
Comment 31•12 years ago
|
||
The current SVG text code supports SVG 1.1's dominant-baseline property (one of the split out vertical alignment properties that XSL defined) to control vertical alignment. Eventually we want to support vertical-align on SVG text itself, but for now we just map dominant-baseline to vertical-align.
Attachment #644283 -
Flags: review?(roc)
Assignee | ||
Comment 32•12 years ago
|
||
We don't want margin, padding and borders to apply to SVG text.
Attachment #644284 -
Flags: review?(roc)
Assignee | ||
Comment 33•12 years ago
|
||
We also don't want floats.
Attachment #644285 -
Flags: review?(roc)
Assignee | ||
Comment 34•12 years ago
|
||
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•12 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #37) > r+ if you make the CalcDifference change in a separate patch. Here's the patch for that. The test I've added to Part 43 where I've got all my tests.
Attachment #644475 -
Flags: review?(roc)
Attachment #644475 -
Flags: review?(roc) → review+
Assignee | ||
Comment 42•12 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•12 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•12 years ago
|
||
I've removed most uses of IsFloatingStyle.
Attachment #644285 -
Attachment is obsolete: true
Attachment #644285 -
Flags: review?(roc)
Attachment #648189 -
Flags: review?(roc)
Assignee | ||
Updated•12 years ago
|
Attachment #648189 -
Attachment is patch: true
Assignee | ||
Comment 47•12 years ago
|
||
I've added nsStyleDisplay::IsPositioned() etc.
Attachment #644286 -
Attachment is obsolete: true
Attachment #644286 -
Flags: review?(roc)
Attachment #648190 -
Flags: review?(roc)
Assignee | ||
Comment 48•12 years ago
|
||
Attachment #648192 -
Flags: review?(roc)
Assignee | ||
Updated•12 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•12 years ago
|
||
Attachment #648193 -
Flags: review?(roc)
Assignee | ||
Comment 50•12 years ago
|
||
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•12 years ago
|
||
Attachment #648197 -
Flags: review?(roc)
Assignee | ||
Comment 53•12 years ago
|
||
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•12 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•12 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•12 years ago
|
||
Attachment #648189 -
Attachment is obsolete: true
Attachment #648189 -
Flags: review?(roc)
Attachment #648204 -
Flags: review?(roc)
Assignee | ||
Updated•12 years ago
|
Attachment #648204 -
Attachment is patch: true
Assignee | ||
Comment 60•12 years ago
|
||
Attachment #648190 -
Attachment is obsolete: true
Attachment #648190 -
Flags: review?(roc)
Attachment #648205 -
Flags: review?(roc)
Comment on attachment 648204 [details] [diff] [review] Part 14: Ignore float in SVG text frames. (v1.2) Review of attachment 648204 [details] [diff] [review]: ----------------------------------------------------------------- r+ with that ::: layout/generic/nsHTMLReflowState.cpp @@ +618,5 @@ > // see bug 154892; need to revisit later > if (frame->GetPrevInFlow()) > frameType = NS_CSS_FRAME_TYPE_BLOCK; > } > + else if (frame->IsFloating()) { disp->IsFloating(frame) (you missed this comment) ::: layout/style/nsStyleStructInlines.h @@ +60,5 @@ > +{ > + if (aFrame->GetStateBits() & NS_FRAME_IS_SVG_TEXT) { > + return false; > + } > + return IsFloatingStyle(); Actually I think this would be slightly better written as return IsFloatingStyle() && !(aFrame->GetStateBits() & NS_FRAME_IS_SVG_TEXT); just because IsFloatingStyle() will normally be false, so evaluation should be slightly quicker this way.
Attachment #648204 -
Flags: review?(roc) → review+
Comment on attachment 648205 [details] [diff] [review] Part 15: Don't treat SVG text frames as being positioned. (v1.2) Review of attachment 648205 [details] [diff] [review]: ----------------------------------------------------------------- r+ with that ::: layout/style/nsStyleStructInlines.h @@ +69,5 @@ > +{ > + if ((aFrame->GetStateBits() & NS_FRAME_IS_SVG_TEXT)) { > + return false; > + } > + return IsPositionedStyle(); As in part 14, I think it's better to test Is...PositionedStyle first. Plus you can write this as a boolean expression.
Attachment #648205 -
Flags: review?(roc) → review+
Assignee | ||
Comment 63•12 years ago
|
||
Attachment #648195 -
Attachment is obsolete: true
Attachment #648195 -
Flags: review?(roc)
Attachment #648216 -
Flags: review?(roc)
Assignee | ||
Updated•12 years ago
|
Attachment #643437 -
Attachment is patch: true
Assignee | ||
Comment 64•12 years ago
|
||
Attachment #648197 -
Attachment is obsolete: true
Attachment #648197 -
Flags: review?(roc)
Attachment #648217 -
Flags: review?(roc)
Comment on attachment 648216 [details] [diff] [review] Part 7: Add IsFrameSVGText helper function to nsIFrame. (v1.1) Review of attachment 648216 [details] [diff] [review]: ----------------------------------------------------------------- r+ with that ::: layout/generic/nsIFrame.h @@ +2809,5 @@ > VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY = 0x01 > }; > bool IsVisibleConsideringAncestors(PRUint32 aFlags = 0) const; > > + bool IsFrameSVGText() const { return mState & NS_FRAME_IS_SVG_TEXT; } IsSVGText. "Frame" is redundant.
Attachment #648217 -
Flags: review?(roc) → review+
Attachment #648216 -
Flags: review?(roc) → review+
Assignee | ||
Comment 66•12 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•12 years ago
|
Attachment #640892 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #640892 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #644269 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Attachment #644280 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #644281 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #644283 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #644284 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #644475 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #648192 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #648193 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #648199 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #648204 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #648205 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #648216 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #648217 -
Flags: checkin+
Comment 67•12 years ago
|
||
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•12 years ago
|
||
Attachment #648620 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 69•12 years ago
|
||
Attachment #648621 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 70•12 years ago
|
||
This is to allow the frame's normal painting code path to look at 'fill' instead of 'color'.
Attachment #648622 -
Flags: review?(roc)
Assignee | ||
Comment 71•12 years ago
|
||
Attachment #648624 -
Flags: review?(roc)
Assignee | ||
Comment 72•12 years ago
|
||
Attachment #648625 -
Flags: review?(jwatt)
Assignee | ||
Updated•12 years ago
|
Attachment #648624 -
Attachment description: Make nsTextFrame QueryFrame-able. → Part 20: Make nsTextFrame QueryFrame-able.
Comment 73•12 years ago
|
||
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 74•12 years ago
|
||
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•12 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•12 years ago
|
||
My current plan is to treat xml:space="preserve" just like white-space="pre", but I am having second thoughts. It would mean that markup like <text x="100" y="100" xml:space="preserve"> abc def </text> would change from being rendered as: | abc def to: | abc | def where the "a" is placed at (100,100) in both cases. There isn't a value of white-space that corresponds nicely to SVG's xml:space="preserve" rules. And I don't want to keep duplicated special SVG white space handling code. Do you think instead I should be adding say a -moz-something value for white-space that means "don't collapse spaces but don't insert line breaks either", and mapping xml:space="preserve" to that?
Attachment #648631 -
Flags: review?(jwatt)
Attachment #648622 -
Flags: review?(roc) → review?(dbaron)
Attachment #648624 -
Flags: review?(roc) → review+
Assignee | ||
Comment 77•12 years ago
|
||
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•12 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•12 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•12 years ago
|
Attachment #648620 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #648621 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #648622 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #648624 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #648632 -
Flags: checkin+
Assignee | ||
Comment 82•12 years ago
|
||
This adds all the callbacks necessary for SVG text frames to be notified to paint the different parts of an nsTextFrame. (I mistakenly included nsTextFrame::PathCallback in the Part 25 patch; I've got it in here now with it and its member functions renamed and commented.)
Attachment #649067 -
Flags: review?(roc)
Assignee | ||
Comment 83•12 years ago
|
||
Not clear whether we want text-shadow to work with SVG text, so disable it for now. (It already wouldn't work when the "drawing text as paths" code path is used.)
Attachment #649068 -
Flags: review?(roc)
Assignee | ||
Comment 84•12 years ago
|
||
We want to treat <text> as a block for the purpose of unicode-bidi, but the real block is the anonymous block child. So we inherit the value of unicode-bidi into it. (This uses up one of the 64 bidi embedding levels, but I don't think that's a real concern.)
Attachment #649069 -
Flags: review?(jwatt)
Assignee | ||
Updated•12 years ago
|
Attachment #649067 -
Attachment is obsolete: true
Attachment #649067 -
Flags: review?(roc)
Assignee | ||
Updated•12 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•12 years ago
|
||
Attachment #649072 -
Flags: review?(roc)
Comment 87•12 years ago
|
||
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•12 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•12 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•12 years ago
|
||
Attachment #649068 -
Attachment is obsolete: true
Attachment #649068 -
Flags: review?(roc)
Attachment #649520 -
Flags: review?(roc)
Comment on attachment 649520 [details] [diff] [review] Part 27: Ignore text-shadow in SVG text frames. (v1.1) Review of attachment 649520 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsTextFrameThebes.cpp @@ +5269,5 @@ > &foreground, &background); > gfxPoint textBaselinePt(aFramePt.x + xOffset, aTextBaselinePt.y); > > // Draw shadows, if any > + if (textStyle->mTextShadow && !IsSVGText()) { Use your new function! @@ +5631,5 @@ > > nscolor foregroundColor = textPaintStyle.GetTextColor(); > if (!aCallbacks) { > const nsStyleText* textStyle = GetStyleText(); > + if (textStyle->mTextShadow && !IsSVGText()) { Use your new function!
Attachment #649520 -
Flags: review?(roc) → review+
Assignee | ||
Comment 97•12 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•12 years ago
|
||
I've changed this to pass in the colour that the different parts of the text frame will be painted with into the callback functions, rather than just setting it on the context. That's clearer, and it also lets us pass in the special selection style color constants (like NS_SAME_AS_FOREGROUND_COLOR and NS_40PERCENT_FOREGROUND_COLOR). NS_SAME_AS_FOREGROUND_COLOR will be interpreted by nsSVGTextFrame2's callbacks as meaning "fill with 'fill' and then stroke with 'stroke'", whereas any other color value will be interpreted as meaning "fill with the specified color and don't stroke". So now nsTextPaintStyle can be used in a "non-resolving" mode, where the special color values like NS_SAME_AS_FOREGROUND_COLOR are not resolved down into the actual color. We set this mode when a DrawPathCallbacks object is passed in to PaintText().
Attachment #649070 -
Attachment is obsolete: true
Attachment #649070 -
Flags: review?(roc)
Attachment #649524 -
Flags: review?(roc)
Assignee | ||
Comment 99•12 years ago
|
||
I've changed GetTextDecorations to have a non-colour-resolving mode too, so that the callbacks can be told to paint a text decoration with NS_SAME_AS_FOREGROUND_COLOR.
Attachment #649527 -
Flags: review?(roc)
Assignee | ||
Updated•12 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•12 years ago
|
||
Attachment #649558 -
Flags: review?(roc)
Assignee | ||
Updated•12 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•12 years ago
|
||
We need to pass in an nsCharClipDisplayItem to nsTextFrame::PaintText, so we need to be able to create one outside of nsDisplayListBuilder.
Attachment #649572 -
Flags: review?(roc)
Assignee | ||
Comment 106•12 years ago
|
||
Attachment #649574 -
Flags: review?(roc)
Assignee | ||
Comment 107•12 years ago
|
||
I've taken this out so that nsSVGTextFrame2 can call it to determine the closest part of a text frame to a point (used when handling selection with the mouse), since parts of text frames get rendered in different places due to SVG glyph positioning. I've made it a template because I have my text frame parts in SVG user units.
Attachment #649575 -
Flags: review?(roc)
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•12 years ago
|
Attachment #648625 -
Flags: review?(jwatt) → review+
Comment 110•12 years ago
|
||
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•12 years ago
|
Attachment #649069 -
Flags: review?(jwatt) → review+
Comment 111•12 years ago
|
||
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•12 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•12 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•12 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•12 years ago
|
||
It does have a Reflow() method. I'll work it out and add it to the comments.
Assignee | ||
Comment 117•12 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•12 years ago
|
||
Attachment #649574 -
Attachment is obsolete: true
Attachment #649574 -
Flags: review?(roc)
Attachment #649603 -
Flags: review?(roc)
Assignee | ||
Comment 119•12 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•12 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•12 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?
Comment 121•12 years ago
|
||
(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•12 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•12 years ago
|
||
This example demonstrates the minimal support required for baseline-shift to be useful in Scientific/Engineering applications. The absolute offset of the super/sub script characters is not crucial, but the resulting text must rotate, scale, and accept font styles like "italic", as shown in the example.
(In reply to Cameron McCormack (:heycam) from comment #119) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment > #104) > > Actually the ideal thing here would be to have generic functionality for > > converting a stroke to its path outline, and just use that with our existing > > text-decoration-line code. (We need that functionality anyway to implement > > proposed 2D canvas extensions.) > > That would be ideal. Should we wait for that, or just switch this new > nsCSSRendering::DecorationLineToPath to it when it's ready? How about for now we have your dedicated function but just support solid underlines? That's 99% of text-decoration anyway. Hopefully Jeff will landed stroke_to_path and then we can unify the functions and support all decoration types.
(In reply to Cameron McCormack (:heycam) from comment #117) > I think it is because I am posting an eTreeChange reflow request, the > anonymous frame is a reflow root, and my nsSVGTextFrame2, when it needs > update glyph positions, will only reflow the anonymous frame if it is marked > dirty or with dirty children. Does that make sense? Isn't it the nsSVGTextFrame2 that should be a reflow root, so that reflows from the anonymous child bubble up to it and trigger updating of glyph positions?
Assignee | ||
Comment 126•12 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•12 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•12 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•12 years ago
|
||
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•12 years ago
|
||
Attachment #649603 -
Attachment is obsolete: true
Attachment #649603 -
Flags: review?(roc)
Attachment #649945 -
Flags: review?(roc)
Comment on attachment 649945 [details] [diff] [review] Part 37: Reflow SVG text even if undisplayed content is added/removed. (v1.2) Review of attachment 649945 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSFrameConstructor.cpp @@ +7277,5 @@ > + // need to trigger a reflow of the SVG text. This is for the same > + // reason as when removing frame-less content from SVG text: cached > + // information that matches up text frames to content nodes needs > + // to be updated. > + parentFrame->InvalidateOverflowRect(); This shouldn't be necessary. If reflowing the nsSVGTextFrame2 causes the character offsets to be updated, that itself should trigger invalidation. @@ +7280,5 @@ > + // to be updated. > + parentFrame->InvalidateOverflowRect(); > + mPresShell->FrameNeedsReflow(parentFrame, > + nsIPresShell::eTreeChange, > + NS_FRAME_IS_DIRTY); Since we only need to reflow the frame and we don't actually need to treat anything as dirty for the purposes of invalidating cached layout data, we can pass NS_FRAME_HAS_DIRTY_CHILD here. Should probably also use eResize. We want to do the minimum amount work that ensures nsSVGTextFrame2 gets reflowed. @@ +7346,5 @@ > + // some cached information that helps map between SVG text frames and > + // content nodes. We do a reflow since that's easier and shouldn't happen > + // too often to be too wasteful. > + for (nsIContent* parent = aContainer; parent; parent = parent->GetParent()) { > + nsIFrame* parentFrame = GetFrameFor(parent); Looping all the way up to the root seems wasteful for the usual case where no frame IsSVGText(). Why do we have to go up all the way to the root?
Assignee | ||
Comment 132•12 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•12 years ago
|
||
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•12 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•12 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•12 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•12 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•12 years ago
|
Attachment #649900 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #642687 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #649524 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #649520 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #649527 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #649948 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #649572 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #649575 -
Flags: checkin+
Assignee | ||
Comment 143•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/76835a9e3efb
Assignee | ||
Updated•12 years ago
|
Attachment #649069 -
Flags: checkin+
Comment 144•12 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #128) > (In reply to David Mathog from comment #122) > > Will this work resolve the baseline-shift issue (Bug 308338), which is > > still unresolved after 7 years? If yes, is than an estimate of when these > > changes will make it into production code? > > It will not make baseline-shift work, but it will make it easier to > implement. With these patches, internally dominant-baseline (the alignment > property we do currently support for SVG text) is mapped into an appropriate > vertical-align value when performing CSS text layout. We could do the same > thing for baseline-shift. Time frame? I started working on implementing baseline-shift, in a manner very similar to dominant-baseline, and then discovered this thread, so stopped work. No point modifying the current code if my changes will be immediately broken by these planned changes.
Comment 145•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fcde7b899e87 https://hg.mozilla.org/mozilla-central/rev/856faff0f587 https://hg.mozilla.org/mozilla-central/rev/a90b2757ccc2 https://hg.mozilla.org/mozilla-central/rev/dd0e5c739347 https://hg.mozilla.org/mozilla-central/rev/e45aca2a5d87 https://hg.mozilla.org/mozilla-central/rev/342718ea6734 https://hg.mozilla.org/mozilla-central/rev/5cd1bb53c262 https://hg.mozilla.org/mozilla-central/rev/9f477f32c157 https://hg.mozilla.org/mozilla-central/rev/76835a9e3efb
Comment 146•12 years ago
|
||
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•12 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•12 years ago
|
||
I think we need to handle xml:space="preserve" as it's currently defined in the SVG spec, then. This patch adds a -moz-pre-one-line value for white-space that implements this behaviour. (I can worry about whether we should add this to css4-text, or if the SVG spec should define this special newline discarding behaviour on top of CSS white space processing, later when I get to rewriting the SVG 2 Text chapter.)
Attachment #650459 -
Flags: review?(roc)
Comment on attachment 650459 [details] [diff] [review] Part 23a: Add white-space:-moz-pre-one-line value with white space collapsing behavior like SVG's xml:space="preserve". Review of attachment 650459 [details] [diff] [review]: ----------------------------------------------------------------- Seems fine to me, although I'm not sure about the name of the constant. I think NS_STYLE_WHITESPACE_PRE_DISCARD_NEWLINES might be better. Up to dbaron. ::: layout/generic/nsTextFrameUtils.cpp @@ +67,5 @@ > PRUnichar ch = *aText++; > if (IsDiscardable(ch, &flags)) { > aSkipChars->SkipChar(); > } else { > + if (ch == '\n' && aCompression == DISCARD_NEWLINE) { See below @@ +174,5 @@ > aSkipChars->SkipChar(); > } else { > + if (ch == '\n' && aCompression == DISCARD_NEWLINE) { > + aSkipChars->SkipChar(); > + continue; Instead of using "continue", just || this condition after IsDiscardable.
Attachment #650459 -
Flags: review?(roc) → review?(dbaron)
Comment on attachment 650459 [details] [diff] [review] Part 23a: Add white-space:-moz-pre-one-line value with white space collapsing behavior like SVG's xml:space="preserve". Asking fantasai for feedback on this, if only so that she's aware of it.
Attachment #650459 -
Flags: feedback?(fantasai.bugs)
Comment 152•12 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•12 years ago
|
||
This one's ready for review now. The diff is split up to make it easier to review the changes after moving; e.g. the first part that touches nsSVGUtils.cpp just moves all the functions over verbatim, and the second part fixes the functions up.
Attachment #640895 -
Attachment is obsolete: true
Attachment #650789 -
Flags: review?(jwatt)
Assignee | ||
Comment 154•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b32dbbd0df6f
Assignee | ||
Comment 155•12 years ago
|
||
This is needed so that nsSVGTextFrame2 get bounding boxes / covered regions of nsTextFrame children.
Attachment #650809 -
Flags: review?(jwatt)
Comment 156•12 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•12 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•12 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•12 years ago
|
||
nsSVGTextFrame2 doesn't implement nsSVGTextContainerFrame, so we need to go searching for the closest ancestor nsSVGTextFrame2 if we request a bbox for an ns{Block,Inline,Text}Frame instead.
Attachment #650816 -
Flags: review?(jwatt)
Comment 159•12 years ago
|
||
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•12 years ago
|
Attachment #650789 -
Flags: review?(jwatt) → review+
Updated•12 years ago
|
Attachment #650809 -
Flags: review?(jwatt) → review+
Updated•12 years ago
|
Attachment #650816 -
Flags: review?(jwatt) → review+
Assignee | ||
Comment 160•12 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.
Comment 161•12 years ago
|
||
(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•12 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•12 years ago
|
Attachment #650789 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #650809 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #650816 -
Flags: checkin+
Assignee | ||
Comment 163•12 years ago
|
||
We'll encounter NS_FRAME_IS_SVG_TEXT frames as we propagate SVG changes down the tree, so we shouldn't assert if we find them.
Attachment #651089 -
Flags: review?(jwatt)
Updated•12 years ago
|
Attachment #651089 -
Flags: review?(jwatt) → review+
Comment 164•12 years ago
|
||
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•12 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•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/8c95a0d692fa
Assignee | ||
Updated•12 years ago
|
Attachment #651089 -
Flags: checkin+
Assignee | ||
Comment 168•12 years ago
|
||
I've gone with the property value renaming that roc suggested.
Attachment #650459 -
Attachment is obsolete: true
Attachment #650459 -
Flags: review?(dbaron)
Attachment #654901 -
Flags: review?(dbaron)
Comment 169•12 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•12 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•12 years ago
|
||
Since switching to painting SVG with display lists, we need this check here too.
Attachment #655517 -
Flags: review?(roc)
Assignee | ||
Comment 172•12 years ago
|
||
This is how we will handle scaling text to handle very small or very large font sizes. nsSVGTextFrame2 will return a scaling factor to apply to the font-size of all of its text frames.
Attachment #655521 -
Flags: review?(roc)
Assignee | ||
Comment 173•12 years ago
|
||
Attachment #655523 -
Flags: review?(longsonr)
Assignee | ||
Comment 174•12 years ago
|
||
nsSVGTextFrame2s will handle their positioning themselves, like SVG geometry frames.
Attachment #655525 -
Flags: review?(jwatt)
Assignee | ||
Comment 175•12 years ago
|
||
This makes all of the SVG DOM methods on nsSVGText{Content,}Element switch their implementations to call into the nsSVGTextContainerFrame or nsSVGTextFrame2, depending on whether the "use CSS text frames" pref is set. The nsSVGTextFrame2 methods all take an nsIContent* argument for the particular <text>, <tspan>, etc. element that it was called on, since it is handling the text layout for all of its descendants. (See patch 31b at http://mcc.id.au/temp/bug-655877/ for the not yet cleaned up nsSVGTextFrame2, if you need to.)
Attachment #655526 -
Flags: review?(longsonr)
Updated•12 years ago
|
Attachment #655523 -
Flags: review?(longsonr) → review+
Comment 176•12 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.
Assignee | ||
Comment 178•12 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•12 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•12 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•12 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•12 years ago
|
||
I would need to do this on every open window (pres shell?), yes? Not really sure how to do that.
Comment 183•12 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•12 years ago
|
||
Or if not a property, a state bit that children inherit like NONDISPLAY_CHILD.
Updated•12 years ago
|
Attachment #655525 -
Flags: review?(jwatt) → review+
Assignee | ||
Comment 185•12 years ago
|
||
r?bz for the frame constructor changes, r?longsonr for the SVG parts. This replaces most NS_SVGTextCSSFramesEnabled() calls with a check of the relevant frame's state bits to see if it's NS_FRAME_IS_SVG_TEXT. Thus if you construct <text><tspan>...</tspan></text> with the pref on, then you will get new-style frames, and if you flip the pref and then say modify the text content of the <tspan>, you will still get new-style frames. The only time NS_SVGTextCSSFramesEnabled() is called to find the current value of the pref is in nsCSSFrameConstructor::FindSVGData for a top-level <text> element. Robert: the changes in nsSVGUtils.cpp and nsSVGOuterSVGFrame.cpp are on top of two patches I haven't yet uploaded (they're parts 35 and 36 in http://mcc.id.au/temp/bug-655877/ if you care to look).
Attachment #656320 -
Flags: review?(longsonr)
Attachment #656320 -
Flags: review?(bzbarsky)
Attachment #655521 -
Flags: review?(roc) → review+
Comment 186•12 years ago
|
||
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•12 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•12 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•12 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•12 years ago
|
||
Here's the big one. I haven't got it working with DLBI yet, but I suspect that'll be a small patch (and would be better as a separate patch anyway).
Attachment #657565 -
Flags: review?(jwatt)
Assignee | ||
Comment 191•12 years ago
|
||
r?roc for the nsInlineFrame parts, r?jwatt for the SVG parts. roc: Let me know if you think it makes sense to use a mutation events listener (now that we have one registered for element insertion/removal) for the attribute changes here, instead of modfiying nsInlineFrame itself.
Attachment #657566 -
Flags: review?(roc)
Attachment #657566 -
Flags: review?(jwatt)
Assignee | ||
Comment 192•12 years ago
|
||
Note that the "if (NS_SVGTextFramesEnabled())" check gets changed to "mFrame->IsSVGText()" by the later patch part 42a.
Attachment #657567 -
Flags: review?(longsonr)
Assignee | ||
Comment 193•12 years ago
|
||
I want to call this from both nsSVGForeignObjectFrame and nsSVGTextFrame2.
Attachment #657568 -
Flags: review?(longsonr)
Comment 194•12 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•12 years ago
|
Attachment #657567 -
Flags: review?(longsonr) → review+
Comment 195•12 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•12 years ago
|
||
r?roc for the layout/base/ and layout/generic/ changes, r?jwatt for the layout/svg/ changes (since he's reviewing the nsSVGTextFrame2 class patch). We make the nsFrame methods that do text selection call in to the SVG frames to transform a point to pretend that it is over the corresponding part of the nsTextFrame when it is over one of the rendered runs of the text. For example with: <svg> <text x="100 200" y="100">ABC</text> </svg> the mRect of the <text> is going to be something like: x: 6000 y: 6000 - ascentInAppUnits("ABC") width: 6000 + advanceInAppUnits("BC") height: ascentInAppUnits("ABC") + descentInAppUnits("ABC") But when the mouse is at say (201,100) user units, we need to pretend that the point in app units relative to the <svg> is x: 6000 + advanceInAppUnits("A") + 60 y: 6000 and not x: 12000 + 60 y: 6000 so that when selection happens the nsTextFrame thinks the point is over the "B".
Attachment #657630 -
Flags: review?(roc)
Attachment #657630 -
Flags: review?(jwatt)
Assignee | ||
Comment 197•12 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•12 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•12 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•12 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•12 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•12 years ago
|
||
Updated to: * changed TextRenderedRun::GetTransformFromRunUserSpaceToFrameAppUnits to GetTransformFromRunUserSpaceToFrameUserSpace * rename TextRenderedRun::GetFrameUserSpaceRect to GetRunUserSpaceRect * add a GetFrameUserSpaceRect that actually does that * add the ability to TextFrameIterator and TextRenderedRunIterator to limit their iterations to a frame subtree (rather than a content subtree) Those changes are needed due to fixes in later patches. I also addressed a couple of Robert Longson's comments.
Attachment #658703 -
Flags: review?(jwatt)
Assignee | ||
Updated•12 years ago
|
Attachment #658703 -
Attachment is patch: true
Assignee | ||
Updated•12 years ago
|
Attachment #657565 -
Attachment is obsolete: true
Attachment #657565 -
Flags: review?(jwatt)
Assignee | ||
Comment 209•12 years ago
|
||
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•12 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 | ||
Comment 212•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
Attachment #658759 -
Flags: review?(longsonr) → review+
Comment 214•12 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•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1ad5d65bf44 https://hg.mozilla.org/integration/mozilla-inbound/rev/e79271d0adfe
Assignee | ||
Comment 216•12 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+
Comment 219•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f1ad5d65bf44 https://hg.mozilla.org/mozilla-central/rev/e79271d0adfe
Assignee | ||
Comment 220•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d9b336d8b4b
Assignee | ||
Comment 225•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
Attachment #659088 -
Attachment is obsolete: true
Attachment #659088 -
Flags: review?(jwatt)
Assignee | ||
Comment 228•12 years ago
|
||
Some tests. Many of them are tests for combinations of anchoring, ltr/rtl/bidi text, and glyph positioning attributes.
Attachment #665268 -
Flags: review?(jwatt)
Updated•12 years ago
|
Attachment #659121 -
Flags: review?(smontagu) → review+
Assignee | ||
Comment 229•12 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•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6786b5f4ee4a
Assignee | ||
Comment 231•12 years ago
|
||
Backed out part 44, since it depends on the pref having landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/c1b8d9b1b7f0
Updated•12 years ago
|
Attachment #659106 -
Flags: review?(jwatt) → review+
Assignee | ||
Comment 232•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b313eadbbc11
Comment 233•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b313eadbbc11
Flags: in-testsuite+
Assignee | ||
Comment 234•12 years ago
|
||
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•12 years ago
|
||
Attachment #668693 -
Flags: review?(jwatt)
Assignee | ||
Comment 236•12 years ago
|
||
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•12 years ago
|
||
Create the pref, starting disabled.
Attachment #668699 -
Flags: review?(jwatt)
Assignee | ||
Comment 238•12 years ago
|
||
Attachment #668701 -
Flags: review?(jwatt)
Assignee | ||
Comment 239•12 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•12 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•12 years ago
|
||
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•12 years ago
|
||
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•12 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•12 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 249•12 years ago
|
||
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 250•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #668693 -
Flags: review?(jwatt) → review+
Updated•12 years ago
|
Attachment #668699 -
Flags: review?(jwatt) → review+
Comment 251•12 years ago
|
||
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 @@ +506,5 @@ > +{ > + if (IsSVGText()) { > + nsIFrame* svgTextFrame = > + nsLayoutUtils::GetClosestFrameOfType(GetParent(), > + nsGkAtoms::svgTextFrame2); GetClosestFrameOfType not necessary here either. ::: layout/svg/nsSVGEffects.cpp @@ +295,5 @@ > + NS_ASSERTION(text, "expected to find an ancestor nsSVGTextFrame2"); > + static_cast<nsSVGTextFrame2*>(text)->NotifyGlyphMetricsChange(); > + } else { > + NS_ASSERTION(aFrame->IsFrameOfType(nsIFrame::eSVG), "SVG frame expected"); > + if (aFrame->GetType() == nsGkAtoms::svgTextPathFrame) { Can we just assert |aFrame->GetType() == nsGkAtoms::svgTextPathFrame|? If not, add a comment noting why. @@ +305,1 @@ > class nsAsyncNotifyGlyphMetricsChange MOZ_FINAL : public nsIReflowCallback This class died in bug 779971. You'll need to adjust the nsChangeHint_UpdateTextPath handling code in nsCSSFrameConstructor.cpp. @@ +347,5 @@ > + // nsSVGTextPathProperty if this <textPath> were a descendant of the > + // target <path>. > + bool nowValid = TargetIsValid(); > + if (!mValid && !nowValid) { > + return; Add a comment before this // Just return if we were previously invalid, and are still invalid Also add a comment saying why we have to call NotifyGlyphMetricsChangeForTextPath the first time we find that we're invalid. ::: layout/svg/nsSVGEffects.h @@ +185,5 @@ > class nsSVGTextPathProperty : public nsSVGIDRenderingObserver { > public: > nsSVGTextPathProperty(nsIURI *aURI, nsIFrame *aFrame, bool aReferenceImage) > + : nsSVGIDRenderingObserver(aURI, aFrame, aReferenceImage), > + mValid(true) {} Comma at start of line. @@ +193,5 @@ > protected: > virtual void DoUpdate() MOZ_OVERRIDE; > + > +private: > + bool TargetIsValid(); Add: /** * Returns true if the target of the textPath is the frame of a 'path' element. */
Assignee | ||
Comment 252•12 years ago
|
||
I assume you wanted to look at this patch again with the comments addressed. The changes due to nsAsyncNotifyGlyphMetricsChange going away I've put in part 36; I'll upload a new one of those to you to look at too (the old one was already r=longsonr).
Attachment #668849 -
Attachment is obsolete: true
Attachment #668849 -
Flags: review?(jwatt)
Attachment #684277 -
Flags: review?(jwatt)
Assignee | ||
Comment 253•12 years ago
|
||
Attachment #657567 -
Attachment is obsolete: true
Attachment #684278 -
Flags: review?(jwatt)
Comment 254•11 years ago
|
||
Comment on attachment 665263 [details] [diff] [review] Part 31b: New frame class nsSVGTextFrame2 for SVG text using CSS block/inline/text frames for layout. (v1.2) Review of attachment 665263 [details] [diff] [review]: ----------------------------------------------------------------- Meta question - how are you handling style changes for 'tspan' elements? ::: layout/svg/nsSVGTextFrame2.cpp @@ +175,5 @@ > + } > +} > + > +/** > + * Intersects an interal as IntersectInterval does but by taking s/interal/interval/ @@ +201,5 @@ > + return aContent; > +} > + > +/** > + * Returns whether the given node is a text content element, taking into Need to clarify what "text content element" is here. (Link to the spec text?) Mention tref? Note that <text> in <text> is not allowed per the spec. @@ +396,5 @@ > +// TextRenderedRun > + > +/** > + * A run of text within a single nsTextFrame whose glyphs can all be painted > + * at once. at once... for? It would be good to clarify that you mean in one call to nsTextFrame::PaintText, and that that means the text rendering run must contain characters all with the same rotation, etc. @@ +415,5 @@ > + > + /** > + * Constructs a TextRenderedRun with all of the information required to > + * paint it. See the member variable declarations below for descriptions > + * of the arguments. Add something like "See the comments documenting the members below for info on the arguments." @@ +428,5 @@ > + mFontSizeScaleFactor(aFontSizeScaleFactor), > + mBaseline(aBaseline), > + mContentOffset(aContentOffset), > + mContentLength(aContentLength), > + mCharIndex(aCharIndex) Maybe mTextFrameOffset, mTextFrameLength and mTextElementIndex? I think that would make it much more immediately obvious what these are about. @@ +451,5 @@ > + } > + > + /** > + * Returns the transform that converts from a <text> element's user space into > + * the coordinate space that rendered runs can be painted directly in. It would be great to have an ASCII art diagram explaining where the origin of the paint space is for ltr and rtl text. Or maybe we should have a nice long comment at the top of the header file with various diagrams explaining the various spaces. @@ +467,5 @@ > + /** > + * Returns the transform that converts from "run user space" to a <text> > + * element's user space. Run user space is a coordinate system that has the > + * same size as the <text>'s user space but rotated and translated such that > + * (0,0) is the top-left of the rectangle that covers the text. s/covers/bounds/? @@ +496,5 @@ > + }; > + > + /** > + * Returns a rectangle that covers the fill and/or stroke of the rendered run > + * in run user space. s/covers/bounds/ @@ +507,5 @@ > + gfxRect GetRunUserSpaceRect(nsPresContext* aContext, uint32_t aFlags) const; > + > + /** > + * Returns a rectangle that covers the fill and/or stroke of the rendered run > + * in frame user space. It would be good to clarify here that it's the nsTextFrame's space that we're talking about here. @@ +534,5 @@ > + /** > + * Gets the app unit amounts to clip from the left and right edges of > + * the nsTextFrame in order to paint just this rendered run. > + */ > + void GetClipEdges(nscoord& aLeftEdge, nscoord& aRightEdge) const; Can you clarify here the behavior of nsTextFrame clipping - specifically that if the clip lands in the middle of a glyph, that glyph won't be painted. @@ +1043,5 @@ > +/** > + * TextNodeCorrespondence is used as the value of a frame property that > + * is stored on all its descendant nsTextFrames. It stores the number of DOM > + * characters between it and the previous nsTextFrame that are not within a > + * valid text content element. These are called "undisplayed characters". Note that we're specifically talking about elements that contain text but don't create a frame: |display:none| and invalid <text> in <text>? @@ +1843,5 @@ > + enum CharacterFilter { > + // Iterate over all original characters from the DOM that are within valid > + // text content elements. > + eOriginal, > + // Iterate only over characters that are that are not skipped per the s/that are that are/that are/ @@ +2583,5 @@ > + virtual ~nsDisplaySVGText() { > + MOZ_COUNT_DTOR(nsDisplaySVGText); > + } > +#endif > + Remove trailing white space. @@ +2664,5 @@ > +nsSVGTextFrame2::BuildDisplayList(nsDisplayListBuilder* aBuilder, > + const nsRect& aDirtyRect, > + const nsDisplayListSet& aLists) > +{ > + UpdateGlyphPositioning(true); This call should not be necessary since reflow should have happened and taken care of updating the glyph positioning. Change this to an assertion. @@ +2672,5 @@ > + > +NS_IMETHODIMP > +nsSVGTextFrame2::AttributeChanged(int32_t aNameSpaceID, > + nsIAtom* aAttribute, > + int32_t aModType) Grr to the white space. ;) @@ +2678,5 @@ > + if (aNameSpaceID != kNameSpaceID_None) > + return NS_OK; > + > + if (aAttribute == nsGkAtoms::transform) { > + NotifySVGChanged(TRANSFORM_CHANGED); Not sure this is doing the right thing - need to come back to this. You need to reflow to handle the text scaling thing at least. @@ +2706,5 @@ > + if (!(kid->GetStateBits() & NS_FRAME_FIRST_REFLOW)) { > + nsSVGOuterSVGFrame* outer = nsSVGUtils::GetOuterSVGFrame(this); > + if (!(outer->GetStateBits() & NS_FRAME_IN_REFLOW)) { > + NotifyGlyphMetricsChange(); > + RequestReflow(nsIPresShell::eStyleChange, NS_FRAME_IS_DIRTY); Pretty sure this RequestReflow is redundant. @@ +2713,5 @@ > + } > +} > + > +NS_IMETHODIMP > +nsSVGTextFrame2::AttributeChanged(int32_t aNameSpaceID, This can die, since you already defined this above. @@ +2740,5 @@ > + nsIContent *aFirstNewContent, > + int32_t aNewIndexInContainer) > +{ > + mFrame->NotifyGlyphMetricsChange(); > + mFrame->RequestReflow(nsIPresShell::eResize, NS_FRAME_HAS_DIRTY_CHILDREN); eResize is wrong. Pretty sure this RequestReflow is redundant, if you can make sure that NotifyGlyphMetricsChange passes @@ +2750,5 @@ > + nsIContent *aChild, > + int32_t aIndexInContainer) > +{ > + mFrame->NotifyGlyphMetricsChange(); > + mFrame->RequestReflow(nsIPresShell::eResize, NS_FRAME_HAS_DIRTY_CHILDREN); Redundant? @@ +2761,5 @@ > + int32_t aIndexInContainer, > + nsIContent *aPreviousSibling) > +{ > + mFrame->NotifyGlyphMetricsChange(); > + mFrame->RequestReflow(nsIPresShell::eResize, NS_FRAME_HAS_DIRTY_CHILDREN); Redundant? @@ +2913,5 @@ > + return NS_OK; > + > + AutoCanvasTMForMarker autoCanvasTMFor(this, FOR_PAINTING); > + > + UpdateGlyphPositioning(true); Reflow should have made sure that this call has happened. Just assert that we don't need to call UpdateGlyphPositioning here. @@ +3116,5 @@ > + uint32_t aFlags) > +{ > + NS_ASSERTION(GetFirstPrincipalChild(), "must have a child frame"); > + > + UpdateGlyphPositioning(true); I think for DOM calls like this we should do: if (!(mState & NS_STATE_SVG_NONDISPLAY_CHILD)) { ReflowSVG(); } else { UpdateGlyphPositioning(true); } @@ +3225,5 @@ > + */ > +uint32_t > +nsSVGTextFrame2::GetNumberOfChars(nsIContent* aContent) > +{ > + UpdateGlyphPositioning(false); Unnecessary, I'd think. @@ +3710,5 @@ > + } > + > + // We assume the first character position is (0,0) unless we later see > + // otherwise, and note it as unaddressable if it is. > + mPositions.AppendElement(CharPosition()); Start off with an unspecified value and fill it in later in DoGlyphPositioning. @@ +4269,5 @@ > +nsSVGTextFrame2::NotifyGlyphMetricsChange() > +{ > + mPositioningDirty = true; > + nsSVGUtils::InvalidateBounds(this, false); > + nsSVGUtils::InvalidateAndScheduleReflowSVG(this); Change the InvalidateAndScheduleReflowSVG to ScheduleReflowSVG. (I'm killing InvalidateAndScheduleReflowSVG in another patch elsewhere.) @@ +4334,5 @@ > + TextNodeCorrespondenceRecorder::RecordCorrespondence(this); > +} > + > +void > +nsSVGTextFrame2::RequestReflow(nsIPresShell::IntrinsicDirty aType, uint32_t aBit) This can die? ::: layout/svg/nsSVGTextFrame2.h @@ +38,5 @@ > + mHidden(false), > + mUnaddressable(false), > + mClusterOrLigatureGroupMiddle(false), > + mRunBoundary(false), > + mStartOfChunk(false) Can you use the style: CharPosition() : mAngle(0) , mHidden(false) etc. Here and elsewhere. @@ +60,5 @@ > + cp.mUnaddressable = aUnaddressable; > + return cp; > + } > + > + bool IsAngleSpecified() const Rename to "HasExplicitRotate", I'd think. Also add a comment along the lines of "Only valid after nsSVGTextFrame2::ResolvePositions() has been called, and before nsSVGTextFrame2::DoGlyphPosition() has set the position and orientations the xxx phase and before the yyy phase". Returns true if this character has an explicit rotation from a value in a 'rotate' attribute. Only used by nsSVGTextFrame2::DoGlyphPositioning while calculating and setting the rotation of a list of CharPosition objects. Before nsSVGTextFrame2::ResolvePositions() has been called this will always return true since all CharPosition objects start off as unspecified. After nsSVGTextFrame2::DoGlyphPositioning() has calculated and assigned positions and orientations to any CharPosition objects without explicit values this will always return true. @@ +65,5 @@ > + { > + return mAngle != UnspecifiedAngle(); > + } > + > + bool IsXSpecified() const HasExplicitX @@ +70,5 @@ > + { > + return mPosition.x != UnspecifiedCoord(); > + } > + > + bool IsYSpecified() const HasExplicitY @@ +74,5 @@ > + bool IsYSpecified() const > + { > + return mPosition.y != UnspecifiedCoord(); > + } > + Can you put all the members right at the end of class/struct definitions? @@ +75,5 @@ > + { > + return mPosition.y != UnspecifiedCoord(); > + } > + > + gfxPoint mPosition; Append "// user units" to this line. Add a comment before this line, like "Once complete, this value will be the final position at which the glyph corresponding to this character should be painted (i.e. after taking into account x/y/dx/dy attributes, bidi reordering, text-anchor adjustment, etc.). If this character corresponds to the middle of a cluster or ligature group, then this is the position of that part of the cluster/ligature." @@ +76,5 @@ > + return mPosition.y != UnspecifiedCoord(); > + } > + > + gfxPoint mPosition; > + double mAngle; Append a "// [angular unit]" to this line. @@ +78,5 @@ > + > + gfxPoint mPosition; > + double mAngle; > + > + // not displayed due to falling off the end of a <textPath> Can you start comments with an uppercase letter, please? @@ +82,5 @@ > + // not displayed due to falling off the end of a <textPath> > + bool mHidden; > + > + // skipped in positioning attributes due to being collapsed-away white space > + bool mUnaddressable; Make this comment "True if this character is ignored (due to being collapsed-away white space) when associating the list of values in x/y/dx/dy positioning attributes with a list of CharPosition objects." @@ +84,5 @@ > + > + // skipped in positioning attributes due to being collapsed-away white space > + bool mUnaddressable; > + > + // a preceding character is what positioning attributes address This makes it sound like positioning attribute values are applied to later characters, rather than being ignored. Maybe "This character is associated with the latter (non-first) part of a cluster or ligature, which means that any positioning values from x/y/dx/dy attributes that are associated with this character are ignored." @@ +87,5 @@ > + > + // a preceding character is what positioning attributes address > + bool mClusterOrLigatureGroupMiddle; > + > + // rendering is split here since an explicit position or rotation was given s/was given/ from a x/y/rotate attribute is associated with this character/ @@ +90,5 @@ > + > + // rendering is split here since an explicit position or rotation was given > + bool mRunBoundary; > + > + // an anchored chunk begins here Maybe "The first character in an anchored chunk of text (a piece of text that is adjusted as a unit when accounting for the text-anchor property)." @@ +116,5 @@ > +/** > + * Frame class for SVG <text> elements, used when the > + * layout.svg.css-text.enabled is true. > + * > + * An nsSVGTextFrame2 manages SVG text layout, painting and interaction for s/interaction/hit-testing/ @@ +121,5 @@ > + * all descendent text content elements. The frame tree will look like this: > + * > + * nsSVGTextFrame2 -- for <text> > + * <anonymous block frame> > + * ns{Block,Inline,Text}Frames -- for text nodes, <tspan>s, <a>s, etc. Not nsBlockFrame? Add assertions for that? @@ +123,5 @@ > + * nsSVGTextFrame2 -- for <text> > + * <anonymous block frame> > + * ns{Block,Inline,Text}Frames -- for text nodes, <tspan>s, <a>s, etc. > + * > + * SVG text layout is done by: s/SVG text layout is done by/During SVG text layout we create a CharPosition array and fill out its members by/. (Or otherwise mention that it's CharPosition array that we're working to populate during this process.) Make sure to add an XXX comment here for the follow-up changes that you'll be making in light of our discussion with fantasai and Simon. @@ +181,5 @@ > + NS_IMETHOD AttributeChanged(int32_t aNameSpaceID, > + nsIAtom* aAttribute, > + int32_t aModType); > + > + virtual void DidSetStyleContext(nsStyleContext* aOldStyleContext) MOZ_OVERRIDE; (You could add MOZ_OVERRIDE to the other relevant methods in this new class if you can be bothered.) @@ +192,5 @@ > + { > + return GetFirstPrincipalChild()->GetContentInsertionFrame(); > + } > + > + NS_IMETHOD Reflow(nsPresContext* aPresContext, This cannot be called unless something crazy is going on. Run your patch over the test suite with an NS_NOTREACHED in the implementation of this method just to be sure, then remove this override. As a result you can also move the content of DoReflow into ReflowSVG and get rid of DoReflow. @@ +215,5 @@ > + return MakeFrameName(NS_LITERAL_STRING("SVGText2"), aResult); > + } > +#endif > + > + virtual void InvalidateInternal(const nsRect& aDamageRect, I think DLBI killed this - remove. @@ +230,5 @@ > + uint32_t aFlags); > + > + // nsSVGContainerFrame methods: > + virtual gfxMatrix GetCanvasTM(uint32_t aFor); > + Delete white space. @@ +249,5 @@ > + > + // nsSVGTextFrame2 methods: > + > + /** > + * Schedules mPositions to be recomputed and the covered region to be "covered regions" as a concept has been dead for a long time. I think you can just rename this to "ScheduleReflow" and change the comment to "Schedules a reflow (which will also update the mPositions array)".
Comment 255•11 years ago
|
||
>
> @@ +3225,5 @@
> > + */
> > +uint32_t
> > +nsSVGTextFrame2::GetNumberOfChars(nsIContent* aContent)
> > +{
> > + UpdateGlyphPositioning(false);
>
> Unnecessary, I'd think.
I'd have thought you would still need this for text in <defs> e.g. text that's a child of a clipPath. In that case this may be the first time that the text is positioned at all. This is certainly true of the code as it is now.
Comment 256•11 years ago
|
||
This may be a little off topic for this thread, but since the same code sections are probably involved... The attached image was created in Inkscape. It uses 18 pt Arial text. In the svg everything is in px, so 22.5px font, as inkscape is 90dpi internally. On each line a red boundary box was drawn manually from the leading edge of the first period to the trailing edge of the last "A". When this SVG is viewed in Firefox the text is slightly wider than the boundary, but the height is correct. So inkscape is apparently doing something different with this text than firefox is, otherwise the text would be exactly in the box in Firefox too. One possibility is that the browser is using some font other than Arial, which is a bit wider (but the same height). Is there some way to make the unmodified browser cough up this information? If this is the case, is there something that can be added to SVG to force it to use the specified font? Or is something else entirely going on? Thank you.
Comment 257•11 years ago
|
||
(In reply to David Mathog from comment #256) > Created attachment 688347 [details] > Text rendering incompatibility: Inkscape vs. Firefox > > This may be a little off topic for this thread, but since the same code > sections are probably involved... > > The attached image was created in Inkscape. It uses 18 pt Arial text. In > the svg everything is in px, so 22.5px font, as inkscape is 90dpi > internally. On each line a red boundary box was drawn manually from the > leading edge of the first period to the trailing edge of the last "A". When > this SVG is viewed in Firefox the text is slightly wider than the boundary, > but the height is correct. > > So inkscape is apparently doing something different with this text than > firefox is, otherwise the text would be exactly in the box in Firefox too. > > One possibility is that the browser is using some font other than Arial, > which is a bit wider (but the same height). Is there some way to make the > unmodified browser cough up this information? If this is the case, is there > something that can be added to SVG to force it to use the specified font? > > Or is something else entirely going on? > > Thank you. It is off topic. Can you create your own bug for this please?
Updated•11 years ago
|
Attachment #688347 -
Attachment is obsolete: true
Comment 258•11 years ago
|
||
(In reply to Robert Longson from comment #255) > > > > @@ +3225,5 @@ > > > + */ > > > +uint32_t > > > +nsSVGTextFrame2::GetNumberOfChars(nsIContent* aContent) > > > +{ > > > + UpdateGlyphPositioning(false); > > > > Unnecessary, I'd think. > > I'd have thought you would still need this for text in <defs> e.g. text > that's a child of a clipPath. In that case this may be the first time that > the text is positioned at all. This is certainly true of the code as it is > now. Counting the number of chars doesn't depend on positioning them though, right? (The comment is specifically for GetNumberOfChars.)
Comment 259•11 years ago
|
||
(In reply to Jonathan Watt [:jwatt] (offline Thu-Sun) from comment #258) > Counting the number of chars doesn't depend on positioning them though, > right? (The comment is specifically for GetNumberOfChars.) We currently do whitespace handling there as part of working out positioning. At the moment we take that into account which may of course be the wrong thing to do. i.e. we'd say that " abc" was 3 characters if we're compressing whitespace since that matches what we're going to display.
Assignee | ||
Comment 260•11 years ago
|
||
This what the spec says currently: Returns the total number of characters available for rendering within the current element, which includes referenced characters from ‘tref’ reference, regardless of whether they will be rendered. Effectively, this is equivalent to the length of the Node::textContent attribute from DOM4 ([DOM4], section 5.3), if that attribute also expanded ‘tref’ elements. My patch skips invalid elements, so I guess that doesn't match what the spec says there, but it is closer than what we currently have. All of my SVG DOM text method implementations use "actual DOM character indexes except for invalid elements" for character indexes, including getNumberOfChars. It looks like all the current text methods work on the post white space collapsed indexes? That at least would be consistent with the counting you do for the x/y/dx/dy positioning attributes, which also skip over collapsed white space. My current patch doesn't need UpdateGlyphPositioning, since it just looks over the nsTextNodes in the tree, but if I follow our current behaviour, it would need to, as I'd be using CharIterator, which inspects the nsTextFrames.
Comment 261•11 years ago
|
||
Yes, as I implemented it I made everything work after white-space processing. It does sound like what your implementing to is closer to what we should do though so we should go with that if we can.
Assignee | ||
Comment 262•11 years ago
|
||
(In reply to Jonathan Watt [:jwatt] (offline Thu-Sun) from comment #254) > Meta question - how are you handling style changes for 'tspan' elements? If for example the fill of a tspan changes, it will cause nsInlineFrame::InvalidateFrame to be called, and that now checks if IsSVGText(), and if so calls InvalidateFrame on the nsSVGTextFrame2. If the style change causes a reflow, then that will just eventually cause nsSVGOuterSVGFrame::Reflow to call our ReflowSVG.
Assignee | ||
Comment 263•11 years ago
|
||
Comment on attachment 665263 [details] [diff] [review] Part 31b: New frame class nsSVGTextFrame2 for SVG text using CSS block/inline/text frames for layout. (v1.2) Review of attachment 665263 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/svg/nsSVGTextFrame2.cpp @@ +428,5 @@ > + mFontSizeScaleFactor(aFontSizeScaleFactor), > + mBaseline(aBaseline), > + mContentOffset(aContentOffset), > + mContentLength(aContentLength), > + mCharIndex(aCharIndex) I'll go for mTextElementContentOffset, mTextElementContentLength and mTextElementCharIndex. I think it's important to keep "ContentOffset" and "ContentLength" in the names since that's what those are called in nsTextFrame. @@ +451,5 @@ > + } > + > + /** > + * Returns the transform that converts from a <text> element's user space into > + * the coordinate space that rendered runs can be painted directly in. I added a more substantial discussion with some examples, but didn't attempt ASCII art. @@ +507,5 @@ > + gfxRect GetRunUserSpaceRect(nsPresContext* aContext, uint32_t aFlags) const; > + > + /** > + * Returns a rectangle that covers the fill and/or stroke of the rendered run > + * in frame user space. I've added a definition of "frame user space" here too. @@ +2678,5 @@ > + if (aNameSpaceID != kNameSpaceID_None) > + return NS_OK; > + > + if (aAttribute == nsGkAtoms::transform) { > + NotifySVGChanged(TRANSFORM_CHANGED); Yes, it seems like we should be setting needGlyphMetricsUpdate = true in NotifySVGChanged when TRANSFORM_CHANGED, but we're not. (There are some other probably HiDPI related problems with the text scaling thing I'm finding locally, which I'll also need to fix.) @@ +2706,5 @@ > + if (!(kid->GetStateBits() & NS_FRAME_FIRST_REFLOW)) { > + nsSVGOuterSVGFrame* outer = nsSVGUtils::GetOuterSVGFrame(this); > + if (!(outer->GetStateBits() & NS_FRAME_IN_REFLOW)) { > + NotifyGlyphMetricsChange(); > + RequestReflow(nsIPresShell::eStyleChange, NS_FRAME_IS_DIRTY); Per discussions F2F, I removed the whole DidSetStyleContext and nothing broke. @@ +2740,5 @@ > + nsIContent *aFirstNewContent, > + int32_t aNewIndexInContainer) > +{ > + mFrame->NotifyGlyphMetricsChange(); > + mFrame->RequestReflow(nsIPresShell::eResize, NS_FRAME_HAS_DIRTY_CHILDREN); Tests all passed without the RequestReflow calls, so I've removed them. @@ +2913,5 @@ > + return NS_OK; > + > + AutoCanvasTMForMarker autoCanvasTMFor(this, FOR_PAINTING); > + > + UpdateGlyphPositioning(true); I've kept the UpdateGlyphPositioning call but only when mState & NS_STATE_SVG_NONDISPLAY_CHILD, since we need to update for each use of the <text> inside <mask>s etc.
Assignee | ||
Comment 264•11 years ago
|
||
(That didn't seem to keep the context of your questions, but maybe it looks better through Splinter.)
Assignee | ||
Comment 265•11 years ago
|
||
This was another thing we discussed irl.
Attachment #692132 -
Flags: review?(jwatt)
Assignee | ||
Comment 266•11 years ago
|
||
Address comments so far.
Attachment #692133 -
Flags: review?(jwatt)
Assignee | ||
Updated•11 years ago
|
Attachment #665263 -
Attachment is obsolete: true
Attachment #665263 -
Flags: review?(jwatt)
Updated•11 years ago
|
Attachment #692132 -
Flags: review?(jwatt) → review+
Comment 267•11 years ago
|
||
Comment on attachment 684278 [details] [diff] [review] Part 36: Make referenced text path updates cause SVG text relayout. (v1.1) >+ if (aFrame->GetType() == nsGkAtoms::svgTextPathFrame) { >+ // Invalidate and reflow the entire nsSVGTextFrame: >+ static_cast<nsSVGTextPathFrame*>(aFrame)->NotifyGlyphMetricsChange(); >+ } else if (aFrame->IsSVGText()) { >+ // Invalidate and reflow the entire nsSVGTextFrame2: >+ nsIFrame* text = nsLayoutUtils::GetClosestFrameOfType( >+ aFrame, >+ nsGkAtoms::svgTextFrame2); >+ NS_ASSERTION(text, "expected to find an ancestor nsSVGTextFrame2"); >+ static_cast<nsSVGTextFrame2*>(text)->NotifyGlyphMetricsChange(); >+ } else { If I understand correctly, the nsGkAtoms::svgTextPathFrame case is for when nsSVGTextFrame2 is disabled, yes? In the IsSVGText case, can you add an assertion that aFrame->GetContent()->Tag == nsGkAtoms::textPath?
Assignee | ||
Comment 268•11 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #267) > If I understand correctly, the nsGkAtoms::svgTextPathFrame case is for when > nsSVGTextFrame2 is disabled, yes? Yes that's right. > In the IsSVGText case, can you add an assertion that > aFrame->GetContent()->Tag == nsGkAtoms::textPath? Even better, I will assert aFrame->GetContent()->IsSVG(nsGkAtoms::textPath).
Assignee | ||
Comment 269•11 years ago
|
||
Or maybe I should just check it in the "else if", since it seems the NS_ABORT_IF_FALSE just below that should catch any case where we're sending the hint incorrectly?
Comment 270•11 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #269) > Or maybe I should just check it in the "else if", since it seems the > NS_ABORT_IF_FALSE just below that should catch any case where we're sending > the hint incorrectly? Yes, that's what I meant, in the |else if (aFrame->IsSVGText())|.
Comment 271•11 years ago
|
||
Comment on attachment 684278 [details] [diff] [review] Part 36: Make referenced text path updates cause SVG text relayout. (v1.1) r=me with that.
Attachment #684278 -
Flags: review?(jwatt) → review+
Comment 272•11 years ago
|
||
Comment on attachment 692133 [details] [diff] [review] Part 31b: New frame class nsSVGTextFrame2 for SVG text using CSS block/inline/text frames for layout. (v1.3) [includes part 31e] r=me.
Attachment #692133 -
Flags: review?(jwatt) → review+
Comment 273•11 years ago
|
||
In part 41b > nsRect > nsLayoutUtils::TransformAncestorRectToFrame(nsIFrame* aFrame, > const nsRect &aRect, > const nsIFrame* aAncestor) > { ... > + > + if (text && false) { > + result = TransformGfxRectFromAncestor(text, result, aAncestor); > + result = text->TransformFrameRectToTextChild(result, aFrame); > + } else { > result = TransformGfxRectFromAncestor(aFrame, result, aAncestor); > + } Because of the false the else will always get called. Why not just have the else line?
Updated•11 years ago
|
Attachment #665265 -
Flags: review?(jwatt) → review+
Updated•11 years ago
|
Attachment #684277 -
Flags: review?(jwatt) → review+
Comment 274•11 years ago
|
||
Comment on attachment 665268 [details] [diff] [review] Part 43: Tests for new SVG text support. Mostly just a rubber stamp r+ on the tests to be honest. The coverage looks good, but I haven't considered each individual test in detail to consider whether it's doing the right thing. I'm going to trust that you did. :)
Attachment #665268 -
Flags: review?(jwatt) → review+
Updated•11 years ago
|
Attachment #668701 -
Flags: review?(jwatt) → review+
Comment 275•11 years ago
|
||
Remember to address comment 273 (I'm guessing you have already).
Assignee | ||
Comment 276•11 years ago
|
||
Thanks for the reviews! I will land the patches soon. I'm not going to enable the pref yet since there are text layout problems on HiDPI that I haven't fixed yet. (The changes to make due to the discussions we had with fantasai and smontagu I think don't need to wait for the feature to be enabled, but obviously still need to be done.)
Assignee | ||
Updated•11 years ago
|
Attachment #692133 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #692132 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #684278 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #684277 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #668890 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #668701 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #668699 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #668693 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #665268 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #668692 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #665265 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #665264 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #659121 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #659106 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #658759 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #658758 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #657568 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #656320 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #655526 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #655525 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #655523 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #655521 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #654901 -
Flags: checkin+
Assignee | ||
Comment 277•11 years ago
|
||
Sorry for all the bugspam; I couldn't seem to modify the checkin flag on the remaining attachments in one go, and I couldn't bear to leave them inconsistent. Let's open a new bug for preffing this stuff on, so we can have a couple of dependent bugs for it. https://hg.mozilla.org/integration/mozilla-inbound/rev/102258597a1c https://hg.mozilla.org/integration/mozilla-inbound/rev/9642172270f1 https://hg.mozilla.org/integration/mozilla-inbound/rev/76dd3bdc30df https://hg.mozilla.org/integration/mozilla-inbound/rev/53005a015407 https://hg.mozilla.org/integration/mozilla-inbound/rev/0e81334f73d1 https://hg.mozilla.org/integration/mozilla-inbound/rev/9bc717b46687 https://hg.mozilla.org/integration/mozilla-inbound/rev/37efcc78e70e https://hg.mozilla.org/integration/mozilla-inbound/rev/22b95038354b https://hg.mozilla.org/integration/mozilla-inbound/rev/ddf482186017 https://hg.mozilla.org/integration/mozilla-inbound/rev/c55fb05e73d6 https://hg.mozilla.org/integration/mozilla-inbound/rev/204b51a6c645 https://hg.mozilla.org/integration/mozilla-inbound/rev/d11ce45846b5 https://hg.mozilla.org/integration/mozilla-inbound/rev/230087dd7d3b https://hg.mozilla.org/integration/mozilla-inbound/rev/665c760a078b https://hg.mozilla.org/integration/mozilla-inbound/rev/dc9b408b8b78 https://hg.mozilla.org/integration/mozilla-inbound/rev/6d9ca0891b4b https://hg.mozilla.org/integration/mozilla-inbound/rev/03cd2973dcff
Whiteboard: [leave open]
Comment 278•11 years ago
|
||
This caused permaorange of the unexpected-pass variety on WinXP: REFTEST TEST-UNEXPECTED-PASS | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/bugs/580863-1.html | image comparison (==) I've removed the fails-if (which was added by bug 623454) to green up the tree: https://hg.mozilla.org/integration/mozilla-inbound/rev/1df0b4b3cd55
Comment 279•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/102258597a1c https://hg.mozilla.org/mozilla-central/rev/9642172270f1 https://hg.mozilla.org/mozilla-central/rev/76dd3bdc30df https://hg.mozilla.org/mozilla-central/rev/53005a015407 https://hg.mozilla.org/mozilla-central/rev/0e81334f73d1 https://hg.mozilla.org/mozilla-central/rev/9bc717b46687 https://hg.mozilla.org/mozilla-central/rev/37efcc78e70e https://hg.mozilla.org/mozilla-central/rev/22b95038354b https://hg.mozilla.org/mozilla-central/rev/ddf482186017 https://hg.mozilla.org/mozilla-central/rev/c55fb05e73d6 https://hg.mozilla.org/mozilla-central/rev/204b51a6c645 https://hg.mozilla.org/mozilla-central/rev/d11ce45846b5 https://hg.mozilla.org/mozilla-central/rev/230087dd7d3b https://hg.mozilla.org/mozilla-central/rev/665c760a078b https://hg.mozilla.org/mozilla-central/rev/dc9b408b8b78 https://hg.mozilla.org/mozilla-central/rev/6d9ca0891b4b https://hg.mozilla.org/mozilla-central/rev/03cd2973dcff https://hg.mozilla.org/mozilla-central/rev/1df0b4b3cd55
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Assignee | ||
Comment 280•11 years ago
|
||
(In reply to Ed Morley [:edmorley UTC+0] from comment #278) > I've removed the fails-if (which was added by bug 623454) to green up the > tree: > https://hg.mozilla.org/integration/mozilla-inbound/rev/1df0b4b3cd55 Thanks Ed. I didn't intend to fix that, and I'm not sure what of my patches would have made that test pass now.
Assignee | ||
Updated•11 years ago
|
Alias: svgtext
Updated•11 years ago
|
Comment 281•11 years ago
|
||
This will be preffed on in bug 839955.
Comment 282•11 years ago
|
||
When can we expect it to land in a stable firefox? Version 23.0.1 does not support underline in svg, so i assume it do not have this code already. Thanks for this important code!
Comment 283•11 years ago
|
||
See comment 281. This is enabled in Firefox 25.
You need to log in
before you can comment on or make changes to this bug.
Description
•