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

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

Trunk
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

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.
(Assignee)

Comment 2

11 years ago
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.
(Assignee)

Comment 3

11 years ago
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.
(Assignee)

Comment 5

11 years ago
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.
Assignee: general → jwatt
Attachment #254536 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #254687 - Flags: superreview?
Attachment #254687 - Flags: review?(tor)
(Assignee)

Updated

11 years ago
Attachment #254687 - Flags: superreview? → superreview?(dbaron)
Wouldn't a change to width/height be at least an eTreeChange?
(Assignee)

Comment 7

11 years ago
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.
(Assignee)

Comment 8

11 years ago
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.
(Assignee)

Comment 9

11 years ago
Interesting that eResize got the testcase working though.

Updated

11 years ago
Attachment #254687 - Flags: review?(tor) → review+
(Assignee)

Comment 10

11 years ago
Created attachment 255154 [details] [diff] [review]
patch updated to tip

Also with the change to eStyleChange.
Attachment #254687 - Attachment is obsolete: true
Attachment #255154 - Flags: superreview?(dbaron)
Attachment #254687 - Flags: superreview?(dbaron)
(Assignee)

Updated

11 years ago
Blocks: 379615
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+
(Assignee)

Comment 12

11 years ago
Thanks for review! Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: in-testsuite?
(Assignee)

Updated

11 years ago
Depends on: 383296
You need to log in before you can comment on or make changes to this bug.