615794 - Fix incorrect usage of ToNearestPixels in FrameLayerBuilder and elsewhere

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

In FrameLayerBuilder in a couple of places we do
  nsIntRect itemVisibleRect =
    aItem->GetVisibleRect().ToNearestPixels(appUnitsPerDevPixel);

This code is assuming that when the item draws, whatever it draws will be
snapped to pixel boundaries. But this assumption is overoptimistic for a couple
of reasons:
-- Some content, such as an element transformed by rotation, simply doesn't
snap its drawing.
-- If the context has a tricky transform on it when we come to draw the
content, we might not snap.

Using ToOutsidePixels here would be the conservative approach and it would give
correct results. However it would mean that for content that *does* snap, but
is not already aligned at pixel boundaries, the visible rect is bigger than the
area that actually gets drawn, so a layer containing only that content could no
longer be treated as opaque. That could be bad.

In BasicLayers we have a couple of places where we do
gfxUtils::ClipToRegionSnapped that have related issues.

Comment 1

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

Attachment: Part 1: Be conservative when converting visible regions and opaque regions from appunits to pixels

Comment 2

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

Attachment: Part 2: Remove some usage of gfxUtils::ClipToRegionSnapped since snapping the visible region may not be correct

Comment 3

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

Attachment: Part 3: Snap bounds of border and background display items to pixels if we're sure they will be snapped

The previous two patches are conservative and correct. But they can lead to regressions because we've lost information about what's snapped. When a display item draws a snapped rectangle, we can give it a smaller rectangle for the visible nsIntRegion, and a bigger rectangle for the opaque nsIntRegion. This patch adjusts GetBounds and GetOpaqueRegion to return the snapped region if we know the display item is not transformed.

The patch also modifies PresShell::RenderDocument so we tell nsDisplayListBuilder we're in a transform if we're being asked to draw into a context which has a transform (other than a straight transform by pixels).

Comment 4

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

Attachment: Part 1 v2

Fixes a regression in this patch. When converting a ThebesLayer to a ColorLayer we need to ensure that not only is the display item a uniform color inside its bounds, but that those bounds contain the entire visible rect of pixels for the item. If the bounds aren't pixel-aligned there might be a row or column of pixels that aren't fully covered.

Comment 5

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

Attachment: Part 4: mark tests as passing

Something in my patch queue --- I think the fixes in this bug --- makes the abspos-*.html transform tests pass with D2D.

Comment 6

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

Attachment: Part 5: Don't abort in RoundedRectIntersectsRect

Part 3 means the display item GetBounds is not always the frame border-box, so occasionally we can get an abort in RoundedRectIntersectsRect. We should just return false there instead.

Comment 7

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

Comment on attachment 496318 [details] [diff] [review]
Part 5: Don't abort in RoundedRectIntersectsRect

r=dbaron

Comment 8

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

Blocks a blocker, and fixes regressions too.

Comment 9

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

Attachment: Part 3 v2

Snap nsDisplayClip too

Comment 10

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

Attachment: Part 1.5: Fix drawing of mForcedBackgroundColor to only fill the bounds of the visible region

It's not correct to completely fill the destination context with the
mForcedBackgroundColor. FindOpaqueBackgroundColorFor only checks that
the bounds of the layer's visible region can be filled with the color.
Drawing outside the bounds of the visible region may not necessarily
be ignored (see comments on Layer::SetVisibleRegion).

(This is an existing bug revealed by part 2.)

Comment 11

[Timothy Nikkel (:tnikkel)]

Comment on attachment 494909 [details] [diff] [review]
Part 3: Snap bounds of border and background display items to pixels if we're sure they will be snapped

I assume this is obsolete now?

Comment 12

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

Comment on attachment 494909 [details] [diff] [review]
Part 3: Snap bounds of border and background display items to pixels if we're sure they will be snapped

Yes

Comment 13

[Timothy Nikkel (:tnikkel)]

Could you push your patch queue to your users repo? I will probably want to look at FrameLayerBuilder in the context of your other pending changes.

Comment 14

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

Done.

Comment 15

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

Blocks 579808, which is a regression.

Comment 16

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

A tryserver build with these patches was green (modulo known random orange) on a tryserver build with "-b d -p all -t none -u all":
http://hg.mozilla.org/try/pushloghtml?changeset=b3f754733e39
I think this is safe to land.

Comment 17

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/a955dcb369f6
http://hg.mozilla.org/mozilla-central/rev/9fb07b18a3a5
http://hg.mozilla.org/mozilla-central/rev/bf71941d1aee
http://hg.mozilla.org/mozilla-central/rev/1ca912846884
http://hg.mozilla.org/mozilla-central/rev/a61970a01de4
http://hg.mozilla.org/mozilla-central/rev/ca890a13c598

Comment 18

[Ilja Sekler]

Something in Parts 1.5 - 5 has caused <https://bugzilla.mozilla.org/show_bug.cgi?id=363861#c145>, while Part 1 alone fixes Bug 598575. Would you like to treat this in a new bug or would you like to investigate the problem here?

Comment 19

[Ilja Sekler]

(In reply to comment #18)
> Something in Parts 1.5 - 5 has caused
> <https://bugzilla.mozilla.org/show_bug.cgi?id=363861#c145>

While backout of these checkins helped when printing to PDF my own reduced testcase, this didn't restore printing of semi-transparent areas of a real-life case <http://weblogs.mozillazine.org/asa/archives/2010/12/visualizing_plugin_m.html>. Even if these patches contributed to the new problem, they are not alone responsible.

My apologies.