See this profile for example: <https://perfht.ml/2lr1FhO> STR: Load <https://docs.google.com/presentation/d/1gqQpeG1vHky4nZ45mIYSC5DAEFX-5HipjemLwhBDe1c/> (requires LDAP), wait for all of the thumbnails to load, then select the first thumbnail and keep the down key pressed.

So the issue is SVGTextContentElement::GetSubStringLength() invokes SVGTextContentElement::GetSVGTextFrame() which flushes the layout. Looking at the code, I don't think GetSubStringLength() really needs a layout flush. I think a frame flush would be enough, so it should call GetSVGTextFrameForNonLayoutDependentQuery() instead. Actually I suspect that all methods below GetSubStringLength() also don't need layout flush to work correctly. They should probably be investigated together. Moving to SVG since it seems to be a more relevant component.

I don't think we can skip layout, since GetSubStringLength() needs to look at the text runs to accumulate glyph advances, and the text runs are created during reflow.

Text runs are not necessarily created during reflow. Text runs are created lazily when requested. nsTextFrame::EnsureTextRun would ensure text run to be properly constructed. Constructing text runs is part of text frame reflow, but it can also be called when calculating intrinsic isize, which indicates that it doesn't depend on reflow, but reflow depends on it.

And the advantage of constructing text runs, if that's safe, is that it can be done for a leaf part of the tree, rather than starting at the root.

I think it is safe to construct text run without reflow, but ni? jfkthame in case I miss something.

To Jonathan for a look.

As Xidorn said in comment 5, it should be OK to construct textruns independently of reflow. nsTextFrame::EnsureTextRun() will construct the textrun for a frame if it doesn't already exist, regardless of whether we're in reflow at the time. However, that's not sufficient for methods like GetSubStringLength(), because it will call through to SVGTextFrame::DoGlyphPositioning(), which in turn expects its child to have been reflowed. Avoiding that doesn't look at all straightforward... :jwatt, do you have any sense of whether it's feasible to support GetSubStringLength() for un-reflowed frames, where we'd be able to iterate through the textruns but not have access to actual frame position/dimensions?

(In reply to Jonathan Kew (:jfkthame) from comment #7) > :jwatt, do you have any sense of whether it's feasible to support > GetSubStringLength() for un-reflowed frames, where we'd be able to iterate > through the textruns but not have access to actual frame position/dimensions? I had thought that's not possible in the general case since glyph positions depend on the 'x'/'y' attributes which can contain percentage values which resolve against the dimensions of the SVG viewport, and of course resolving the dimensions of the SVG viewport can require reflowing the nearest reflow root (likely the root of the document tree). However, it seems that the SVG 2 draft has removed the requirement that this method include 'x'/'y' attribute offsets: https://svgwg.org/svg2-draft/text.html#__svg__SVGTextContentElement__getSubStringLength There is a requirement that the values of the 'letter-spacing' and 'word-spacing' properties are included in the computation, but those are a lot simpler to account for and are much, much less likely to be set to percentage values.

Actually, neither we nor Chrome currently account for letter or word spacing in getSubStringLength, so we can currently ignore that too.

(In reply to Jonathan Watt [:jwatt] from comment #9) > Actually, neither we nor Chrome currently account for letter or word spacing > in getSubStringLength, so we can currently ignore that too. We might if we ever implemented support for them though: bug 371787

(In reply to Jonathan Watt [:jwatt] from comment #8) > There is a requirement that the values of the 'letter-spacing' and > 'word-spacing' properties are included in the computation, but those are a > lot simpler to account for and are much, much less likely to be set to > percentage values. For letter-spacing and word-spacing, style flush + text run should be enough. But yeah, if no one currently takes that into account at all, we can probably just ignore it for now as well.

Taking account of letter and word spacing wouldn't be particularly onerous. I think we'd need to add some mechanism to the iterator code to indicate that we should bail out if a percentage value was encountered, and then we would retry after a full sync reflow in that rare case. I just meant to say we don't currently need to do that.

It looks like we should be able to use GetSVGTextFrameForNonLayoutDependentQuery(), avoid calling UpdateGlyphPositioning(), and walk the text runs computing the length (and in the future bail and fall back to using GetSVGTextFrame() in the unlikely event that a percentage value for 'letter-spacing' and 'word-spacing' is encountered). Well, except that SVGTextFrame::GetSubStringLength uses TextRenderedRunIterator, and TextRenderedRunIterator::First() requires that mFrameIterator.UndisplayedCharacters() act on an initialized value, and that value is only initialized by the TextNodeCorrespondenceRecorder::RecordCorrespondence() call at the end of SVGTextFrame::DoReflow(). It's not clear to me so far if we can call RecordCorrespondence() without reflow.

Seems like TextNodeCorrespondenceRecorder::RecordCorrespondence would be safe to call after the SVGTextFrame and its descendants have been constructed. I'm not seeing a way to make nsCSSFrameConstructor do that though.

(In reply to Jonathan Watt [:jwatt] from comment #14) > Seems like TextNodeCorrespondenceRecorder::RecordCorrespondence would be > safe to call after the SVGTextFrame and its descendants have been > constructed. I'm not seeing a way to make nsCSSFrameConstructor do that > though. We have FlushType::Frames (which is currently equal to Style because we construct the frames at the same time as we resolve the style). I think doing that is cheaper than flushing all the layout anyway.

Right, FlushType::Frames is what GetSVGTextFrameForNonLayoutDependentQuery uses (which is why I said we'd call that in comment 13). So making sure the frames have been constructed isn't the issue, the issue is how to call TextNodeCorrespondenceRecorder::RecordCorrespondence during frame construction just after the SVGTextFrame and it's descendant frames have been created. There doesn't appear to be a standard hook for that in nsCSSFrameConstructor code or virtual method on nsIFrame. I'll do it a different way though.

Comment on attachment 8900323 [details] [diff] [review] patch Review of attachment 8900323 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks. I forgot that the text node correspondence doesn't rely on reflow (that's the glyph positioning). Splitting this out from the glyph position seems like the right thing to do. ::: layout/generic/nsFrameStateBits.h @@ +389,5 @@ > FRAME_STATE_BIT(SVG, 23, NS_STATE_SVG_POSITIONING_MAY_USE_PERCENTAGES) > > FRAME_STATE_BIT(SVG, 24, NS_STATE_SVG_TEXT_IN_REFLOW) > > +FRAME_STATE_BIT(SVG, 25, NS_STATE_SVG_TEXT_CORRESPONDENCE_DIRTY) Add a comment? ::: layout/svg/SVGTextFrame.cpp @@ +1728,5 @@ > }; > > uint32_t > TextFrameIterator::UndisplayedCharacters() const > { Can you assert in here that mRootFrame's NS_STATE_SVG_TEXT_CORRESPONDENCE_DIRTY bit is not set?

Oh, and just to confirm, this will handle the case where we toggle a <tpsan> element from display:none to display:inline, yes?

I don't think I can get this approach to fully work. I've split the patch into three pieces for easier digestion, and I'll attach them so that anyone that's interested can take a look.

I'm not sure there's much utility in this frame state bit part of this patch. (Although that's not the issue here.)

Among other things, this patch stops using TextRenderedRunIterator in SVGTextFrame::GetSubStringLength since that requires a reflow for many different things. A highly cut down version of that code flow consisting of just the bare essentials is used instead, but even then it seems that nsTextFrame::GetTrimmedOffsets requires a reflow, and we can't avoid calling that method. Specifically we fail this assertion in nsTextFrame::GetTrimmedOffsets: https://dxr.mozilla.org/mozilla-central/rev/db7f19e26e571ae1dd309f5d2f387b06ba670c30/layout/generic/nsTextFrame.cpp#2943

Try push with those patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b939c8054215e24f2ba12465feb7c45a55360ffe

Two ways we could get around the need for the trimmed offsets: 1. We could check the text nodes' content to see whether we would ever need to trim any white space, and skip the reflow if so. In regular SVG content, it is pretty common to see e.g. <text> ABC </text> where the leading and trailing white space of that text node are trimmed, but perhaps in the case we're worried about here we don't have these spaces? Any value other than white-space:pre (and -moz-pre-space, the way we implement xml:space="preserve") on the <text> can result in some white space being trimmed, and for pre-wrap and pre-line we can have multiple lines of text, each with its own trimmed white space. (We don't currently support setting a width on the <text>, so the enforced line breaks from pre, pre-wrap, pre-line and -moz-pre-space are the only way we can get multi-line SVG text.) So I think what you could do is check that the <text> (and I suppose all of its descendant text content elements) is white-space:normal, and if so call GetTrimmableWhitespaceCount without needing the frames to have been reflowed. Alternatively you could fall back to reflowing if you find any text node with leading or trailing white space characters, when white-space is normal. And for pre and -moz-pre-space I think we'll always have no trimmed spaces. For pre-wrap and pre-line, which will be exceptionally uncommon, we should just fall back to reflowing. 2. We could add a mechanism to do a kind of limited reflow just for the <text> subtree that skips SVG glyph positioning. I don't think there are any inputs to the reflow that would come from the parent of the <text> element that affects how it would get laid out, would there?

nsTextFrame::GetTrimmedOffsets has a parameter aPostReflow, and when it is set to false, it would skip the assertions (that it doesn't require reflow), and it would treat the text as if it is at the start and the end of the line (thus trim all preceding and trailing whitespaces). It seems to me that we need reflow to know whether we are at the start or end of the line, but I guess we don't really need that kind of information for SVG text? If so, we can probably just pass false to aPostReflow.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #26) > nsTextFrame::GetTrimmedOffsets has a parameter aPostReflow, and when it is > set to false, it would skip the assertions (that it doesn't require reflow), > and it would treat the text as if it is at the start and the end of the line > (thus trim all preceding and trailing whitespaces). That is handy. :-) > It seems to me that we need reflow to know whether we are at the start or > end of the line, but I guess we don't really need that kind of information > for SVG text? If so, we can probably just pass false to aPostReflow. Right, we can only have multiple lines with trimmable white space if white-space is pre-wrap or pre-line.

(In reply to Cameron McCormack (:heycam) from comment #25) > 2. We could add a mechanism to do a kind of limited reflow just for the > <text> subtree that skips SVG glyph positioning. I don't think there are > any inputs to the reflow that would come from the parent of the <text> > element that affects how it would get laid out, would there? We need positioning for deciding how many characters in a textPath count. So I think we need a reflow fallback anyway.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #26) > nsTextFrame::GetTrimmedOffsets has a parameter aPostReflow, and when it is > set to false, it would skip the assertions (that it doesn't require reflow), > and it would treat the text as if it is at the start and the end of the line > (thus trim all preceding and trailing whitespaces). Well spotted, thanks. I had seen that but misread the behavior.

Comment on attachment 8902906 [details] [diff] [review] part 1 - Add a method to SVGTextFrame for resolving bi-di before reflow As discussed on IRC.

Comment on attachment 8902906 [details] [diff] [review] part 1 - Add a method to SVGTextFrame for resolving bi-di before reflow Review of attachment 8902906 [details] [diff] [review]: ----------------------------------------------------------------- AFAICT, this should be OK. (As we discussed on irc, the tricky stuff comes in the later patch.)

I was unable to get getSubStringLength working without reflow for text containing a textPath (we need to know if some characters fell off the end of the path, which we can only know after reflow and glyph positioning), and for text containing bidi strings (figuring out how to trim text white space correctly when there are bidi continuations is hard). In those two cases the part 4 patch makes us fall back to flushing reflow and then using the old code paths for calculating the length.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71d820939906c8dcc9d9b25b39b7a57e46e1dd27 Everything now finally seems to pass, except for some apparent rounding differences on for dom/svg/test/test_text_scaled.html on Linux. The order of difference is somewhat small, such as: FAIL | text1 char 1 start offset x - 251 should be close to 250 I think this is due to us not calling UpdateFontSizeScaleFactor() when taking the new code paths. Either we can add that call (I'll look into that), or else we can increase the fuzz for the "text1" part of test_text_scaled.html.

Comment on attachment 8903199 [details] [diff] [review] part 2 - Allow SVGTextFrame's CharIterator helper to be used before reflow Review of attachment 8903199 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/svg/SVGTextFrame.cpp @@ +2139,5 @@ > * @param aSVGTextFrame The SVGTextFrame whose characters to iterate > * through. > * @param aFilter Indicates which characters to iterate over. > * @param aSubtree A content subtree to track whether the current character > * is within. Add docs for the new argument and when it should be used?

Comment on attachment 8903202 [details] [diff] [review] part 4 - Add a version of SVGTextFrame::GetSubStringLength that can be used independantly of reflow, to avoid sync reflows Review of attachment 8903202 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/svg/SVGTextFrame.cpp @@ +4095,5 @@ > * Implements the SVG DOM GetSubStringLength method for the specified > * text content element. > */ > nsresult > SVGTextFrame::GetSubStringLength(nsIContent* aContent, I'd really like it if we could make this fast path work without duplicating too much iteration logic. Is it possible to leave the existing GetSubStringLength as is, except to do the check for whether we can fast path it, and if so, skip the UpdateGlyphPositioning and pass in a flag/mode to TextRenderedRunIterator that will end up passing aPostReflow=false to GetTrimmedOffsets (and maybe ignore run mRunBoundary and mHidden, which would make it work for <textPath> too)? Maybe that stretches what TextRenderedRuniterator is for (since it will now return a single run that is unrelated to whether what is renderable in a single chunk), but I'd rather that than more iteration logic.

(In reply to Cameron McCormack (:heycam) from comment #38) > I'd really like it if we could make this fast path work without duplicating > too much iteration logic. That would be nice, but... > Is it possible to leave the existing > GetSubStringLength as is, except to do the check for whether we can fast > path it, and if so, skip the UpdateGlyphPositioning and pass in a flag/mode > to TextRenderedRunIterator that will end up passing aPostReflow=false to > GetTrimmedOffsets (and maybe ignore run mRunBoundary and mHidden, I don't think so. I did try that approach but the invariant that reflow and glyph positioning has occurred is so built into TextRenderedRun[Iterator] and much of the code that it uses that it doesn't seem practical. Besides that, it kind of breaks what TextRenderedRun is, which is about rendered positioning. > which would make it work for <textPath> too)? Any code path that avoids reflow and glyph positioning can't be made to work for textPath. textPath requires that layout and glyph positioning have been completed to figure out which glyphs will actually be displayed. The spec says things like: When the inline-progression-direction is horizontal, then any ‘x’ attributes on ‘text’, ‘tspan’, ‘tref’ or ‘altGlyph’ elements represent new absolute offsets along the path, thus providing explicit new values for startpoint-on-the-path. Similar things for 'dx', and 'startOffset' matters too. > Maybe that stretches what TextRenderedRuniterator is for (since it will now > return a single run that is unrelated to whether what is renderable in a > single chunk), but I'd rather that than more iteration logic. I think making TextRenderedRun have logic/state that makes it not a rendered run would be pretty horrible too. :(

Pushed by jwatt@jwatt.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/ff8a16b5ebfa Limit SVGTextFrame::GetSubStringLength sync reflows to the SVGTextFrame. r=heycam

Pushed by jwatt@jwatt.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/3a540f01983e Revert landing of wrong patch. r=me

Pushed by jwatt@jwatt.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/545efe0fed92 part 1 - Add a method to SVGTextFrame for resolving bi-di before reflow. r=jfkthame https://hg.mozilla.org/integration/mozilla-inbound/rev/af56ade91b5e part 2 - Allow SVGTextFrame's CharIterator helper to be used before reflow. r=heycam

Comment on attachment 8903202 [details] [diff] [review] part 4 - Add a version of SVGTextFrame::GetSubStringLength that can be used independantly of reflow, to avoid sync reflows Review of attachment 8903202 [details] [diff] [review]: ----------------------------------------------------------------- I think this is OK; I reviewed by comparing to the existing GetSubstringLength and TextRenderedRunIterator::Next code. ::: layout/svg/SVGTextFrame.cpp @@ +4146,5 @@ > + } > + > + charnum = chit.TextElementCharIndex(); > + chit.NextWithinSubtree(nchars); > + nchars = chit.TextElementCharIndex() - charnum; I just noticed that this code to convert element-relative character indexes to <text>-relative ones is common to SelectSubString and GetSubStringLength (though with different nchars == 0 handling). If you think it's helpful, cCan you factor that out into a separate function? @@ +4185,5 @@ > + > + uint32_t offset = textElementCharIndex; > + > + // Intersect the substring we are interested in with the range covered by > + // the rendered run. Probably doesn't make sense to talk about rendered runs here.

https://hg.mozilla.org/mozilla-central/rev/545efe0fed92 https://hg.mozilla.org/mozilla-central/rev/af56ade91b5e

Pushed by jwatt@jwatt.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/302d9e49ac75 part 3 - Support recording of SVGTextFrame correspondence before reflow. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/309140f65fc5 part 4 - Add a version of SVGTextFrame::GetSubStringLength that can be used independantly of reflow, to avoid sync reflows. r=heycam

https://hg.mozilla.org/mozilla-central/rev/302d9e49ac75 https://hg.mozilla.org/mozilla-central/rev/309140f65fc5

Parts 3 and 4 of this patch were backed out from 57 in bug 1403583 due to various regressions. https://hg.mozilla.org/releases/mozilla-beta/rev/c98ecac85244 https://hg.mozilla.org/releases/mozilla-beta/rev/961888633c2c