Can't zoom in some cases with native APZ

RESOLVED FIXED in Firefox 45

Status

()

Firefox for Android
Graphics, Panning and Zooming
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: snorp, Assigned: snorp)

Tracking

unspecified
Firefox 45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

With native APZ turned on, I'm unable to zoom after the following:

1) Open Fennec, navigate to nytimes.com
2) Ater load, open https://bluemarvin.github.io/html/iframe.html via adb

I can't zoom iframe.html, when I should in fact be able to.
Depends on: 1199798
Created attachment 8689686 [details] [diff] [review]
Fix some bustage from refactoring APZ tree traversal
Attachment #8689686 - Flags: review?(bugmail.mozilla)
Blocks: 1206872
Assignee: nobody → snorp
Comment on attachment 8689686 [details] [diff] [review]
Fix some bustage from refactoring APZ tree traversal

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

LGTM, but Botond should probably be the one to review it.
Attachment #8689686 - Flags: review?(bugmail.mozilla) → review?(botond)
Comment on attachment 8689686 [details] [diff] [review]
Fix some bustage from refactoring APZ tree traversal

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

Yeah, this part of the refactoring didn't preserve the semantics perfectly. My bad for not catching that during review!

However, this patch doesn't quite restore the original semantics, either.

  - Before the refactor, we would do the "is primary holder" check on every node, 
    and the "skip the subtree" check on every node except the starting node. 

  - The refactor changed it so we call the "skip the subtree" check on the starting node, 
    too, and since we do that before the "is primary holder" check, we can end up skipping 
    the entire subtree of the starting node, which is not desirable.

  - This patch changes it so that we do an extra "is primary holder" check on the starting
    node, but we still do t he "skip the subtree" check afterward so it can still fire.

I believe the correct solution here it to omit the "skip the subtree" check for the starting node. I can submit a patch that does this.
Attachment #8689686 - Flags: review?(botond) → review-
Created attachment 8689699 [details]
MozReview Request: Bug 1226320 - Fix a refactoring in APZCTreeManager that didn't preserve semantics correctly. r=kats

Bug 1226320 - Fix a refactoring in APZCTreeManager that didn't preserve semantics correctly. r=kats
Attachment #8689699 - Flags: review?(bugmail.mozilla)
Attachment #8689699 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8689699 [details]
MozReview Request: Bug 1226320 - Fix a refactoring in APZCTreeManager that didn't preserve semantics correctly. r=kats

https://reviewboard.mozilla.org/r/25693/#review23133

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/88359067c6e2
Blocks: 1199798
No longer depends on: 1199798

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/88359067c6e2
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
You need to log in before you can comment on or make changes to this bug.