1931225 - Bail out from AsyncPanZoomController::ZoomToRect if the target zoom scale is same as the current scale in the case of zoom-to-focused-input

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P2)

Description

[Hiroyuki Ikezoe (:hiro)]

In AsyncPanZoomController::ZoomToRect, we do try centralizing the given rect. But in the case of zoom-to-focused-input and if the final zoom scale will be unchanged, it's inconsistent with what we do in nsDOMWIndowUtils::ZoomToFocusedInput which is called before the ZoomToRect.

In ZoomToFocusedInput we do call ScrollContentIntoView with WhereToScroll::Nearest, WhenToScroll::IfNotVisible, it's not supposed to centralize the element.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

It turned out that just skipping the centralizing is NOT sufficient to fix bug 1929516. ZoomToRect still tried to move the scroll offset.

After some more thought, given that ZoomToFocusedInput is supposed to scroll the given element to a proper position, if the final zoom scale will not be changed, we don't need do anything in ZoomToRect.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Things are not so straight-forward as usual. :/

For example, the fix for bug 1660932, which tried to scroll to the caret position, relies on the fact that ZoomToRect scrolls to the given position. The fix doesn't work if the given focused element is inside an iframe regardless of whether it's in out-of-process or not, I think. I think we should fix bug 1927600 first.

Another issue I notice is that with interactive-widget=resizes-visual or overlays-content, I guess ZoomToRect does ensure the given focused element into the visual viewport (or the visible area on overlays-content mode), without ZoomToRect the element will stay out of view.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)

Another issue I notice is that with interactive-widget=resizes-visual or overlays-content, I guess ZoomToRect does ensure the given focused element into the visual viewport (or the visible area on overlays-content mode), without ZoomToRect the element will stay out of view.

I confirmed this latter case doesn't matter at all. Phew. :)

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1931225 - Set "formhelper.autozoom" and "dom.meta-viewport.enabled" to true to make the tests zoomable on desktops. r?botond

With setting these prefs, even on desktops zoomToFocusedInput zooms in to the
focued element, it would be easier to debug the tests on desktop.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1931225 - Mimimize the zoom animation duration for zoom-to-focused-input tests. r?botond

With the minimized duraion, each test finishes quickly.

Without await promiseApzFlushedRepaints
helper_zoomToFocusedInput_zoom_in_position_fixed.html failed.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1931225 - Stop awaiting `TranformEnd` in zoom-to-focused-input tests. r?botond

In the next change, we stop zooming in to the focused element if we've already
zoomed in the contents. Thus we will never get TransformEnd notification. There
were two different type of use cases of TransformEnd.

a) No zoom happens, no scrolling happens
b) No zoom happens, but scrolling happens

For 1) awaiting TransformEnd is replaced by awaiting a scrollend event.
For 2) awaiting TransformEnd is replaced by awaiting a
promiseApzFlushedRepaints() call.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1931225 - Center the focused element as a part of scrollIntoView rather than doing it in APZ. r?botond

And skip creating a ZoomAnimation for zoom-to-focused-input if the final zoom
scale would be same as the current zoom scale.

A caveat here is that the scrollIntoView operation respects scroll-behavior
property, whereas our ZoomAnimation is always smooth, so that this change will
introduce a UX change in some cases.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0396fbf37102
Set "formhelper.autozoom" and "dom.meta-viewport.enabled" to true to make the tests zoomable on desktops. r=botond
https://hg.mozilla.org/integration/autoland/rev/457119ca702a
Mimimize the zoom animation duration for zoom-to-focused-input tests. r=botond
https://hg.mozilla.org/integration/autoland/rev/ced0a0b2a075
Stop awaiting `TranformEnd` in zoom-to-focused-input tests. r=botond
https://hg.mozilla.org/integration/autoland/rev/673e6bdd2393
Center the focused element as a part of scrollIntoView rather than doing it in APZ. r=botond
https://hg.mozilla.org/integration/autoland/rev/dbf0cf3a740b
apply code formatting via Lando

Comment 9

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/0396fbf37102
https://hg.mozilla.org/mozilla-central/rev/457119ca702a
https://hg.mozilla.org/mozilla-central/rev/ced0a0b2a075
https://hg.mozilla.org/mozilla-central/rev/673e6bdd2393
https://hg.mozilla.org/mozilla-central/rev/dbf0cf3a740b