Closed Bug 873806 Opened 6 years ago Closed 6 years ago

invalidation problem with mask-content.svg and clipPath-content.svg

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(4 files, 1 obsolete file)

These two tests do not re-paint properly once the Ahem font loads.
There are two issues here.  The first is that the test needs to wait until the font is loaded.  The second is that when the font does load, and the document is reflowed, nsSVGDisplayContainerFrame::ReflowSVG() will stop at a non-display subtree.  We need to make sure any nsSVGTextFrame2 inside there is marked dirty and then invalidates its rendering observers, so that the anonymous block frame is reflowed.
Attached patch patch (obsolete) — Splinter Review
This appears to work, though I am unsure if it is the right way to solve the problem.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #751445 - Flags: review?(jwatt)
Blocks: 839955
> +HTTP(../..) == mask-content.svg mask-content-ref.svg  # fails due to invalidation problem - bug 873806

The # fails comments can go.
Comment on attachment 751445 [details] [diff] [review]
patch

The nsSVGContainerFrame.cpp changes are something of a sledgehammer.

So what I find peculiar is that if I change the text content of the second <text> from "   " to "  i", then the "i" repaints at the correct size and position, but the line-through doesn't get updated to extend all the way across the text. Why is that? How is the length of the line-through being cached?
So here's what I understand about how the invalidation and reflow works in these two tests:

The Ahem font loads after the document has finished loading and has reflowed. When the Ahem font loads, we get this call:

  nsIDocument::SetNeedStyleFlush
  nsCSSFrameConstructor::PostRestyleEventInternal
  nsCSSFrameConstructor::PostRebuildAllStyleDataEvent
    ^ sets mRebuildAllStyleData = true
  nsPresContext::PostRebuildAllStyleDataEvent(NS_STYLE_HINT_REFLOW)
  nsPresContext::UserFontSetUpdated

which causes nsIFrame::InvalidateFrameSubtree to be called over all frames. In the case of clipPath-content.svg, when InvalidateFrameSubtree is called on the nsSVGClipPathFrame it also invalidates its rendering observers:

  nsIFrame::InvalidateFrameSubtree               <-  nsSVGPathGeometryFrame
  nsSVGPaintingProperty::DoUpdate
  nsSVGRenderingObserver::InvalidateViaReferencedElement
  nsSVGRenderingObserverList::InvalidateAll
  nsSVGEffects::InvalidateDirectRenderingObservers
  nsSVGEffects::InvalidateDirectRenderingObservers
  InvalidateFrameInternal
  nsIFrame::InvalidateFrame                      <-  nsSVGClipPathFrame
  nsIFrame::InvalidateFrameSubtree
  nsIFrame::InvalidateFrameSubtree
  nsIFrame::InvalidateFrameSubtree
  nsIFrame::InvalidateFrameSubtree
  nsIFrame::InvalidateFrameSubtree
  nsIFrame::InvalidateFrameSubtree
  nsIFrame::InvalidateFrameSubtree
  DoApplyRenderingChangeToTree
  ApplyRenderingChangeToTree
  nsCSSFrameConstructor::ProcessRestyledFrames
  nsCSSFrameConstructor::DoRebuildAllStyleData
  nsCSSFrameConstructor::RebuildAllStyleData
  nsCSSFrameConstructor::ProcessPendingRestyles  <-  ViewportFrame
  PresShell::FlushPendingNotifications

Since InvalidateFrameSubtree is called on all frames the invalidation of the rendering observers is kinda redundant of course.

After the restyling is done, PresShell::DoReflow is called on the ViewportFrame to do an NS_FRAME_IS_DIRTY reflow, which traverses the entire frame tree, except that it doesn't descend into NS_STATE_SVG_NONDISPLAY_CHILD (since the reflow overhead and the invalidation of observers of all NS_STATE_SVG_NONDISPLAY_CHILD frames would generally be very wasteful).

I'm not sure yet exactly what a better solution to the addition of ReflowSVGText might look like, but I'd definitely like to understand more about why the line-through doesn't get updated unless we reflow the text frames whereas the glyphs do.
Comment on attachment 751445 [details] [diff] [review]
patch

Cancelling r? for now until we get some more insight here.
Attachment #751445 - Flags: review?(jwatt)
(In reply to Jonathan Watt [:jwatt] from comment #4)
> So what I find peculiar is that if I change the text content of the second
> <text> from "   " to "  i", then the "i" repaints at the correct size and
> position, but the line-through doesn't get updated to extend all the way
> across the text. Why is that? How is the length of the line-through being
> cached?

The text frame isn't getting reflowed, and the length of the decoration is based on the width of the frame.  The text that is painted simply runs off the edge of the frame's rect.
That is, since we haven't reflowed the text frame, it still has its "old" width (the correct width for the fallback font).  The text frames have the correct new font and font size since they do get restyled, even the non-display frames, under the ComputeStyleChangeFor() call in nsCSSFrameConstructor::DoRebuildAllStyleData.
Attached image test
Here is a slightly simpler test that exhibits the same problem, without the downloadable font.
Let me know if you need any more information.
Flags: needinfo?(jwatt)
Blocks: 876831
Comment on attachment 751445 [details] [diff] [review]
patch

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

If we were to take this, here would be my review comments.

::: layout/svg/nsSVGContainerFrame.cpp
@@ +244,5 @@
>  }
>  
> +/**
> + * Traverses a frame tree, marking any nsSVGTextFrame2 frame as dirty
> + * and calling InvalidateRenderingObservers() on it.

We need to expand on this comment a fair bit to explain _why_ it exists. Specifically, append something like this:

 *
 * The reason that this helper exists is because nsSVGTextFrame2 is special.
 * None of the other SVG frames ever need to be reflowed when they have the
 * NS_STATE_SVG_NONDISPLAY_CHILD bit set on them because their PaintSVG methods
 * (and those of any containers that they can validly be contained within) do
 * not make use of mRect or overflow rects. "em" lengths, etc., are resolved
 * as those elements are painted.
 *
 * nsSVGTextFrame2 is different because its anonymous block and inline frames
 * need to be reflowed in order to get the correct metrics when things like
 * inherited font-size of an ancestor changes, or a delayed webfont loads and
 * applies.
 *
 * We assume that any change that requires the anonymous kid of an
 * nsSVGTextFrame2 to reflow will result in an NS_FRAME_IS_DIRTY reflow. When
 * that reflow reaches an NS_STATE_SVG_NONDISPLAY_CHILD frame it would normally
 * stop, but this helper looks for any nsSVGTextFrame2 descendants of such
 * frames and marks them NS_FRAME_IS_DIRTY so that the next time that they are
 * painted their anonymous kid will first get the necessary reflow.

@@ +247,5 @@
> + * Traverses a frame tree, marking any nsSVGTextFrame2 frame as dirty
> + * and calling InvalidateRenderingObservers() on it.
> + */
> +static void
> +ReflowSVGText(nsSVGContainerFrame* aContainer)

Please call this ReflowSVGNonDisplayText.

@@ +248,5 @@
> + * and calling InvalidateRenderingObservers() on it.
> + */
> +static void
> +ReflowSVGText(nsSVGContainerFrame* aContainer)
> +{

Assert that NS_STATE_SVG_NONDISPLAY_CHILD is set on aContainer.

@@ +309,5 @@
>        // We build up our child frame overflows here instead of using
>        // nsLayoutUtils::UnionChildOverflow since SVG frame's all use the same
>        // frame list, and we're iterating over that list now anyway.
>        ConsiderChildOverflow(overflowRects, kid);
> +    } else {

Assert here that kid is NS_STATE_SVG_NONDISPLAY_CHILD.

Also I'm pretty sure you only need to do this if the NS_FRAME_IS_DIRTY bit is set on this container frame, so you could make it conditional on that, and assert in ReflowSVGText that that bit is set on aContainer's parent.
Flags: needinfo?(jwatt)
(In reply to Cameron McCormack (:heycam) from comment #9)
> Created attachment 753286 [details]
> test
> 
> Here is a slightly simpler test that exhibits the same problem, without the
> downloadable font.

Please also add this as a reftest.
I'm actually not sure that this patch will work in certain instances. Another reftest to add is something like this:

  <defs>
    <g>
      <mask>
        <text>
      </mask>
    </g>
  </defs>

Now do the class="a" setting on the <g> and see if the text updates or not.
If that doesn't work you may need to override DidSetStyleContext. The ReflowSVGText thing would still be needed for delayed loading of webfonts, though.
(In reply to Jonathan Watt [:jwatt] (away 1-23 June) from comment #13)
> I'm actually not sure that this patch will work in certain instances.

Your intuition was right; this doesn't work when the style change is on the non-display <g>.
Attached patch patch (v2)Splinter Review
This addresses your comments, and uses DidSetStyleContext to notice the style change on the non-display element.  I added a couple more tests for cases like that.  I moved the real work that ReflowSVGNonDisplayText into a member function on nsSVGTextFrame2 so I can call it from nsSVGTextFrame2::DidSetStyleContext too.

I'm wondering whether non-display containers should implement nsISVGChildFrame, and have their ReflowSVG implementation not calculate mRect and overflow rects, but just do the work of ReflowSVGNonDisplayText.  There are probably a few call sites of nsISVGChildFrame that we'd need to audit to make sure they are not assuming the frame is not a non-display container if it doesn't implement that interface.  Maybe we can consider that in a followup, if it's a reasonable idea?
Attachment #753286 - Attachment is obsolete: true
Attachment #755744 - Flags: review?(jwatt)
Attachment #753286 - Attachment is obsolete: false
Attachment #751445 - Attachment is obsolete: true
By the way, can we really optimise the condition inside DidSetStyleContext to check whether only properties that apply to SVG text were changed?  Won't it be possible that PeekStyleXXX on the old style struct will return null, and we won't know what the old style is for that style struct?

Instead, can we maybe inspect whether the <text> has any observers?  For what we have in this patch, it's no big deal, since we only set the dirty bit and call InvalidateRenderingObservers, which will do nothing if we don't have any.  But for bug 843917, whose patch I'll rework on top of this one, where we're going to do a bit more work, it might matter more.
(In reply to Cameron McCormack (:heycam) from comment #18)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8bfd675518e9

Ignore that, wrong bug.
Updated patch just with some further changes to mask-content.svg and clipPath-content.svg.  Since we're now reflowing the non-display nsSVGTextFrame2s outside of painting the mask/clipPath, we're getting a different mFontSizeScaleFactor set.  That's the issue I brought up in bug 877957.

The changes to these tests are just to render the <text>-with-line-decoration reference image through <pattern>, so that it gets the same mFontSizeScaleFactor as the test files (i.e., 1).  Otherwise the thickness of the decoration is slightly different.
Attachment #757125 - Flags: review?(longsonr)
One thing the Part 1 patch brought to my attention was that in a frame tree for content like this:

  <foreignObject requiredFeatures="fail">
    <svg>
      <text>

the nsSVGForeignObjectFrame is NS_STATE_SVG_NONDISPLAY_CHILD, but the nsSVGOuterSVGFrame, nsSVGOuterSVGAnonChildFrame and nsSVGTextFrame2 frames are not.  That's because the nsSVGOuterSVGFrame doesn't check to see if it's in a non-display subtree and set the bit on itself accordingly.  We can't just look up to the parent for this -- we need to look all the way to the closest SVG frame, since NS_STATE_SVG_NONDISPLAY_CHILD is not propagated into non-SVG frames.  We could maybe do this a bit better to avoid looking back up the tree, but it'd be a bit of pain and probably doesn't matter too much.

Anyway, the upshot of this was that we were not correctly calling ReflowSVGNonDisplayText on an nsSVGTextFrame2 in this case, which then caused DOM calls on the element to crash.
Attachment #757128 - Flags: review?(longsonr)
Attachment #757128 - Flags: review?(longsonr) → review+
Comment on attachment 757125 [details] [diff] [review]
Part 1: Ensure non-display SVG text frames get reflowed. (v2.1)

>+static void
>+ReflowSVGNonDisplayText(nsSVGContainerFrame* aContainer)
>+{
>+  NS_ASSERTION(aContainer->GetStateBits() & NS_FRAME_IS_DIRTY,
>+               "expected aContainer to be NS_FRAME_IS_DIRTY");
>+  NS_ASSERTION(aContainer->GetStateBits() & NS_STATE_SVG_NONDISPLAY_CHILD,
>+               "it is wasteful to call ReflowSVGNonDisplayText on a container "
>+               "frame that is not NS_STATE_SVG_NONDISPLAY_CHILD");
>+  for (nsIFrame* kid = aContainer->GetFirstPrincipalChild(); kid;
>+       kid = kid->GetNextSibling()) {
>+    nsSVGTextFrame2* text = do_QueryFrame(kid);

This works but we generally use GetType() if only one kind of frame can match (and then a static_cast) Persumably that way is faster. If you really really like your way I'm OK with it though.

if (kid->GetType() == nsGkAtoms::svgTextFrame2) {
    static_cast<nsSVGTextFrame2*>(kid)...

>+    if (text) {
>+      text->ReflowSVGNonDisplayText();
>+    } else {
>+      nsSVGContainerFrame* kidContainer = do_QueryFrame(kid);

I think this might give you a XUL frame in which case the state bit checking above wouldn't work. I think we need to check that the content IsSVG() too.

>+      if (kidContainer) {
>+        ReflowSVGNonDisplayText(kidContainer);
>+      }
>+    }
>+  }
>+}
>+
> void
> nsSVGDisplayContainerFrame::ReflowSVG()
> {
>@@ -287,6 +333,18 @@ nsSVGDisplayContainerFrame::ReflowSVG()
>       // nsLayoutUtils::UnionChildOverflow since SVG frame's all use the same
>       // frame list, and we're iterating over that list now anyway.
>       ConsiderChildOverflow(overflowRects, kid);
>+    } else {
>+      // Inside a non-display container frame, we might have some
>+      // nsSVGTextFrame2s.  We need to cause those to get reflowed in
>+      // case they are the target of a rendering observer.
>+      NS_ASSERTION(kid->GetStateBits() & NS_STATE_SVG_NONDISPLAY_CHILD,
>+                   "expected kid to be a NS_STATE_SVG_NONDISPLAY_CHILD frame");
>+      if (kid->GetStateBits() & NS_FRAME_IS_DIRTY) {
>+        nsSVGContainerFrame* container = do_QueryFrame(kid);

Check that the content IsSVG() too to make sure you don't have a XUL frame here.

>+        if (container) {
>+          ReflowSVGNonDisplayText(container);
>+        }
>+      }
>     }
>   }
> 
>diff --git a/layout/svg/nsSVGTextFrame2.cpp b/layout/svg/nsSVGTextFrame2.cpp
>index ee71b3f..4eff229 100644
>--- a/layout/svg/nsSVGTextFrame2.cpp
>+++ b/layout/svg/nsSVGTextFrame2.cpp
>@@ -3043,6 +3043,52 @@ nsSVGTextFrame2::GetType() const
>   return nsGkAtoms::svgTextFrame2;
> }
> 

> void
>diff --git a/layout/svg/nsSVGTextFrame2.h b/layout/svg/nsSVGTextFrame2.h
>index e13b5b2..eaab59d 100644
>--- a/layout/svg/nsSVGTextFrame2.h
>+++ b/layout/svg/nsSVGTextFrame2.h
>@@ -238,6 +238,8 @@ public:
>   }
> #endif
> 
>+  virtual void DidSetStyleContext(nsStyleContext* aOldStyleContext);
>+

Add MOZ_OVERRIDE

r=longsonr with review comments addressed.
Attachment #757125 - Flags: review?(longsonr) → review+
https://hg.mozilla.org/mozilla-central/rev/8b8cd5ba2a1e
https://hg.mozilla.org/mozilla-central/rev/a4523b6a2e5f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Attachment #755744 - Flags: review?(jwatt)
You need to log in before you can comment on or make changes to this bug.