541270 - filter region for SVG filters on HTML content computed incorrectly when zoomed

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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 [:longsonr]]

Seems to be a similar effect to bug 460291

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

In this case, the problem is probably one or more of the conversions in nsSVGIntegrationUtils.cpp.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Attachment: testcase 2 (reduced)

Comment 6

[Daniel Holbert [:dholbert]]

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

Attachment: testcase 3 (reduced)

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 10

[Jonathan Watt [:jwatt]]

Basically, overrideBBox needs to be in CSS px, not dev pixels.

Comment 11

[Daniel Holbert [:dholbert]]

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

Attachment: patch

Comment 13

[Jonathan Watt [:jwatt]]

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

Oh, and I changed the parameters to be gfxRects instead on nsIntRects to avoid being misleading.

Comment 15

[Jonathan Watt [:jwatt]]

Attachment: patch

Try found some more tests with bogus references that needed to be fixed.

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 634855 [details] [diff] [review]
patch

Looks good!

Comment 17

[Phil Ringnalda (:philor)]

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

Sorry, I screwed up the resolution of my merge conflict updating to tip before pushing. Should have rebuilt.

Comment 19

[Jonathan Watt [:jwatt]]

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

https://hg.mozilla.org/mozilla-central/rev/010c24939cc5

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Sweet, thanks, roc.