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

RESOLVED FIXED in mozilla16

Status

()

Core
SVG
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: dbaron, Assigned: jwatt)

Tracking

Trunk
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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

7 years ago
Seems to be a similar effect to bug 460291
(Reporter)

Comment 2

7 years ago
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.
(Reporter)

Comment 3

7 years ago
In this case, the problem is probably one or more of the conversions in nsSVGIntegrationUtils.cpp.
(Reporter)

Comment 4

7 years ago
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.
Created attachment 499967 [details]
testcase 2 (reduced)
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).
Created attachment 499970 [details]
testcase 3 (reduced)
Attachment #499969 - Attachment is obsolete: true
Attachment #499969 - Attachment description: testcase 3 → (ignore; attached wrong testcase)

Updated

6 years ago
Duplicate of this bug: 700012
(Assignee)

Updated

5 years ago
Assignee: nobody → jwatt
(Assignee)

Comment 9

5 years ago
Created attachment 634564 [details] [diff] [review]
patch
Attachment #634564 - Flags: review?(dholbert)
(Assignee)

Comment 10

5 years ago
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?
(Assignee)

Comment 12

5 years ago
Created attachment 634622 [details] [diff] [review]
patch
Attachment #634564 - Attachment is obsolete: true
Attachment #634564 - Flags: review?(dholbert)
Attachment #634622 - Flags: review?(dholbert)
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

5 years ago
Oh, and I changed the parameters to be gfxRects instead on nsIntRects to avoid being misleading.
(Assignee)

Comment 15

5 years ago
Created attachment 634855 [details] [diff] [review]
patch

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+
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!" ;)
(Assignee)

Comment 18

5 years ago
Sorry, I screwed up the resolution of my merge conflict updating to tip before pushing. Should have rebuilt.
(Assignee)

Comment 19

5 years ago
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
Last Resolved: 5 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.
(Assignee)

Comment 22

5 years ago
Sweet, thanks, roc.
(Assignee)

Updated

5 years ago
Blocks: 614732
You need to log in before you can comment on or make changes to this bug.