Rounding of composition bounds causes incorrect displayport margin computation

RESOLVED FIXED in Firefox 32, Firefox OS v2.0

Status

()

Core
Panning and Zooming
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
mozilla33
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox30 wontfix, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v1.3 wontfix, b2g-v1.4 wontfix, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

Attachments

(2 attachments)

Created attachment 8450484 [details]
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
Created attachment 8450491 [details] [diff] [review]
Convert mCompositionBounds to ParentLayerRect

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

Comment 2

4 years ago
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.
Attachment #8450491 - Flags: review+
Local B2G and Linux builds work, gtests pass. Try push: https://tbpl.mozilla.org/?tree=Try&rev=999e49a94a0e
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.
blocking-b2g: --- → 2.0?
Whiteboard: [NO_UPLIFT]
status-b2g-v1.3: --- → wontfix
status-b2g-v1.4: --- → wontfix
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
status-firefox30: --- → wontfix
status-firefox31: --- → wontfix
status-firefox32: --- → affected
status-firefox33: --- → affected

Updated

4 years ago
blocking-b2g: 2.0? → 2.0+
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
https://hg.mozilla.org/mozilla-central/rev/3743891048ce
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
status-b2g-v2.1: affected → fixed
status-firefox33: affected → fixed
https://hg.mozilla.org/releases/mozilla-aurora/rev/7300535dc196
status-b2g-v2.0: affected → fixed
status-firefox32: affected → fixed
Whiteboard: [NO_UPLIFT]
You need to log in before you can comment on or make changes to this bug.