Created attachment 254512 [details] 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.
11 years ago
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.
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.
Created attachment 254536 [details] [diff] [review] 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.
> 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.
Created attachment 254687 [details] [diff] [review] 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.
Attachment #254687 - Flags: superreview? → superreview?(dbaron)
Wouldn't a change to width/height be at least an eTreeChange?
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.
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.
Interesting that eResize got the testcase working though.
Created attachment 255154 [details] [diff] [review] patch updated to tip Also with the change to eStyleChange.
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.
Attachment #255154 - Flags: superreview?(dbaron) → superreview+
Thanks for review! Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
11 years ago
You need to log in before you can comment on or make changes to this bug.