Last Comment Bug 541270 - filter region for SVG filters on HTML content computed incorrectly when zoomed
: filter region for SVG filters on HTML content computed incorrectly when zoomed
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Jonathan Watt [:jwatt] (back in October - email directly if necessary)
:
Mentors:
: 700012 (view as bug list)
Depends on:
Blocks: 614732
  Show dependency treegraph
 
Reported: 2010-01-21 17:36 PST by David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
Modified: 2012-06-30 17:44 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (3.98 KB, application/xhtml+xml)
2010-01-21 17:36 PST, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details
testcase 2 (reduced) (600 bytes, application/xhtml+xml)
2010-12-27 18:16 PST, Daniel Holbert [:dholbert]
no flags Details
(ignore; attached wrong testcase) (306 bytes, application/vnd.mozilla.xul+xml)
2010-12-27 18:24 PST, Daniel Holbert [:dholbert]
no flags Details
testcase 3 (reduced) (750 bytes, application/xhtml+xml)
2010-12-27 18:24 PST, Daniel Holbert [:dholbert]
no flags Details
patch (2.88 KB, patch)
2012-06-19 13:27 PDT, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
no flags Details | Diff | Splinter Review
patch (10.20 KB, patch)
2012-06-19 15:29 PDT, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
no flags Details | Diff | Splinter Review
patch (12.32 KB, patch)
2012-06-20 05:13 PDT, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
dholbert: review+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-01-21 17:36:17 PST
Created attachment 422882 [details]
testcase

When zooming, the filter region appears to be computed incorrectly for SVG filters applied to HTML content.

Steps to reproduce:
 1. load attached testcase
 2. zoom out (so the text is smaller)
 3. zoom in (so the text is bigger than default)

Actual results:
 2. right and bottom sides of page are cut off
 3. extra scrollbars show up

Expected results:
 2 & 3: no scrollbars and whole page displayed

I'm seeing this in pretty current Minefield nightlies on both Windows and Linux.
Comment 1 Robert Longson 2010-01-22 05:34:44 PST
Seems to be a similar effect to bug 460291
Comment 2 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-01-22 07:52:58 PST
That may well be due to it being an easy mistake to make (making the wrong choice between CSS pixels per appunit vs. device pixels per appunit) than actually being the same bug.
Comment 3 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-01-27 14:47:10 PST
In this case, the problem is probably one or more of the conversions in nsSVGIntegrationUtils.cpp.
Comment 4 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-10-06 17:48:33 PDT
There are reftests checked in for this bug in layout/svg/reftests/:


svg-effects-area-unzoomed.xhtml
svg-effects-area-zoomed-in.xhtml
svg-effects-area-zoomed-out.xhtml

Prior to the changes in bug 542595, the second fails because of an incorrect scrollbar and the third fails because the area painted is too small.  Bug 542595 makes SVG effects no longer contribute to scrollable overflow, and therefore fixes the failure of the second test.  But the third still fails, and the underlying bug is still present.
Comment 5 Daniel Holbert [:dholbert] 2010-12-27 18:16:39 PST
Created attachment 499967 [details]
testcase 2 (reduced)
Comment 6 Daniel Holbert [:dholbert] 2010-12-27 18:24:16 PST
Created attachment 499969 [details]
(ignore; attached wrong testcase)

This version uses a red div, without a filter, that should be entirely covered up by the filtered div (but is not covered up when I zoom out).
Comment 7 Daniel Holbert [:dholbert] 2010-12-27 18:24:55 PST
Created attachment 499970 [details]
testcase 3 (reduced)
Comment 8 Robert Longson 2011-11-05 00:23:02 PDT
*** Bug 700012 has been marked as a duplicate of this bug. ***
Comment 9 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-06-19 13:27:41 PDT
Created attachment 634564 [details] [diff] [review]
patch
Comment 10 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-06-19 13:28:35 PDT
Basically, overrideBBox needs to be in CSS px, not dev pixels.
Comment 11 Daniel Holbert [:dholbert] 2012-06-19 13:39:24 PDT
Comment on attachment 634564 [details] [diff] [review]
patch

(In reply to Jonathan Watt [:jwatt] from comment #10)
> Basically, overrideBBox needs to be in CSS px, not dev pixels.

Ok -- then this comment (in the contextual code) needs an update, then, right?
>   // overrideBBox is in "user space", in dev pixels:
>   nsIntRect overrideBBox =
>     GetPreEffectsVisualOverflowUnion(firstFrame, aFrame,
>                                      aPreEffectsOverflowRect,
>                                      firstFrameToUserSpace).
>-      ToOutsidePixels(appUnitsPerDevPixel);
>+      ToOutsidePixels(aFrame->PresContext()->AppUnitsPerCSSPixel());

Also: we only use overrideBBox for this call to GetPostFilterBounds:
> 
>   nsRect overflowRect =
>     filterFrame->GetPostFilterBounds(firstFrame, &overrideBBox).
>-                   ToAppUnits(appUnitsPerDevPixel);
>+                   ToAppUnits(aFrame->PresContext()->AppUnitsPerDevPixel());

...and the documentation for that method says "The rects are in device pixels"
http://mxr.mozilla.org/mozilla-central/source/layout/svg/base/src/nsSVGFilterFrame.h#72

Is that no longer accurate?
Comment 12 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-06-19 15:29:00 PDT
Created attachment 634622 [details] [diff] [review]
patch
Comment 13 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-06-19 15:30:32 PDT
(In reply to Daniel Holbert [:dholbert] from comment #11)
> Ok -- then this comment (in the contextual code) needs an update, then,
> right?

Right. Done.

> Is that no longer accurate?

It never was, that's the problem. SVG bboxes are in user space, aka CSS px, and despite the comment, it was treating the passed in rect as such.
Comment 14 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-06-19 15:31:29 PDT
Oh, and I changed the parameters to be gfxRects instead on nsIntRects to avoid being misleading.
Comment 15 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-06-20 05:13:26 PDT
Created attachment 634855 [details] [diff] [review]
patch

Try found some more tests with bogus references that needed to be fixed.
Comment 16 Daniel Holbert [:dholbert] 2012-06-22 14:29:39 PDT
Comment on attachment 634855 [details] [diff] [review]
patch

Looks good!
Comment 17 Phil Ringnalda (:philor) 2012-06-22 19:14:06 PDT
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6c4b38bcdfc7 - the red builds in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&onlyunstarred=1&rev=12f76b9a2d78 say "Looks bad!" ;)
Comment 18 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-06-22 19:38:19 PDT
Sorry, I screwed up the resolution of my merge conflict updating to tip before pushing. Should have rebuilt.
Comment 19 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-06-22 20:05:18 PDT
The build log is not very clear about what went on, but it looks like the "unused variable" warning is being treated as an error, while only reporting it as a warning. I've fixed that and pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/010c24939cc5

As an aside, it would be really nice if compiler warning that are fatal on tinderbox were also fatal on local developer machines. That unused variable warning is not fatal for me on Mac. Hmm, maybe that's because I'm using clang?
Comment 20 Ryan VanderMeulen [:RyanVM] 2012-06-23 05:43:47 PDT
https://hg.mozilla.org/mozilla-central/rev/010c24939cc5
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-24 16:27:32 PDT
(In reply to Jonathan Watt [:jwatt] from comment #19)
> As an aside, it would be really nice if compiler warning that are fatal on
> tinderbox were also fatal on local developer machines. That unused variable
> warning is not fatal for me on Mac. Hmm, maybe that's because I'm using
> clang?

Add --enable-warnings-as-errors to your mozconfig.
Comment 22 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-06-25 17:51:31 PDT
Sweet, thanks, roc.

Note You need to log in before you can comment on or make changes to this bug.