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

RESOLVED FIXED in mozilla16

Status

()

defect
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: dbaron, Assigned: jwatt)

Tracking

Trunk
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

Posted file 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.
Seems to be a similar effect to bug 460291
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.
In this case, the problem is probably one or more of the conversions in nsSVGIntegrationUtils.cpp.
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.
Posted file (ignore; attached wrong testcase) (obsolete) —
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).
Attachment #499969 - Attachment is obsolete: true
Attachment #499969 - Attachment description: testcase 3 → (ignore; attached wrong testcase)
Duplicate of this bug: 700012
Assignee: nobody → jwatt
Posted patch patch (obsolete) — Splinter Review
Attachment #634564 - Flags: review?(dholbert)
Basically, overrideBBox needs to be in CSS px, not dev pixels.
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?
Posted patch patch (obsolete) — Splinter Review
Attachment #634564 - Attachment is obsolete: true
Attachment #634564 - Flags: review?(dholbert)
Attachment #634622 - Flags: review?(dholbert)
(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.
Oh, and I changed the parameters to be gfxRects instead on nsIntRects to avoid being misleading.
Posted patch patchSplinter Review
Try found some more tests with bogus references that needed to be fixed.
Attachment #634622 - Attachment is obsolete: true
Attachment #634622 - Flags: review?(dholbert)
Attachment #634855 - Flags: review?(dholbert)
Comment on attachment 634855 [details] [diff] [review]
patch

Looks good!
Attachment #634855 - Flags: review?(dholbert) → review+
Sorry, I screwed up the resolution of my merge conflict updating to tip before pushing. Should have rebuilt.
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?
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/010c24939cc5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(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.
Sweet, thanks, roc.
Blocks: 614732
You need to log in before you can comment on or make changes to this bug.