Closed Bug 600574 Opened 9 years ago Closed 9 years ago

Dynamically resized elements with svg background doesn't invalidate background correctly

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: cork, Assigned: dholbert)

References

Details

(Keywords: testcase)

Attachments

(4 files, 3 obsolete files)

Attached image Background svg
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b7pre) Gecko/20100927 Firefox/4.0b7pre

Dynamically resized elements with an svg file that has width/height=100% doesn't reflow to catch the changed dimensions the element has changed size. (see attached testcase)

Reproducible: Always

Steps to Reproduce:
1. Open the attached test case
2. Watch the animation
3. Move focus to another window to force firefox to reflow

Actual Results:
A triangle is drawn from the left to the right. starting at 10px and ending at 500px

Expected Results:
A slowly enlarging square changing from 10x10px to 500x500px
Attached file testcase
Blocks: 231179
blocking2.0: --- → ?
Hardware: x86_64 → All
Summary: Dynamically resized elements with svg background doesn't reflow → Dynamically resized elements with svg background doesn't invalidate background correctly
It's an invalidation bug; we need to invalidate the whole background area and we aren't.  

Daniel, nsStyleBackground::Layer::RenderingMightDependOnFrameSize needs to handle this (either itself or by having nsStyleBackground::Size::DependsOnFrameSize deal).
Assignee: nobody → dholbert
blocking2.0: ? → betaN+
So I think the thing that we care about here is whether we have an SVG image **with a viewBox attribute** on the root node.

Normally (with no viewBox), we'll just draw our SVG image at 1:1 scale, and crop it to our backround area's size.  But if our SVG image has a viewBox attribute, we scale the viewBox-region to fit our frame size, and our rendering will definitely "depend on frame size."

Still need to spin up some reftests for this, but here's the patch.
This patch includes three reftests, the third of which (background-resize-3.html) actually tests this bug -- dynamically resizing an area whose background is an SVG image that has a viewBox.

The other two added reftests already pass (they don't need this patch), but they're worth testing anyway. (They test the same situation -- dynamically-resized background-area -- but with no viewBox. One uses a px-valued height/width on the <svg>, and the other uses the (default) %-valued height/width on <svg>)
Attachment #479603 - Attachment is obsolete: true
Attachment #479625 - Flags: review?(bzbarsky)
I don't think HasAttr on the viewBox is right. The viewbox might only exist because it's been animated into existence. Instead you need to call IsValid on the nsSVGViewBoxRect (you may need to add some plumbing in order to be able to call this).
Comment on attachment 479625 [details] [diff] [review]
patch 2 v2 (added tests): Check for SVG image with viewBox on root node, in RenderingMightDependOnFrameSize

Ah, good point -- thanks, Robert.
Attachment #479625 - Flags: review?(bzbarsky)
Attachment #479600 - Attachment is patch: true
Attachment #479600 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 479600 [details] [diff] [review]
patch 1: Move RenderingMightDependOnFrameSize to cpp file

Do we really need to take this out of line, or can we keep it as-is so the common case is fast?  I guess we only hit this code once we know we have a background, so ok.
Attachment #479600 - Flags: review?(bzbarsky) → review+
(In reply to comment #8)
> Do we really need to take this out of line

Yes, unless we add some #includes to the .h file. (imgIContainer.h in particular right now, and soon some more nsSVG interface headers to address comment 7).

I chose not to add the #include(s), to minimize contamination of that header file.  Let me know if you think that's the wrong way to go on this tradeoff... (For now, I take your r+ to mean it's fine as-is.)
It's fine as-is; my question was whether we could add a _new_ out-of-line function for the slow case (e.g. the case when we actually have a background image).  Doesn't seem worth it, though.
Ok, this version checks whether the root SVG element's nsSVGViewBox::IsValid.

I also added a reftest for that case (an "animated-into-existence" viewBox), but it actually passes already, due to invalidations from SMIL samples (as described in bug 601012 comment 1).
Attachment #479625 - Attachment is obsolete: true
Attachment #479965 - Flags: superreview?(longsonr)
Attachment #479965 - Flags: review?(bzbarsky)
(This patch also includes a fix for this recently-introduced build warning, while I'm touching nsStyleStruct:
nsStyleStruct.h: In constructor 'nsStyleBorder::nsStyleBorder(nsPresContext*)':
nsStyleStruct.h:953: warning: 'nsStyleBorder::mBorderImage' will be initialized after
nsStyleStruct.h:923: warning:   'bool nsStyleBorder::mImageTracked'
nsStyleStruct.cpp:379: warning:   when initialized here
)
Comment on attachment 479965 [details] [diff] [review]
patch 2 v3: Check for SVG image with viewBox on root node, in RenderingMightDependOnFrameSize

r=me
Attachment #479965 - Flags: review?(bzbarsky) → review+
Attachment #479965 - Flags: superreview?(longsonr) → review?(longsonr)
Comment on attachment 479965 [details] [diff] [review]
patch 2 v3: Check for SVG image with viewBox on root node, in RenderingMightDependOnFrameSize

>+  // Are we an SVG image with a viewBox attribute?
>+  if (mImage.GetType() == eStyleImageType_Image) {
>+    nsCOMPtr<imgIContainer> imageContainer;
>+    mImage.GetImageData()->GetImage(getter_AddRefs(imageContainer));
>+    if (imageContainer &&
>+        imageContainer->GetType() == imgIContainer::TYPE_VECTOR) {
>+      nsIFrame* rootFrame = imageContainer->GetRootLayoutFrame();
>+      if (rootFrame) {
>+        if (nsSVGUtils::RootSVGElementHasViewbox(rootFrame->GetContent())) {
>+          return PR_TRUE;

Nit: You could combine these last two lines into a single if (rootFrame && nsSVGUtils...

r=longsonr whether or not you make the above change
Attachment #479965 - Flags: review?(longsonr) → review+
Applied longsonr's suggestion from comment 14.  Here's the ready-to-land version.
Attachment #479965 - Attachment is obsolete: true
Attachment #480173 - Flags: review?
Attachment #480173 - Flags: review? → review+
Whiteboard: [needs landing]
Landed:
part 1: http://hg.mozilla.org/mozilla-central/rev/15bdefceda18
part 2: http://hg.mozilla.org/mozilla-central/rev/06f22d3a34a1
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b8
Verified fixed in
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101007 Firefox/4.0b8pre
Status: RESOLVED → VERIFIED
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.