Closed Bug 577824 Opened 14 years ago Closed 7 years ago

HTML element attached is not rendered if an SVG filter is applied to it

Categories

(Core :: SVG, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: Jeremie, Assigned: u459114)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; en-US; rv:2.0b2pre) Gecko/20100710 Minefield/4.0b2pre
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; en-US; rv:2.0b2pre) Gecko/20100710 Minefield/4.0b2pre

I'm not sure if it's "SVG" related or "Layout: View Rendering" related

When you dynamically attach an HTML element to the DOM tree of an HTML document, the element is not render if it exist a stylesheet which define that an SVG filter must apply to it.

Reproducible: Always

Steps to Reproduce:
1. Open the testcase
 
Actual Results:  
You can see one grey square

Expected Results:  
You should see 2 grey square
Attached file testcase
Summary: A HTML element attach to the DOM is note render if an SVG filter apply to it → An HTML element attach to the DOM is not render if an SVG filter apply to it
Looks like the filter is created with zero size as all the style is applied together so when the filter region is calculated it is empty as the width and height are still default values i.e. before the style has set them to 100px. Setting the filter asynchronously works. 

I'm not sure what to do here to fix this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I've run some tests with an image instead of an arbitrary DIV element and it behave the same even without CSS width and height. So I guess that it's not only a mater of CSS, but a global question of when the size of an element is computed and when the filter is applied. 

Unfortunately, I've no idea on how the width and height are computed under the hood. But if your right, is it possible to force the SVG filter to be applied after the size of the element is determine? (yes, I know that it's really naive but who knows?)

FWIW, if the page is force to reflow (by changing the window size for example), the second square become visible.
Robert, I'd think changes to the element size would recompute the filter.  That's the only sane thing to do in this situation, no?
Indeed, I'm not sure why that's not happening though. The resize should cause a repaint which should automatically recompute the filter.
Are we failing to invalidate things due to the filter being applied or something?
Attached file Specifique TC for HTML img element (obsolete) —
Hello,

I'm working on a new app that rely on SVG filters over HTML img elements and this bug just drive me crazy (I guess it's the same bug but let me know if it is not).

This test case provide a new use case when the `src` attribut of a img element is changed without providing explicitly the width and heigth of the new image source.

In this TC, the first image is well loaded and the filter just work fine. The second image have an intrinsic size of 200x100px. When the page is loaded, we change it's source for an image with an intrinsic size of 200x200px. Unfortunately, once this new image is loaded, the filter is partially applied to the new image. It crop it for an height a bit bigger than the previous image source !

I hope this will help finding what's happening with filters here.
Comment 7 seems like a different bug that should be filed separately.
Blocks: 590434
(In reply to comment #8)
> Comment 7 seems like a different bug that should be filed separately.

Done on Bug 590434
Attachment #468654 - Attachment is obsolete: true
The original testcase works for me now. No idea what fixed it.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WORKSFORME
It seems that the change occur with this build : http://hg.mozilla.org/mozilla-central/rev/d708c2fa7fea
Jeremie,

I don't think that can be right. My build isn't as new as that (http://hg.mozilla.org/mozilla-central/rev/a3f202325d27)
Ok, I found it.

In fact, the bug is not resolved.

If you open the test case, even with the last build, the test will fail.
But, if you let the test open, then relaunch minefield, you'll see the test as it pass (but if you try to reload it you'll see that it's not the case).
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Seems to work on trunk now.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → WORKSFORME
The test case is still broken with the current nightly (from 2011/12/18) :

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0a1) Gecko/20111218 Firefox/11.0a1
http://hg.mozilla.org/mozilla-central/rev/a5e63e00db27

I'm waiting for the next nightly to check it.
There is nothing that would fix things in the next nightly but it didn't disappear on any kind of refresh when I tried it.
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: WORKSFORME → ---
(In reply to Robert Longson from comment #16)
> it didn't disappear on any kind of refresh when I tried it.

I'm not sure what you expect to disappear. The test case should show 2 gray squares and with the current nightly there is only one visible.

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0a1) Gecko/20111219 Firefox/11.0a1
http://hg.mozilla.org/mozilla-central/rev/d75ebb37080e
I was confused by comment 13 in that I see things exactly the same no matter how I reload it. I do only see 1 item though.
(In reply to Robert Longson from comment #18)
> I was confused by comment 13 in that I see things exactly the same no matter
> how I reload it. I do only see 1 item though.

Yes, it seams that the behavior from comment 13 no longer occur but the test case is still broken.
Assignee: nobody → cku
nsIFrame::FinishAndStoreOverflow of the filtered frame is called three times, if we write <div></div> in html; By using js appending the filtered div, nsIFrame::FinishAndStoreOverflow is only called once, and the size of the filtered frame is not calculated yet(width=0 and height=0)

Steps:
1. Set breakpoint at nsFilterInstance::GetPostFilterBounds
2. dump aFilteredFrame->GetVisualOverflowRect().

Case 1: <div></div>: #2 return valid width and height at 2nd and 3rd call.
Case 2: js.appendElement: #2 return (0,0,0,0) and is called once only.
nsFilterInstance::GetPostFilterDirtyArea(nsIFrame *aFilteredFrame,
                                         const nsRegion& aPreFilterDirtyRegion)

The size of aPreFilterDirtyRegion, which is passed from FrameLayerBuilder, is (1X1)
A big problem behind this bug is why nsBlockFrame::Reflow of the static <div> be called tree times.
Well, it should be called at least twice, right?  Once during load, and again after the thing is appended right after it and potentially margin-collapses through it...
Blocks: svg-enhance
Summary: An HTML element attach to the DOM is not render if an SVG filter apply to it → HTML element attached is not rendered if an SVG filter is applied to it
(In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #29)
> Well, it should be called at least twice, right?  Once during load, and
> again after the thing is appended right after it and potentially
> margin-collapses through it...
Sounds reasonable. Thanks.
The condition of this bug:
1. An element which is created and append to document by javascript.
2. There is an SVG effect apply onto that element. (css filter functions is ok)

That element will appear if it's been reflow by any reason.
Attachment #8894999 - Flags: review?(jwatt)
Attachment #8895010 - Flags: review?(jwatt)
(In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #29)
> Well, it should be called at least twice, right?  Once during load, and
> again after the thing is appended right after it and potentially
> margin-collapses through it...

Possibly the other time is due to use reflowing assuming scrollbars will be needed, then reflowing again after we discover that they're not needed.
Comment on attachment 8894999 [details]
Bug 577824 - Part 1. Set the frame size in nsIFrame::FinishAndStoreOverflow before the ComputeEffectsRect call that uses it.

https://reviewboard.mozilla.org/r/166126/#review171752

This patch basically moves the setting of mRect to be before the ComputeEffectsRect and GetClipPropClipRect calls, but both of those functions are supposed to do their work using the passed "new size" instead of the frame's current mRect values. In other words, if this patch fixes the issue then it points to a deeper problem under ComputeEffectsRect or GetClipPropClipRect.

Can you build without your patch, set a breakpoint before the ComputeEffectsRect, run and break there just before the problematic calls are going to be made, put a memory breakpoint on mRect's width and height, and then figure out who is accessing those members under those calls?

It seems fine to move the SetRect call up, I think, but doing so would also be masking the bug in ComputeEffectsRect/GetClipPropClipRect. We should fix that bug.

::: layout/generic/nsFrame.cpp:9055
(Diff revision 7)
>      }
>    }
>  
>    ComputeAndIncludeOutlineArea(this, aOverflowAreas, aNewSize);
>  
> +  /* If we're transformed, transform the overflow rect by the current transformation. */

This comment belongs to the code below the code that you have moved up. We'd want to keep it down there.
Comment on attachment 8895010 [details]
Bug 577824 - Part 2. Add a reftest for an SVG filtered HTML element that's inserted into the DOM by script.

https://reviewboard.mozilla.org/r/166140/#review171776

We shouldn't be adding to layout/reftests/bugs since it makes it harder to figure out what tests we have for various parts of the code. Please put this test at layout/reftests/svg/svg-integration/filter-html-dynamic-01.xhtml

r+ assuming you move the test.
Attachment #8895010 - Flags: review?(jwatt) → review+
Comment on attachment 8895010 [details]
Bug 577824 - Part 2. Add a reftest for an SVG filtered HTML element that's inserted into the DOM by script.

https://reviewboard.mozilla.org/r/166140/#review171782

::: commit-message-ebb37:1
(Diff revision 5)
> +Bug 577824 - Part 2. A reftest for apply SVG filter onto a dynamic created element.

Oh, and please make this commit message: "Add a reftest for an SVG filtered HTML element that's inserted into the DOM by script"
(In reply to Jonathan Watt [:jwatt] from comment #42)
> Comment on attachment 8894999 [details]
> Bug 577824 - Part 1. Mask sure the size of the filtered frame been set
> correctly before computing filter region.
> 
> https://reviewboard.mozilla.org/r/166126/#review171752
> 
> This patch basically moves the setting of mRect to be before the
> ComputeEffectsRect and GetClipPropClipRect calls, but both of those
> functions are supposed to do their work using the passed "new size" instead
> of the frame's current mRect values. In other words, if this patch fixes the
> issue then it points to a deeper problem under ComputeEffectsRect or
> GetClipPropClipRect.
For ComputeEffectsRect, it calls GetPostFilterBounds:
nsFilterInstance::GetPostFilterBounds()
{
  UniquePtr<UserSpaceMetrics> metrics = UserSpaceMetricsForFrame(aFilteredFrame);
}

metrics->GetSize() basically the size of aFilteredFrame.

For GetClipPropClipRect, will check
Comment on attachment 8894999 [details]
Bug 577824 - Part 1. Set the frame size in nsIFrame::FinishAndStoreOverflow before the ComputeEffectsRect call that uses it.

https://reviewboard.mozilla.org/r/166126/#review171752

For ComputeEffectsRect

nsSVGFilterInstance::ComputeBounds uses the size stored in mMetrics to compute bounds in the user space.
nsSVGFilterInstance::ComputeBounds() #15
{
  gfxRect userSpaceBounds =
    nsSVGUtils::GetRelativeRect(filterUnits, XYWH, mTargetBBox, mMetrics); #14
}

mMatrics is passed from the caller: nsFilterInstance::GetPostFilterBounds
nsFilterInstance::GetPostFilterBounds(const nsRect *aPreFilterBounds) << aPreFilterBounds is aNewSize
{
  UniquePtr<UserSpaceMetrics> metrics = UserSpaceMetricsForFrame(aFilteredFrame);
}

(gdb) rwatch *0x7fffc6c40d70
(gdb) bt 30
#0  0x00007fffe11dad85 in mozilla::gfx::BaseRect<int, nsRect, nsPoint, nsSize, nsMargin>::Size (this=0x7fffc6c40d68) at /home/cjcool/repository/mozilla-central-debug/obj-debug/dist/include/mozilla/gfx/BaseRect.h:350
#1  0x00007fffe15841eb in nsIFrame::GetRectRelativeToSelf (this=0x7fffc6c40d60) at /home/cjcool/repository/mozilla-central-debug/layout/generic/nsIFrame.h:941
#2  0x00007fffe3d71329 in BoxToRect::AddBox (this=0x7fffffff7ac0, aFrame=0x7fffc6c40d60) at /home/cjcool/repository/mozilla-central-debug/layout/base/nsLayoutUtils.cpp:4017
#3  0x00007fffe3d51242 in nsLayoutUtils::AddBoxesForFrame (aFrame=0x7fffc6c40d60, aCallback=0x7fffffff7ac0) at /home/cjcool/repository/mozilla-central-debug/layout/base/nsLayoutUtils.cpp:3943
#4  (aFrame=0x7fffc6c40d60, aCallback=0x7fffffff7ac0) at /home/cjcool/repository/mozilla-central-debug/layout/base/nsLayoutUtils.cpp:3951
#5  0x00007fffe3d514ae in nsLayoutUtils::GetAllInFlowRects (aFrame=0x7fffc6c40d60, aRelativeTo=0x7fffc6c40d60, aCallback=0x7fffffff7b20, aFlags=0) at /home/cjcool/repository/mozilla-central-debug/layout/base/nsLayoutUtils.cpp:4076
#6  0x00007fffe3d5176b in nsLayoutUtils::GetAllInFlowRectsUnion (aFrame=0x7fffc6c40d60, aRelativeTo=0x7fffc6c40d60, aFlags=0) at /home/cjcool/repository/mozilla-central-debug/layout/base/nsLayoutUtils.cpp:4120
#7  0x00007fffe3fd2715 in nsSVGIntegrationUtils::GetSVGCoordContextForNonSVGFrame (aNonSVGFrame=0x7fffc6c40d60) at /home/cjcool/repository/mozilla-central-debug/layout/svg/nsSVGIntegrationUtils.cpp:209
#8  0x00007fffe3222857 in mozilla::dom::NonSVGFrameUserSpaceMetrics::GetSize (this=0x7fffa85ed410) at /home/cjcool/repository/mozilla-central-debug/dom/svg/nsSVGLength2.cpp:192
#9  0x00007fffe32228b3 in mozilla::dom::UserSpaceMetricsWithSize::GetAxisLength (this=0x7fffa85ed410, aCtxType=1 '\001') at /home/cjcool/repository/mozilla-central-debug/dom/svg/nsSVGLength2.cpp:198
#10 0x00007fffe3222c74 in nsSVGLength2::GetUnitScaleFactor (this=0x7fffffff7da4, aMetrics=..., aUnitType=2 '\002') at /home/cjcool/repository/mozilla-central-debug/dom/svg/nsSVGLength2.cpp:259
#11 0x00007fffe3feae15 in nsSVGLength2::GetAnimValue (this=0x7fffffff7da4, aMetrics=...) at /home/cjcool/repository/mozilla-central-debug/dom/svg/nsSVGLength2.h:130
#12 0x00007fffe3fe473f in nsSVGUtils::UserSpace (aMetrics=..., aLength=0x7fffffff7da4) at /home/cjcool/repository/mozilla-central-debug/layout/svg/nsSVGUtils.cpp:344
#13 0x00007fffe3fe781b in nsSVGUtils::GetRelativeRect (aUnits=1, aXYWH=0x7fffffff7d80, aBBox=..., aMetrics=...) at /home/cjcool/repository/mozilla-central-debug/layout/svg/nsSVGUtils.cpp:1250
#14 0x00007fffe3fca25e in nsSVGFilterInstance::ComputeBounds (this=0x7fffffff7e50) at /home/cjcool/repository/mozilla-central-debug/layout/svg/nsSVGFilterInstance.cpp:90
#15 0x00007fffe3fca0fc in nsSVGFilterInstance::nsSVGFilterInstance (this=0x7fffffff7e50, aFilter=..., aTargetFrame=0x7fffc6c40d60, aTargetContent=0x7fffc05ffa60, aMetrics=..., aTargetBBox=..., aUserSpaceToFilterSpaceScale=...)
    at /home/cjcool/repository/mozilla-central-debug/layout/svg/nsSVGFilterInstance.cpp:57
#16 0x00007fffe3fb5d2d in nsFilterInstance::BuildPrimitivesForFilter (this=0x7fffffff8190, aFilter=..., aTargetFrame=0x7fffc6c40d60, aInputIsTainted=true)
    at /home/cjcool/repository/mozilla-central-debug/layout/svg/nsFilterInstance.cpp:319
#17 0x00007fffe3fb5b8b in nsFilterInstance::BuildPrimitives (this=0x7fffffff8190, aFilterChain=..., aTargetFrame=0x7fffc6c40d60, aFilterInputIsTainted=true)
    at /home/cjcool/repository/mozilla-central-debug/layout/svg/nsFilterInstance.cpp:294
#18 0x00007fffe3fb56c6 in nsFilterInstance::nsFilterInstance (this=0x7fffffff8190, aTargetFrame=0x7fffc6c40d60, aTargetContent=0x7fffc05ffa60, aMetrics=..., aFilterChain=..., aFilterInputIsTainted=true, aPaintCallback=0x0, 
    aPaintTransform=..., aPostFilterDirtyRegion=0x0, aPreFilterDirtyRegion=0x0, aPreFilterVisualOverflowRectOverride=0x0, aOverrideBBox=0x7fffffff8400)
    at /home/cjcool/repository/mozilla-central-debug/layout/svg/nsFilterInstance.cpp:221
#19 0x00007fffe3fb50b4 in nsFilterInstance::GetPostFilterBounds (aFilteredFrame=0x7fffc6c40d60, aOverrideBBox=0x7fffffff8400, aPreFilterBounds=0x0) at /home/cjcool/repository/mozilla-central-debug/layout/svg/nsFilterInstance.cpp:154
#20 0x00007fffe3fd2b3b in nsSVGIntegrationUtils::ComputePostEffectsVisualOverflowRect (aFrame=0x7fffc6c40d60, aPreEffectsOverflowRect=...) at /home/cjcool/repository/mozilla-central-debug/layout/svg/nsSVGIntegrationUtils.cpp:306
#21 0x00007fffe3e1f3f7 in ComputeEffectsRect (aFrame=0x7fffc6c40d60, aOverflowRect=..., aNewSize=...) at /home/cjcool/repository/mozilla-central-debug/layout/generic/nsFrame.cpp:6792
Comment on attachment 8894999 [details]
Bug 577824 - Part 1. Set the frame size in nsIFrame::FinishAndStoreOverflow before the ComputeEffectsRect call that uses it.

https://reviewboard.mozilla.org/r/166126/#review174510

::: commit-message-65507:1
(Diff revision 8)
> +Bug 577824 - Part 1. Mask sure the size of the filtered frame been set correctly before computing filter region.

This summary commit message doesn't give much of a clue to people reading m-c commit logs (say looking at the commits in a regression range and trying to figure out which they should look at) about which part of the Mozilla code is being touched. Make it "Part 1. Set the frame size in nsIFrame::FinishAndStoreOverflow before the ComputeEffectsRect call that uses it. r=jwatt". Mentioning nsIFrame::FinishAndStoreOverflow gives a better indication of what code is being touched.

All the rest of the very large expanded commit message is basically an explanation of why the bug that is being fixed doesn't show up in most cases, and detailing a specific case where it does show up. That seems to miss the main requirements of a good commit message (explaining *what* is changing and *why*) while providing a lot of detail that probably better belongs in the bug.

The summary line explains what we're changing, so we just need to explain why in the expanded message. I would change the expanded commit message to:

"""
This change ensures that HTML frames with SVG filters applied are given the
correct overflow regions so that they will display and invalidate correctly.

The bug that this commit fixes does not manifest in many cases since
often elements happen to be reflowed more than once when a document loads.
When that happened the bug would be masked because the filtered frame would be
given a size during the first reflow, and that would then make the overflow
calculations in subsequent reflows work.  It was only in cases where the
filtered frame was only reflowed once (such as when dynamically inserting a
filtered element into the DOM using script) that the lack of a valid frame
size during the overflow calculations would result in bad overflow regions
being calculated.
"""

::: commit-message-65507:3
(Diff revision 8)
> +Bug 577824 - Part 1. Mask sure the size of the filtered frame been set correctly before computing filter region.
> +
> +Here is the test case:

The commit comment should document what the change is doing and why. The testcase markup really doesn't belong in the commit comment, especially since you're actually commiting it as an actual reftest.

::: layout/generic/nsFrame.cpp:9061
(Diff revision 8)
> +  bool sizeChanged = ((aOldSize ? *aOldSize : oldSize) != aNewSize);
> +
> +  /* Since our size might not actually have been computed yet, we need to make sure that we use the
> +   * correct dimensions by overriding the stored bounding rectangle with the value the caller has
> +   * ensured us we'll use.
> +   */

This comment made a lot more sense when it was added a decade ago back in bug 435293. Since then the code has morphed enough that it really needs rewritten. While you're here, can you change the comment to something like:

  // Our frame size may not have been computed and set yet, but code under
  // functions such as ComputeEffectsRect (which we're about to call) use the
  // values that are stored in our frame rect to compute their results.  We
  // need the results from those functions to be based on the frame size that
  // we *will* have, so we temporarily set our frame size here before calling
  // those functions.
  //
  // XXX Someone should document here why we revert the frame size before we
  // return rather than just leaving it set.
Attachment #8894999 - Flags: review?(jwatt) → review+
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3b9a469ab1d6
Part 1. Set the frame size in nsIFrame::FinishAndStoreOverflow before the ComputeEffectsRect call that uses it. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/98f8d05483cf
Part 2. Add a reftest for an SVG filtered HTML element that's inserted into the DOM by script. r=jwatt
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e8059f9f0da2
Part 1. Set the frame size in nsIFrame::FinishAndStoreOverflow before the ComputeEffectsRect call that uses it. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/ce28b39e2dbc
Part 2. Add a reftest for an SVG filtered HTML element that's inserted into the DOM by script. r=jwatt
Flags: needinfo?(cku)
https://hg.mozilla.org/mozilla-central/rev/e8059f9f0da2
https://hg.mozilla.org/mozilla-central/rev/ce28b39e2dbc
Status: ASSIGNED → RESOLVED
Closed: 13 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
QA Whiteboard: [good first verify]
I have reproduced this bug with Nightly 20.0a1 (2012-12-07) on Ubuntu 16.04 LTS!

This bug's fix is verified with latest Beta!

Build   ID    20171009192146
User Agent    Mozilla/5.0 (X11; Linuxx86_64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [good first verify] → [good first verify] [bugday-20171011]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: