1034258 - Rounding of composition bounds causes incorrect displayport margin computation

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: Log snippet

Use the STR at [1], and then examine the displayport that is being used by layout for the contenteditable scrollable frame (using logging or gdb or whatever). I added some local logging, it is attached.

In the last line of the log, you can see that the bottom margin is negative 0.020407. This ends up making the critical displayport shorter than it should be, resulting in checkerboarding. However if you look at the first line of the log, the APZC computes a displayport (in CSS pixels) that stretches from y=-44 with a height of 144. The composition bounds is 49 layer pixels tall at a zoom of 0.490, which makes it 100 CSS pixels tall. The scroll position is at y=44 CSS pixels, and the scrollable rect is 144 CSS pixels tall. Basically, the first line indicates the displayport should cover the entire scrollable rect and composition bounds, but the last line sets up a value so that it doesn't.

The reason this happens is because the zoom isn't exactly 0.49, it's (480/980) == 0.48979591836735. Which means the composition bounds should be 100*0.48979591836735 = 48.979591836735 parent layer pixels, rather than the rounded 49 parent layer pixels. Therefore, when CalculatePendingDisplayPort calculates the compositionSize variable, it converts the 49 layer pixels back into 100.04166666666604 CSS pixels rather than 100 CSS pixels, introducing a rounding error 0.0416 CSS pixels. This can be seen in line 2 of the log output.

The proper fix here is probably to change mCompositionBounds to be a ParentLayerRect rather than a ParentLayerIntRect, so that it is more precise. I'll write up a patch that does that, but it might be too risky to uplift to 2.0. I might have to find some other hacky way to fix this for 2.0.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1034179#c0

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Convert mCompositionBounds to ParentLayerRect

This fixes the problem in comment 0. Not sure yet if it breaks other things.

Comment 2

[Chris Lord [:cwiiis]]

Comment on attachment 8450491 [details] [diff] [review]
Convert mCompositionBounds to ParentLayerRect

Review of attachment 8450491 [details] [diff] [review]:
-----------------------------------------------------------------

Pre-emptive r+, looked over it and I don't see anything wrong at least.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Local B2G and Linux builds work, gtests pass. Try push: https://tbpl.mozilla.org/?tree=Try&rev=999e49a94a0e

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Blocks a blocker, so it should be 2.0+. However, I'm not sure if we want a smaller fix to uplift to 2.0, so flagging as NO_UPLIFT for now.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Try is green enough, landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/3743891048ce

Comment 6

[Kartikaya Gupta (email:[e-mail])]

I rebased this patch for aurora (only one hunk needed manual fixup), did a build-only try push [1], and did some local testing on B2G. I think it's actually not that bad to uplift. Leaving NO_UPLIFT for the moment because I'd like it to bake on m-c for a bit but I won't spend time coming up with a branch-specific smaller fix unless this patch breaks things.

[1] https://tbpl.mozilla.org/?tree=Try&rev=ee66aa269456

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/3743891048ce

Comment 8

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7300535dc196