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

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Cork]

Attachment: 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

Comment 1

[Cork]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: patch 1: Move RenderingMightDependOnFrameSize to cpp file

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: patch 2: Check for SVG image with viewBox on root node, in RenderingMightDependOnFrameSize

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.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: patch 2 v2 (added tests): Check for SVG image with viewBox on root node, in RenderingMightDependOnFrameSize

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>)

Comment 6

[Robert Longson [:longsonr]]

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 7

[Daniel Holbert [:dholbert]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Daniel Holbert [:dholbert]]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: patch 2 v3: Check for SVG image with viewBox on root node, in RenderingMightDependOnFrameSize

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

Comment 12

[Daniel Holbert [:dholbert]]

(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 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 479965 [details] [diff] [review]
patch 2 v3: Check for SVG image with viewBox on root node, in RenderingMightDependOnFrameSize

r=me

Comment 14

[Robert Longson [: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

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: patch 2 v4 [r=bz, r=longsonr] Check for SVG image with viewBox on root node, in RenderingMightDependOnFrameSize

Applied longsonr's suggestion from comment 14.  Here's the ready-to-land version.

Comment 16

[Daniel Holbert [:dholbert]]

Landed:
part 1: http://hg.mozilla.org/mozilla-central/rev/15bdefceda18
part 2: http://hg.mozilla.org/mozilla-central/rev/06f22d3a34a1

Comment 17

[Cork]

Verified fixed in
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101007 Firefox/4.0b8pre