396771 - SVG text with percentage positioning not shown correctly

Status: RESOLVED FIXED

(Core :: SVG, defect, P2)

Description

[tor]

Attachment: testcase - should see text on the line

Comment 1

[tor]

Attachment: possible fix

Comment 2

[Jonathan Watt [:jwatt]]

I think the correct thing to do may be to implement InitialUpdate for nsSVGTextFrame (and possibly some of the other SVG text frame classes). Right now the InitialUpdate is handled by the nsSVGTextFrames base class nsSVGDisplayContainerFrame.

Comment 3

[Jonathan Watt [:jwatt]]

We should probably audit the code to make sure that the classes that implement NotifyCanvasTMChanged also have an InitialUpdate implementation that does the correct work too.

Comment 4

[tor]

Attachment: InitialUpdate approach

Comment 5

[tor]

Comment on attachment 285496 [details] [diff] [review]
InitialUpdate approach

Specification conformance issue.

Comment 6

[Robert Longson [:longsonr]]

I don't think this approach will work if the text is in a clip path. See bug 387422.

Basically I'm concerned that InitialUpdate is not called on frames that are the children of defs so it would not get called on clip-path text.

Comment 7

[Mike Schroepfer]

Comment on attachment 285496 [details] [diff] [review]
InitialUpdate approach

Is comment 6 addressed?  If so please re-nom

Comment 8

[Jonathan Watt [:jwatt]]

Well this is much better than it was. Requesting blocking to keep this on the radar. I'll look at Robert's comments in more detail when I can, but that shouldn't stop us taking this patch for now.

Comment 9

[Robert Longson [:longsonr]]

Attachment: NotifyCanvasTMChanged approach

Comment 10

[Robert Longson [:longsonr]]

Perhaps I should make a more concrete suggestion rather than just complaining.

Comment 11

[Robert Longson [:longsonr]]

I get lots of "We shouldn't get here. Viewbox width/height is set to 0. Need to disable display of element as per specs." errors running the reftests but then again I always did.

Comment 12

[Jonathan Watt [:jwatt]]

We don't actually need mInitialUpdate. We can get the same information if we use:

  PRBool firstReflow = GetStateBits() & NS_FRAME_FIRST_REFLOW;

_before_ the nsSVGOuterSVGFrameBase::DidReflow() call.

I'm not sure why you're removing the |else| from around the mForeignObjectHash.EnumerateEntries() call. nsSVGForeignObjectFrame::InitialUpdate() calls DoReflow, so removing the |else| just means we'll call DoReflow twice on each foreignObject during startup.

Comment 13

[Robert Longson [:longsonr]]

Attachment: address review comments

IMHO we should try to just use InitialUpdate for setting/resetting reflow flags until we do reflow properly, if we ever do.

I'm leaving the covered region call for now but I think it should go somewhere else.

Comment 14

[Jonathan Watt [:jwatt]]

The only SVG elements that are meant to take part in reflow are 'svg' and 'foreignObject' and, bar a number of small bugs, I think both "do" reflow pretty well now. No other SVG elements should take part in reflow so I'm not sure I follow you.

To me, InitialUpdate is _the_ method for telling SVG elements that the document is sufficiently loaded to provide them with the information they need to display correctly.

Comment 15

[Robert Longson [:longsonr]]

Attachment: address additional review comments

Comment 16

[Robert Longson [:longsonr]]

I'm happy to do it your way for foreignObject then. The problem with initialUpdate is that it is not called on children of defs, however things like text in clipPaths still need to be positioned. I don't think you can put foreignObject in a defs and have it do anything so we're OK in this case.

Comment 17

[Jonathan Watt [:jwatt]]

Comment on attachment 291439 [details] [diff] [review]
address additional review comments

Okay, I'm going to take my own advice and not try worry about tidying the notification stuff in this bug. r=jwatt with the following fixed.

>+  PRBool firstReflow = (GetStateBits() & NS_FRAME_FIRST_REFLOW) !=0;

Space between the != and the 0.

>+  // XXX we should really have a separate notification for viewport changes and
>+  // not overload NotifyCanvasTMChanged, we would call
>+  // NotifyGlyphMetricsChange only on a viewport change.

I found the comment a bit unclear and it doesn't mention that we only need this notification if we have percentage values. Can you change it to:

  // If we are positioned using percentage values we need to update our
  // position whenever our viewport's dimensions change.
  // XXX we should really have a separate notification for viewport changes and
  // not overload NotifyCanvasTMChanged.

Comment 18

[Robert Longson [:longsonr]]

Attachment: address final review comments

Comment 19

[Jonathan Watt [:jwatt]]

For checkin, can you change the comment on InitialUpdate in nsISVGChildFrame.h?

s/Called once on all/Called once on/, then append the sentence "InitialUpdate is called on all SVG frames except descendants of nsSVGDefsFrame.

Comment 20

[Robert Longson [:longsonr]]

Attachment: update nsISVGChildFrame.h

Comment 21

[tor]

Are the changes beyond that in nsSVGTextFrame::NotifyCanvasTMChanged needed to address this particular bug?

Comment 22

[Robert Longson [:longsonr]]

Without the patch what happens is...

Step 1.
nsSVGOuterSVGFrame::Reflow is called
this calls NotifyViewportChange which just returns as mViewportInitialized is false

Step 2.
then nsSVGOuterSVGFrame::DidReflow is called which sets mViewportInitialised this is the first reflow.

Step 3.
nsSVGOuterSVGFrame::DidReflow is called again 

The patch sets mViewportInitialized to true in step 1 so that snSVGOuterSVGFrame::NotifyViewportChange eventually calls nsSVGTextFrame::NotifyCanvasTMChanged.

Comment 23

[tor]

Comment on attachment 291462 [details] [diff] [review]
update nsISVGChildFrame.h

Thanks for the explanation.

Comment 24

[Robert Longson [:longsonr]]

Attachment: more pathological testcase

Comment 25

[Robert Longson [:longsonr]]

resize the browser window to make it less tall or alternatively taller. Note how nothing happens. Now click on refresh and see the text change.

The text is in a mask and therefore under a defs frame. The svg frame ends up with a cached canvas transformation matrix that should be cleared out when the viewport is resized but the presence of the defs frame prevents this.

Possible solutions (in no particular order)

a) don't cache canvas transformation matrices.
b) don't cache canvas transformation matrices if we are children of defs frames.
c) recursively go through frames looking for nsISVGChild frames everywhere like we do for xml:space
d) Implement an interface on nsSVGContainerFrame so that viewport changes could be passed through. We could call it nsISVGContainerFrame - it would be just like old times ;-)

I'm most tempted by c)

Comment 26

[Robert Longson [:longsonr]]

Attachment: now cleans behind the ears too

Comment 27

[Robert Longson [:longsonr]]

This patch also fixes bug 394463

Comment 28

[Jonathan Watt [:jwatt]]

Comment on attachment 292057 [details] [diff] [review]
now cleans behind the ears too

Can you reinstate your comment changes for InitialUpdate? They're valuable.

>+  nsSVGUtils::NotifyCanvasTMChanged(mFrames.FirstChild(), suppressInvalidation);

I'd prefer this to be NotifyChildrenCanvasTMChanged(this, ...) otherwise it looks like it's just FirstChild that's being notified.

>+nsSVGUtils::NotifyCanvasTMChanged(nsIFrame *aFrame, PRBool suppressInvalidation)
>+{
>+  while (aFrame) {
>+    nsISVGChildFrame* SVGFrame = nsnull;
>+    CallQueryInterface(aFrame, &SVGFrame);
>+    if (SVGFrame)
>+      SVGFrame->NotifyCanvasTMChanged(suppressInvalidation); 
>+    else
>+      nsSVGUtils::NotifyCanvasTMChanged(aFrame->GetFirstChild(nsnull),
>+                                        suppressInvalidation);
>+    aFrame = aFrame->GetNextSibling();

This sooo needs a nice big XXX comment explaining why we're doing this. At least if we're hoping to make the code easier for new people to get into (which I hope we are). Please also put { } around the statements under the if..else too so the comment looks grouped with its statement?

Comment 29

[Robert Longson [:longsonr]]

Attachment: address review comment

Oops, I hadn't noticed that the comments had disappeared.

Other review comments addressed too.

Comment 30

[Jonathan Watt [:jwatt]]

Comment on attachment 292846 [details] [diff] [review]
address review comment

Can you also assert IsFrameOfType(nsIFrame::eSVG) in the else? Currently it isn't possible to recurse out of SVG, but it would be nice to make sure we notice if we break that at some point. Thanks Robert.

Comment 31

[Robert Longson [:longsonr]]

Attachment: add assertion

Comment 32

[Robert Longson [:longsonr]]

checked in.