876186 - Turning on svg.text.css-frames.enabled results in bad jank zooming the IE Test Drive Organization Chart demo

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Flipping svg.text.css-frames.enabled to true makes:

http://ie.microsoft.com/testdrive/Graphics/OrganizationChart/Default.xhtml

perform really badly.

Specifically, use the zoom slider at the top right to zoom in and out rapidly. In Firefox 21 and in Nightly it's nice and smooth, but with the CSS-text pref on in Nightly it janks like crazy. :(

Comment 1

[Cameron McCormack (:heycam)]

We are reflowing the anonymous block child and recomputing mPositions for each nsSVGTextFrame2.  There are three reasons for this:

1. We recompute mPositions whenever the viewport size changes, in case we used percentages.  We can track that and skip it if we don't: bug 876323.

2. The whole <svg> is reflowed (as expected), but in nsSVGTextFrame2::ReflowSVG, we force mPositions to get recomputed, even if we weren't going to reflow the anonymous child in the subsequent UpdateGlyphPositioning call.  The comment I wrote there is not good enough for me to remember why we are doing this.

3. We will reflow the anonymous child anyway, since NS_FRAME_IS_DIRTY is set on the nsSVGTextFrame2 in the parent's nsSVGDisplayContainerFrame::ReflowSVG call.  Am I right that we don't actually need to reflow our anonymous child?

Comment 2

[Cameron McCormack (:heycam)]

Another fun thing about the Organization Chart demo is that it uses fill-rule:evenodd on all of the text (it's set in a style rule matching the <svg>), and that makes us fall on to the slower render-SVG-text-as-paths path:

  https://hg.mozilla.org/mozilla-central/file/tip/layout/svg/nsSVGTextFrame2.cpp#l4722

If I remove that check, then some of the jank disappears.  We kind of need to do this for correctness, though.

We could allow the handling of fill-rule:evenodd for SVG text that is rendered directly (i.e. when calling nsTextFrame::PaintText without the callbacks object) by handling that property in nsTextFrame::DrawTextRun.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: patch

How does this patch work you for jwatt?

Comment 4

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #1)
> 3. We will reflow the anonymous child anyway, since NS_FRAME_IS_DIRTY is set
> on the nsSVGTextFrame2 in the parent's nsSVGDisplayContainerFrame::ReflowSVG
> call.  Am I right that we don't actually need to reflow our anonymous child?

Actually I guess we do in general need to reflow the child.  I wonder if it is easy to determine whether reflow made any changes or not?  And if not, to avoid calling UpdateGlyphPositions?

Comment 5

[Jonathan Watt [:jwatt]]

Comment on attachment 754407 [details] [diff] [review]
patch

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

r=me with comment addressed.

::: gfx/thebes/gfxContext.h
@@ +963,5 @@
> +{
> +public:
> +    gfxContextFillRuleAutoSaveRestore(gfxContext *aContext,
> +                                      gfxContext::FillRule aNewFillRule,
> +                                      bool aDoIt = true)

I don't like this aDoIt argument. It makes the code at the consumers quite opaque in my opinion, since the code that should be the conditional of an |if| block is passed as an argument.

Can you do something more like gfxContextAutoSaveRestore? I.e. have a ctor that takes no args and sets mContent to nullptr, and have a separate SetClipRule method (similar to gfxContextAutoSaveRestore::SetContext)?

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Cameron McCormack (:heycam) from comment #1)
> 2. The whole <svg> is reflowed (as expected), but in
> nsSVGTextFrame2::ReflowSVG, we force mPositions to get recomputed, even if
> we weren't going to reflow the anonymous child in the subsequent
> UpdateGlyphPositioning call.  The comment I wrote there is not good enough
> for me to remember why we are doing this.

That is indeed a pretty lousy comment. Shame the person who wrote it was me:

http://hg.mozilla.org/mozilla-central/rev/247b83139d6d

During the SVG reflow overhaul I was being pretty cautious about not breaking things before trying to speed them up. I think I had some vague notion that this might be necessary, but not a good grasp of why. Knowing what I know now I actually don't think it is necessary. Want to knock up a patch to remove that? If something fails then a better comment is indeed called for. :)

> 3. We will reflow the anonymous child anyway, since NS_FRAME_IS_DIRTY is set
> on the nsSVGTextFrame2 in the parent's nsSVGDisplayContainerFrame::ReflowSVG
> call.  Am I right that we don't actually need to reflow our anonymous child?

We don't have enough information at this point to tell, unfortunately. We don't know if the NS_FRAME_IS_DIRTY reflow is happening because the 'font-size' changed on an ancestor, for example. :(

(In reply to Cameron McCormack (:heycam) from comment #4)
> Actually I guess we do in general need to reflow the child.  I wonder if it
> is easy to determine whether reflow made any changes or not?  And if not, to
> avoid calling UpdateGlyphPositions?

I don't believe so, but check with dbaron/bz. It would be great if there was!

Comment 7

[Jonathan Watt [:jwatt]]

Wait, why are we getting an NS_FRAME_IS_DIRTY reflow? A width/height change should result in an NS_FRAME_HAS_DIRTY_CHILDREN reflow. (See nsStylePosition::CalcDifference.) So where is the NS_FRAME_IS_DIRTY coming from?

Comment 8

[Cameron McCormack (:heycam)]

Today we resolved in the SVG F2F that fill-rule should not apply to <text>.  So we can simplify this patch and not make the changes in nsTextFrame.

Comment 9

[Cameron McCormack (:heycam)]

Attachment: patch (v2)

I *think* we should be safe here without explicitly setting the fill rule.  Previous users of the gfxContent ought to keep it as non-zero, right?

Try run in progress: https://tbpl.mozilla.org/?tree=Try&rev=85819b5b95db

Comment 10

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9be2c9347b07

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9be2c9347b07