369827 - Reflow of foreignObject with percentage width/height doesn't work

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Attachment: testcase

Explicit changes to the width or height attributes of a foreignObject now work great. However, when the width and height attributes are given percentage values and the parent SVG is resized (say due to a window resize) then the foreignObject's content doesn't reflow.

Additionally the text in the testcase makes observations about the rather random problem of foreignObject content sometimes failing to render at all.

Comment 1

[Jonathan Watt [:jwatt]]

Actually the content sometimes failing to render is not restricted to foreignObject but rather the whole document. I'll look into that some more and file a separate bug.

Comment 2

[Jonathan Watt [:jwatt]]

I guess the place to trigger a reflow is in nsSVGForeignObjectFrame::NotifyCanvasTMChanged, but sticking in a breakpoint there I hit it over 100 times when resizing by dragging the corner of the window out at a moderate pace. I don't know how kindly the layout system would take to that number of reflow requests.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: questionable patch

Although I can check if the kid is dirty so I don't tell layout it's dirty over and over again. Duh. This patch gets things working. Need to do some more thinking about whether this is the right place to do this, but I'm going to have to come back to it in a bit.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> I don't know how kindly the layout system would take to that number of reflow
> requests.

Just fine.  After the first one, you'll get into FrameNeedsReflow, aIntrinsicDirty != eResize is false, aIntrinsicDirty == eStyleChange is false, The parent already has NS_FRAME_HAS_DIRTY_CHILDREN so we break out of the loop immediately, and PostReflowEvent() is a no-op since one is already posted.

As in, we already have pretty much the dirty bit optimization you're adding.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

Great. So this is essentially the same thing but tidied up a bit. Not sure if eResize is exactly the right thing (would help if the comment for it said what it is maybe :-). Also not sure why we've been using eStyleChange in AttributeChanged, since changes to width/height look like a resize to me.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Wouldn't a change to width/height be at least an eTreeChange?

Comment 7

[Jonathan Watt [:jwatt]]

bz: actually I think SVG is a unique scenario that's not covered in that enum - i.e. we want to mark the intrinsic widths of the descendants dirty, but not the ancestors.

Comment 8

[Jonathan Watt [:jwatt]]

Umm, duh. I'm forgetting foreignObject is a reflow root.

So the only real change in the patch is the reflow request in NotifyCanvasTMChanged. The rest of the patch is just code cleanup so the patch didn't have to increase the code duplication we already had. Boris is right, the request in NotifyCanvasTMChanged should be eStyleChange. I'll change it to that in my checkin assuming the rest of this patch is okay.

Comment 9

[Jonathan Watt [:jwatt]]

Interesting that eResize got the testcase working though.

Comment 10

[Jonathan Watt [:jwatt]]

Attachment: patch updated to tip

Also with the change to eStyleChange.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 255154 [details] [diff] [review]
patch updated to tip

> nsSVGForeignObjectFrame::MarkIntrinsicWidthsDirty()
>   // Since we don't know whether this is because of a style change on an
>   // ancestor or descendant, mark the kid dirty.  If it's a descendant,
>   // all we need is the NS_FRAME_IS_DIRTY_CHILDREN that our caller is
>   // going to set, though.
>-  kid->AddStateBits(NS_FRAME_IS_DIRTY);
>+  //

Add a parenthetical to replace the code:

(If this weren't the case, we'd need a parameter to RequestReflow to pass either NS_FRAME_IS_DIRTY or NS_FRAME_HAS_DIRTY_CHILDREN.)

>   // This is really a style change, except we're already being called
>   // from MarkIntrinsicWidthsDirty, so say it's a resize to avoid doing
>   // the same work over again.
>-  GetPresContext()->PresShell()->FrameNeedsReflow(kid,
>-                                                  nsIPresShell::eResize);
>+
>+  RequestReflow(nsIPresShell::eStyleChange);

The comment above still stands, so this should stay eResize.

With that, sr=dbaron.

Beware merging with bug 378784.  It's replacing the "you must mark dirty or has-dirty-children before calling FrameNeedsReflow" with an additional parameter to FrameNeedsReflow (either NS_FRAME_IS_DIRTY or NS_FRAME_HAS_DIRTY_CHILDREN).  If you land first, let bz know with a comment there.  If he lands first, you'll need to merge.

Comment 12

[Jonathan Watt [:jwatt]]

Thanks for review! Checked in.