Closed Bug 1034179 Opened 10 years ago Closed 10 years ago

GetOrMaybeCreateDisplayport uses the wrong composition bounds, results in a bad displayport

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33
blocking-b2g 2.0+
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v1.3 --- unaffected
b2g-v1.4 --- wontfix
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files)

Attached file Test page (deleted) —
STR: 1. In the B2G browser (Flame running 2.1/master) load the attached test page (taken from bug 1033383) 2. Tap on the text to put focus in the contenteditable field 3. Hit enter twice At this point, the contenteditable field becomes scrollable, and GetOrMaybeCreateDisplayport is called for it. This function generates a FrameMetrics and then computes a displayport for the scroll frame. The FrameMetrics has an incorrect composition bounds (compared to what gets generated later in RecordFrameMetrics for the same element), and this results in a display port bottom margin that is negative (-21.5510 pixels when I did this). This is wrong, because it leaves the critical displayport not covering the user-visible area. This later results in checkerboarding or low-res painting when it shouldn't. I'm not sure if GetOrMaybeCreateDisplayport should even be getting called for this element here; from what I remember that should only be happening on initial page load and not when elements are mutated. If the call is OK, then the composition bounds need to be fixed. The element has a height of 100 CSSPixels, which is 150 LayoutDevicePixels, and 49 LayerPixels. RecordFrameMetrics correctly sets a composition bounds that has height 49, but this code computes a composition bounds with height 150, so it's not taking the resolution factor into account properly.
Assignee: nobody → bugmail.mozilla
Every time content paints the first scrollable element we encounter will get a display port via GetOrMaybeCreateDisplayport, so that sounds expected based on how it is currently implemented. Although perhaps we might want to make that more restrictive based on loading of the page like you say.
Attached patch Fix resolution computation (deleted) — Splinter Review
In both RecordFrameMetrics (which this code is mostly a copy of) and in GetDisplayPortFromMarginsData we only use the local presshell's resolution if we're dealing with the root element. This change fixes the problem I was seeing.
Attachment #8450449 - Flags: review?(tnikkel)
Blocks a blocker, nominating for 2.0+
blocking-b2g: --- → 2.0?
No longer blocks: 1033383
Attachment #8450449 - Flags: review?(tnikkel) → review+
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
blocking-b2g: 2.0? → 2.0+
NO_UPLIFT until sufficiently baked on trunk per discussion with kats.
Whiteboard: [NO_UPLIFT]
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #0) > I'm not sure if GetOrMaybeCreateDisplayport should even be getting called > for this element here; from what I remember that should only be happening on > initial page load and not when elements are mutated. Our initial approach in bug 982141 was to only call GetOrMaybeCreateDisplayPort on first paint, but we ran into cases such as the Contacts app, where the contacts list is empty (and thus not scrollable) when the page loads, but is populated with contacts (and thus made scrollable) later, so we had to change our approach to call GetOrMaybeCreateDisplayPort on every paint (see https://bugzilla.mozilla.org/show_bug.cgi?id=982141#c15).
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: