Closed Bug 328829 Opened 15 years ago Closed 14 years ago

style changes within SVG don't propagate to foreignObject correctly


(Core :: SVG, defect, P2)






(Reporter: dbaron, Assigned: dbaron)


(Whiteboard: [reflow-refactor][patch])


(3 files, 2 obsolete files)

In bug 328550 I added a comment explaining why style changes requiring reflow that happen within SVG content don't propagate to foreignObject elements correctly.  I should write a testcase for this and attach a patch (it's not too hard, although it could slow down SVG reflow a little bit).
Summary: style changes within SVG don't propage to foreignObject correctly → style changes within SVG don't propagate to foreignObject correctly
David, could you take a minute to elaborate on how this should be fixed, or even whip up that patch if you can find the time? :-)
roc made some changes that helped, and the reflow branch should help a little more.  Ask me again after it lands.
Whiteboard: [reflow-refactor]
So the reflow branch has landed. Can I ask again yet David? :-)
Flags: blocking1.9?
Attached file testcase
Here's a testcase, although I'm seeing some refresh problems so nothing at all shows up until I do a zoom-in and zoom-out.
Attached file testcase #2
The bug is also present for style changes outside the SVG (although I'm not sure if that was the case when I filed it).
Attached patch patch (obsolete) — Splinter Review
Here's a patch.  It's a little hacky, but it should work.  And it fixes both problems.

Basically, if a style change requires reflow, we call MarkIntrinsicWidthsDirty on all descendants of the thing with the style change, plus ancestors up to a reflow root.  So when a foreignObject gets MarkIntrinsicWidthsDirty should match exactly the cases when it needs reflow due to style changes.  And as far as I know, those are the only times when it needs an additional reflow.
Attachment #252834 - Flags: superreview?(roc)
Attachment #252834 - Flags: review?(jwatt)
Priority: -- → P2
Whiteboard: [reflow-refactor] → [reflow-refactor][patch]
Target Milestone: --- → mozilla1.9beta
Er, except I don't need to use eStyleChange because we're in the middle of MarkIntrinsicWidthsDirty...
Attachment #252834 - Flags: superreview?(roc)
Attachment #252834 - Flags: review?(jwatt)
Attached patch patchSplinter Review
Attachment #252834 - Attachment is obsolete: true
Attachment #252836 - Flags: superreview?(roc)
Attachment #252836 - Flags: review?(jwatt)
Hmm, weird. Both testcases seem to work for me in the nightly spun on the day this bug was filed:

(and the build from two days before.) Is that possible? I realized after mistakenly testing the patch in the wrong (unpatched) build which caused me to look back through the archives to see when it had been fixed. Perhaps there are cases this patch would fix that are not tested by the testcases?
For what it's worth, for both testcases:
  Expected results: "hello world" entirely visible within page, no overlap
    between words
  Actual results: "hello" and "world" overlap, and "hello" partly off screen
Comment on attachment 252836 [details] [diff] [review]

My apologies, you're right of course. The patch does fix the overlap problem. *blush* Strange that the actual text resizing bit works without it. So I don't know very much about layout, but from what you've explained this looks got to me. r=jwatt
Attachment #252836 - Flags: review?(jwatt) → review+
The bug showed up that way because painting uses the correct style to paint, but if there hasn't been a reflow, it paints in the wrong place.
Attached image testcase 3 (obsolete) —
Even with this patch and the patch from bug 368683 this testcase fails. The text in the foreignObject continues to wrap at the same width when the SVG is resized.
Comment on attachment 253332 [details]
testcase 3

Ah, it seems if I use eStyleChange instead of eResize it works. I'll take that testcase to the other bug.
Attachment #253332 - Attachment is obsolete: true
Attachment #252836 - Flags: superreview?(roc) → superreview+
Fix checked in to trunk.
Closed: 14 years ago
Resolution: --- → FIXED
Note that I had to comment out one of the reftests which fails intermittently (bug 369046).
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.