Closed Bug 1233468 Opened 5 years ago Closed 5 years ago

Canvas is not drawn at the correct viewport, and changes size after refresh and/or pan/zoom

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Version:
45 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
firefox45 --- affected
firefox46 --- fixed

People

(Reporter: padenot, Assigned: kats)

References

Details

Attachments

(1 file)

Patch
5.16 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review 
STR:
(1) Load https://paul.cx on Fennec (I'm testing current nightly)
(2) Refresh page
(3) Pan/zoom and refresh the page

Expected:
The full viewport is used each time

Actual:
At (1), only part of the viewport is used by the canvas
At (2), a bigger part of the viewport is used by the canvas
At (3), the whole viewport is used by the canvas

This might well be an APZ issue, I haven't investigated.
Prior to this push, we always used the full width, but not the full height: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=aa51330c9e0982791b7995c303339ee386ea4172&tochange=0720ea24b69a71909620ec6bb12f4ffbfe325d70

Prior to this push, we always just use the same small part of the viewport, and refreshing makes no difference: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=df9cdd1400626740fbc21f636aad8c284f0e9128&tochange=52d6ee1e55c922c78367528e8752fabe64ebab71

After that push I get pretty much the behaviour Paul is describing.

cc Kats
Sounds kind of like a race condition that gets tickled different ways after refresh. I can take this but probably won't get a chance to look into it until the new year.
Assignee: nobody → bugmail.mozilla
So I think the problem here is that the before-first-paint event (which determines the viewport size) is getting fired after the load event (which the page is using to determine the viewport size).

In general the site should probably have a resize listener than resizes the canvas to the new innerWidth/innerHeight. That would cause it to behave more correctly in the face of device rotations and browser window resizings, which it currently doesn't handle. However that would also paper over the real gecko bug in this case.

One possible fix on the gecko side would be to listen for load events and do the same thing that we do for before-first-paint events. As long as our load event listener gets run before the content's we should have the correct innerWidth/innerHeight in place. I'll give that a shot.
Attached patch Patch — — Splinter Review 
This does the job. We need to make sure we have the CSS viewport set up before content gets the load event, even if that happens before the before-first-paint event is fired (which is the case here).
Attachment #8706584 - Flags: review?(tnikkel)
status-firefox45: --- → affected
status-firefox46: --- → affected
Component: Canvas: 2D → Layout
Comment on attachment 8706584 [details] [diff] [review]
Patch

Seems fine.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> As long as our load
> event listener gets run before the content's we should have the correct
> innerWidth/innerHeight in place.

Hmm, is this guaranteed? Is there a way to guarantee it? I don't know how event listeners work in order to answer that. maybe smaug?
Attachment #8706584 - Flags: review?(tnikkel) → review+
I think it is guaranteed, because I'm using a capture chrome listener which gets run before everything else. And at any rate it will be be deterministic, so if it worked in my local testing it should work always.
https://hg.mozilla.org/mozilla-central/rev/a0355cdb6bdb
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.