Closed Bug 1241332 Opened 8 years ago Closed 8 years ago

Panning and zooming an input field when it gains focus has too much latency in Fennec

Categories

(Core :: Panning and Zooming, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- wontfix
firefox47 --- fixed

People

(Reporter: rbarker, Assigned: rbarker)

References

(Depends on 1 open bug)

Details

(Keywords: polish, Whiteboard: [gfx-noted])

Attachments

(2 files, 7 obsolete files)

1.24 KB, patch
kats
: review+
Details | Diff | Splinter Review
2.88 KB, patch
Details | Diff | Splinter Review
Fennec uses the APZC function ZoomToRect to ensure a focused input element is visible in the window (see bug 1231517). Currently it is using a timeout to wait for the window resize to propagate to the compositor thread. The attached patch was an attempt to flush APZ to ensure the updated frame metrics had propagated to the compositor thread. Unfortunately, it would intermittently fail. Examining the frame metrics in the APZC revealed the composition bounds had not yet been updated when the call failed.
Depends on: 1231517
Assignee: nobody → rbarker
Based on what we do in testing code I think what needs to happen here is that you need to wait for (1) the resize event, (2) any pending paints, and (3) the apz-flush. Once all three have completed the APZC should have the updated composition bounds. And actually if the zoomToRect call goes via the compositor thread then you don't need to wait for (3) because it will implicitly follow (2).
Keywords: polish
Whiteboard: [gfx-noted]
I tested this out today on my 6P, Nightly. 

The delay between A) user touches input field and B) input field is fully zoomed into view is really long right now. I think we need to fix this sooner rather than later because it feels kind of broken to have the feedback for a basic touch be so lagged behind. :)
Flags: needinfo?(rbarker)
(In reply to Anthony Lam (:antlam) from comment #2)
> I tested this out today on my 6P, Nightly. 
> 
> The delay between A) user touches input field and B) input field is fully
> zoomed into view is really long right now. I think we need to fix this
> sooner rather than later because it feels kind of broken to have the
> feedback for a basic touch be so lagged behind. :)

I will attempt the fix outlined in comment #1 when I get a chance. Right now the feature works (not ideally) and we have a number features that are busted that need more urgent attention.
Flags: needinfo?(rbarker)
Summary: calling nsDomWindowUtils.flushApzRepaints() after a window resize does not guarantee the frame metrics will have finished propagating in C++APZ → Panning and zooming a input field when it gains focus has too much latency in Fennec
Summary: Panning and zooming a input field when it gains focus has too much latency in Fennec → Panning and zooming an input field when it gains focus has too much latency in Fennec
Depends on: 1253426
Attachment #8726423 - Flags: review?(bugmail.mozilla)
Attachment #8726423 - Flags: review?(bugmail.mozilla) → review+
Attachment #8726498 - Flags: review?(bugmail.mozilla)
Blocks: 1253469
Comment on attachment 8726498 [details] [diff] [review]
0002-Bug-1241332-part-2-Request-zoomToFocusedInput-after-resize-finishes-and-propagates-through-APZ-r-16030316-1e25225.patch

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

::: mobile/android/chrome/content/browser.js
@@ +1564,5 @@
> +      };
> +
> +      let apzFlushDone = function() {
> +        Services.obs.removeObserver(apzFlushDone, "apz-repaints-flushed", false);
> +        setTimeout(focusInput, 100);

What's this setTimeout for? Can you just call dwu.zoomToFocusedInput() directly?
I think that 0 timeout was left over from the original patch when I was trying to figure out how to ensure the resize had completely propagated.
Attachment #8726498 - Attachment is obsolete: true
Attachment #8726498 - Flags: review?(bugmail.mozilla)
Attachment #8726768 - Flags: review?(bugmail.mozilla)
Comment on attachment 8726768 [details] [diff] [review]
0002-Bug-1241332-part-2-Request-zoomToFocusedInput-after-resize-finishes-and-propagates-through-APZ-r-16030407-bf01cba.patch

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

::: mobile/android/chrome/content/browser.js
@@ +1582,5 @@
> +          paintDone();
> +        }
> +      }
> +
> +      let gotResizeWindow = false;

I'd prefer moving this declaration up to before the resizeWindow declaration.
Attachment #8726768 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/409f7a0e990f
https://hg.mozilla.org/mozilla-central/rev/953545615bdc
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.