Closed Bug 1746126 Opened 2 years ago Closed 1 year ago

Allow controlling zoom-to-focused-input behaviour using `user-scalable` and `touch-action`

Categories

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

Product:
Core
Component:
Panning and Zooming
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
111 Branch

People

(Reporter: botond, Assigned: rzvncj, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(2 files)

Bug 1746126 - Add zoom-to-focused-input mochitests for user-scalable={yes,no}. r?botond
48 bytes, text/x-phabricator-request
Details | Review
Bug 1746126 - Allow controlling zoom-to-focused-input behaviour using `touch-action`. r?botond
48 bytes, text/x-phabricator-request
Details | Review

As discussed in bug 1738696, we would like to allow website authors to disable zoom-to-focused-input behaviour on mobile using either of the following:

  • user-scalable=no in the meta viewport tag
  • touch-action: manipulation (and any other less permissive value of touch-action including none)

For now I'm marking this as a mentored bug, it's a good task for someone looking to learn the APZ code to pick up.

Doesn't this code suggest that the user-scalable=no case should already be handled correctly? Since the bug is still open I'm guessing it's not, which means I'm probably looking at the wrong piece of code. :)

(In reply to Razvan Cojocaru from comment #1)

Doesn't this code suggest that the user-scalable=no case should already be handled correctly? Since the bug is still open I'm guessing it's not, which means I'm probably looking at the wrong piece of code. :)

I think that code is at least potentially relevant!

I think a good place to start here is to check what is in mZoomConstraints when AsyncPanZoomController::ZoomToRect() is called when triggering zoom-to-focused-input behaviour on a page with user-scalable=no.

Do we have mMinZoom == mMaxZoom? If so, ZoomToRect should be a no-op (and if it's not in fact a no-op, we need to check the logic inside ZoomToRect).

If we don't have mMinZoom == mMaxZoom, then we should look more closely at the code you linked and see why not (the values should come from scaleMinFloat/scaleMaxFloat there).

If it turns out that there is a good reason why we can't have mMinZoom == mMaxZoom, then we could alternatively consider having ZoomToRect also respect mAllowZoom (and mAllowZoom should definitely be false with user-scalable=no).

I've had a few moments to look at this again today. Just to recap, you've suggested that we use a copy of this mochitest to debug this. I'm guessing that, in order to reproduce this, the test can be used as-is, except for setting maximum-scale to something else (2, for example)? However, doing so keeps allowing the test to pass, so it's likely that I'm missing something.

I'm running ./mach mochitest --setpref apz.subtest=helper_zoomToFocusedInput_nozoom2.html ./gfx/layers/apz/test/mochitest/test_group_zoomToFocusedInput.html (where helper_zoomToFocusedInput_nozoom2.html is the new test, with maximum-scale=2), and test_group_zoomToFocusedInput.html list of tests has been updated to include it.

Also, I'm guessing that the test (once correctly implemented) is supposed to only fail with an Android build, and not on desktop builds?

(In reply to Razvan Cojocaru from comment #3)

Just to recap, you've suggested that we use a copy of this mochitest to debug this. I'm guessing that, in order to reproduce this, the test can be used as-is, except for setting maximum-scale to something else (2, for example)? However, doing so keeps allowing the test to pass, so it's likely that I'm missing something.

It's possible some minor modifications to the test will be required, though I'm not sure exactly what without playing around with it.

Some things to check are:

  • Does AsyncPanZoomController::ZoomToRect() get called when running the test?
  • If so, what are the values of the starting zoom (Metrics().GetZoom()) and the computed target zoom (endZoomToMetrics.GetZoom() near the end of the function)?

Also, I'm guessing that the test (once correctly implemented) is supposed to only fail with an Android build, and not on desktop builds?

Good question!

The main thing the test relies on that is disabled by default on desktop, is processing of the <meta name="viewport"> tag (sometimes referred to as "mobile viewport sizing" in the code). This can be enabled on desktop by setting the pref dom.meta-viewport.enabled=true. This is technically an unsupported configuration, especially if non-overlay scrollbars are used (because mobile viewport sizing can have weird interactions with scrollbars that take up layout-space on the page; details can be found here if you're curious), and we probably wouldn't want to check this in, but it's probably fine for running the test locally in a desktop build for convenience.

(In reply to Botond Ballo [:botond] from comment #4)

  • Does AsyncPanZoomController::ZoomToRect() get called when running the test?
  • If so, what are the values of the starting zoom (Metrics().GetZoom()) and the computed target zoom (endZoomToMetrics.GetZoom() near the end of the function)?

(This is with the desktop build, with dom.meta-viewport.enabled=true.)

 0:13.11 GECKO(19478) helper_zoomToFocusedInput_nozoom2.html | WaitUntilApzStable: got apz-flush-done in child proc
 0:13.11 GECKO(19478) helper_zoomToFocusedInput_nozoom2.html | WaitUntilApzStable: done promiseFocus
 0:13.14 GECKO(19478) helper_zoomToFocusedInput_nozoom2.html | WaitUntilApzStable: done promiseAllPaintsDone
 0:13.14 GECKO(19478) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: Flushed APZ repaints, waiting for callback...
 0:13.15 GECKO(19478) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: APZ flush done
 0:13.15 GECKO(19478) helper_zoomToFocusedInput_nozoom2.html | WaitUntilApzStable: all done
 0:13.16 PASS helper_zoomToFocusedInput_nozoom2.html | The initial_resolution is 1, which is some sane value
 0:13.34 GECKO(19478) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: Flushed APZ repaints, waiting for callback...
 0:13.35 GECKO(19478) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: APZ flush done
 0:13.39 GECKO(19478) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: Flushed APZ repaints, waiting for callback...
 0:13.40 GECKO(19478) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: APZ flush done
 0:13.42 PASS helper_zoomToFocusedInput_nozoom2.html | focusing input did not change resolution 1
 0:13.42 GECKO(19478) void mozilla::layers::AsyncPanZoomController::ZoomToRect(const mozilla::layers::ZoomTarget &, const uint32_t)
 0:13.43 GECKO(19478) Metrics().GetZoom(): 1, endZoomToMetrics.GetZoom(): 1
 0:13.43 GECKO(19478) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: Flushed APZ repaints, waiting for callback...
 0:13.45 GECKO(19478) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: APZ flush done
 0:13.77 GECKO(19478) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: Flushed APZ repaints, waiting for callback...
 0:13.78 GECKO(19478) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: APZ flush done
 0:13.80 PASS helper_zoomToFocusedInput_nozoom2.html | zoomToFocusedInput input did not change resolution 1

diff --git a/gfx/layers/apz/src/AsyncPanZoomController.cpp b/gfx/layers/apz/src/AsyncPanZoomController.cpp
--- a/gfx/layers/apz/src/AsyncPanZoomController.cpp
+++ b/gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ -12,6 +12,8 @@
 #include <algorithm>    // for max, min
 #include <utility>      // for std::make_pair
 
+#include <iostream>
+
 #include "APZCTreeManager.h"            // for APZCTreeManager
 #include "AsyncPanZoomAnimation.h"      // for AsyncPanZoomAnimation
 #include "AutoDirWheelDeltaAdjuster.h"  // for APZAutoDirWheelDeltaAdjuster
@@ -5822,6 +5824,7 @@ APZCTreeManager* AsyncPanZoomController:
 
 void AsyncPanZoomController::ZoomToRect(const ZoomTarget& aZoomTarget,
                                         const uint32_t aFlags) {
+  std::cout << __PRETTY_FUNCTION__ << std::endl;
   CSSRect rect = aZoomTarget.targetRect;
   if (!rect.IsFinite()) {
     NS_WARNING("ZoomToRect got called with a non-finite rect; ignoring...");
@@ -6047,6 +6050,10 @@ void AsyncPanZoomController::ZoomToRect(
     endZoomToMetrics.SetVisualScrollOffset(rect.TopLeft());
     endZoomToMetrics.RecalculateLayoutViewportOffset();
 
+    std::cout << "Metrics().GetZoom(): " << Metrics().GetZoom()
+      << ", endZoomToMetrics.GetZoom(): " << endZoomToMetrics.GetZoom()
+      << std::endl;
+
     StartAnimation(new ZoomAnimation(
         *this, Metrics().GetVisualScrollOffset(), Metrics().GetZoom(),
         endZoomToMetrics.GetVisualScrollOffset(), endZoomToMetrics.GetZoom()));

Ok, so endZoomToMetrics.GetZoom() being 1 explains why no zooming occurs. Next, let's see how that comes about:

  • Is aZoomTarget.targetRect reasonable? If the focused element is an input field, I would expect it to be the bounding box of the input field.
  • If so, what prevents the target zoom from being something larger that would zoom in on that rect and centre it on the screen?
    • Perhaps mZoomConstraints.mMaxZoom fails to reflect the maximum-scale=2 for some reason?

(In reply to Botond Ballo [:botond] from comment #6)

Ok, so endZoomToMetrics.GetZoom() being 1 explains why no zooming occurs. Next, let's see how that comes about:

  • Is aZoomTarget.targetRect reasonable? If the focused element is an input field, I would expect it to be the bounding box of the input field.
  • If so, what prevents the target zoom from being something larger that would zoom in on that rect and centre it on the screen?
    • Perhaps mZoomConstraints.mMaxZoom fails to reflect the maximum-scale=2 for some reason?
 0:13.94 GECKO(48853) void mozilla::layers::AsyncPanZoomController::ZoomToRect(const mozilla::layers::ZoomTarget &, const uint32_t)
 0:13.94 GECKO(48853) mZoomConstraints.mMaxZoom: 10
 0:13.94 GECKO(48853) aZoomTarget.targetRect: (x=-7, y=8, w=1294, h=200)
 0:13.94 GECKO(48853) Metrics().GetZoom(): 1, endZoomToMetrics.GetZoom(): 1

Is mZoomConstraints.mMaxZoom supposed to be 10 going into ZoomToRect()? Is the 2 multiplied by a factor somewhere after it's read? My viewport tag is <meta name="viewport" content="width=device-width, height=device-height, initial-scale=1.0, minimum-scale=1, maximum-scale=2, user-scalable=no" />.

This is interesting, I had previously just set dom.meta-viewport.enabled to true manually in the browser and just ran the test (and mZoomConstraints.mMaxZoom was 10). I've now put this in test_group_zoomToFocusedInput.html: {"file": "helper_zoomToFocusedInput_nozoom2.html", "prefs": [["dom.meta-viewport.enabled", true]]},, and I get:

1:29.16 GECKO(3447) mZoomConstraints.mMaxZoom: 1

Neither case reflects the value in my test (maximum-scale=2). Quite confusing.

(In reply to Razvan Cojocaru from comment #8)

This is interesting, I had previously just set dom.meta-viewport.enabled to true manually in the browser and just ran the test (and mZoomConstraints.mMaxZoom was 10).

This doesn't work because prefs are stored in the profile directory, and ./mach mochitest creates a temporary profile directory in /tmp, while ./mach run uses a profile directory in the objdir which persists across runs; so prefs set during ./mach run will not be visible to ./mach mochitest.

So the test actually ran with dom.meta-viewport.enaled=false (the default), the meta viewport tag wasn't processed, and the default zoom constraints was used which has a max zoom of 10.

I've now put this in test_group_zoomToFocusedInput.html: {"file": "helper_zoomToFocusedInput_nozoom2.html", "prefs": [["dom.meta-viewport.enabled", true]]},,

Yep, this is the way to do it.

and I get:

1:29.16 GECKO(3447) mZoomConstraints.mMaxZoom: 1

Neither case reflects the value in my test (maximum-scale=2). Quite confusing.

I wonder if the user-scalable=no forces the max zoom to be clamped to 1. Does the max zoom change to 2 if you remove the user-scalable=no?

(In reply to Botond Ballo [:botond] from comment #9)

I've now put this in test_group_zoomToFocusedInput.html: {"file": "helper_zoomToFocusedInput_nozoom2.html", "prefs": [["dom.meta-viewport.enabled", true]]},,

Yep, this is the way to do it.

(An alternative way to do it as a one-off is ./mach mochitest --setpref pref=value.)

(In reply to Botond Ballo [:botond] from comment #9)

I wonder if the user-scalable=no forces the max zoom to be clamped to 1. Does the max zoom change to 2 if you remove the user-scalable=no?

It does apparently, yes. But the test still passes. And furthermore, isn't the test supposed to fail with user-scalable=no (isn't the issue about how user-scalable=no is not taken into account), or am I misunderstanding it?

 1:38.84 GECKO(5832) helper_zoomToFocusedInput_nozoom2.html | WaitUntilApzStable: got apz-flush-done in child proc
 1:38.85 GECKO(5832) helper_zoomToFocusedInput_nozoom2.html | WaitUntilApzStable: done promiseFocus
 1:38.86 GECKO(5832) helper_zoomToFocusedInput_nozoom2.html | WaitUntilApzStable: done promiseAllPaintsDone
 1:38.86 GECKO(5832) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: Flushed APZ repaints, waiting for callback...
 1:38.89 GECKO(5832) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: APZ flush done
 1:38.89 GECKO(5832) helper_zoomToFocusedInput_nozoom2.html | WaitUntilApzStable: all done
 1:39.09 PASS helper_zoomToFocusedInput_nozoom2.html | The initial_resolution is 1, which is some sane value
 1:39.72 GECKO(5832) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: Flushed APZ repaints, waiting for callback...
 1:39.72 GECKO(5832) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: APZ flush done
 1:39.76 GECKO(5832) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: Flushed APZ repaints, waiting for callback...
 1:39.77 GECKO(5832) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: APZ flush done
 1:39.79 PASS helper_zoomToFocusedInput_nozoom2.html | focusing input did not change resolution 1
 1:39.80 GECKO(5832) void mozilla::layers::AsyncPanZoomController::ZoomToRect(const mozilla::layers::ZoomTarget &, const uint32_t)
 1:39.80 GECKO(5832) mZoomConstraints.mMaxZoom: 2
 1:39.80 GECKO(5832) aZoomTarget.targetRect: (x=-7, y=8, w=1294, h=200)
 1:39.80 GECKO(5832) Metrics().GetZoom(): 1, endZoomToMetrics.GetZoom(): 1
 1:39.81 GECKO(5832) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: Flushed APZ repaints, waiting for callback...
 1:39.82 GECKO(5832) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: APZ flush done
 1:40.15 GECKO(5832) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: Flushed APZ repaints, waiting for callback...
 1:40.15 GECKO(5832) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: APZ flush done
 1:40.18 PASS helper_zoomToFocusedInput_nozoom2.html | zoomToFocusedInput input did not change resolution 1

(In reply to Razvan Cojocaru from comment #11)

(In reply to Botond Ballo [:botond] from comment #9)

I wonder if the user-scalable=no forces the max zoom to be clamped to 1. Does the max zoom change to 2 if you remove the user-scalable=no?

It does apparently, yes.

Interesting!

This is good news -- I believe it means that user-scalable=no is already working as intended (i.e. it successfully prevents zoom-to-focused-input).

I guess this part of the bug was filed based on a misunderstanding. (In bug 1738696, the reporter initially tried user-scalable=no alone. I suggested using maximum-scale=1 as a workaround, which also didn't work due to a regression. After we fixed the regression, I thought there was a remaining issue affecting user-scalable=no, but I guess it has been the same regression affecting that too.)

So, for this part of the bug, maybe we can just land a test that uses user-scalable=no and asserts that we don't zoom in?

(In addition, I believe the second part of the bug related to touch-action remains valid.)

But the test still passes.

This part we should investigate further -- the test that we land should cause zooming if the user-scalable=no is removed (otherwise, it's not actually testing that it's the presence of user-scalable=no that's preventing the zooming, not something else).

aZoomTarget.targetRect: (x=-7, y=8, w=1294, h=200)

This is pretty wide. The zoom-to-rect logic is mostly based on zooming in such that the target rect fills the width of the viewport, so if it's already very wide we may not zoom.

Looking at the test, the reason for the large width is the width:100%; on this line, in the input element's style. I guess this was necessary to test the scenario that helper_zoomToFocusedInput_nozoom.html was intended to exercise (which was more specific and related to the dynamic toolbar), but it's not necessary here -- we can probably just remove both the width and height and use an input element with the default size.

(In reply to Botond Ballo [:botond] from comment #13)

aZoomTarget.targetRect: (x=-7, y=8, w=1294, h=200)

This is pretty wide. The zoom-to-rect logic is mostly based on zooming in such that the target rect fills the width of the viewport, so if it's already very wide we may not zoom.

Looking at the test, the reason for the large width is the width:100%; on this line, in the input element's style. I guess this was necessary to test the scenario that helper_zoomToFocusedInput_nozoom.html was intended to exercise (which was more specific and related to the dynamic toolbar), but it's not necessary here -- we can probably just remove both the width and height and use an input element with the default size.

Still passing:

 0:12.70 PASS Starting subtest helper_zoomToFocusedInput_nozoom2.html
 0:13.38 GECKO(9003) WaitUntilApzStable: flushed APZ repaints in parent proc, waiting for callback...
 0:13.39 GECKO(9003) WaitUntilApzStable: APZ flush done in parent proc
 0:13.39 GECKO(9003) helper_zoomToFocusedInput_nozoom2.html | WaitUntilApzStable: got apz-flush-done in child proc
 0:13.39 GECKO(9003) helper_zoomToFocusedInput_nozoom2.html | WaitUntilApzStable: done promiseFocus
 0:13.41 GECKO(9003) helper_zoomToFocusedInput_nozoom2.html | WaitUntilApzStable: done promiseAllPaintsDone
 0:13.41 GECKO(9003) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: Flushed APZ repaints, waiting for callback...
 0:13.43 GECKO(9003) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: APZ flush done
 0:13.43 GECKO(9003) helper_zoomToFocusedInput_nozoom2.html | WaitUntilApzStable: all done
 0:13.45 PASS helper_zoomToFocusedInput_nozoom2.html | The initial_resolution is 1, which is some sane value
 0:13.51 GECKO(9003) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: Flushed APZ repaints, waiting for callback...
 0:13.62 GECKO(9003) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: APZ flush done
 0:13.65 GECKO(9003) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: Flushed APZ repaints, waiting for callback...
 0:13.67 GECKO(9003) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: APZ flush done
 0:13.69 PASS helper_zoomToFocusedInput_nozoom2.html | focusing input did not change resolution 1
 0:13.69 GECKO(9003) void mozilla::layers::AsyncPanZoomController::ZoomToRect(const mozilla::layers::ZoomTarget &, const uint32_t)
 0:13.69 GECKO(9003) mZoomConstraints.mMaxZoom: 2
 0:13.70 GECKO(9003) aZoomTarget.targetRect: (x=-7, y=8, w=222, h=29)
 0:13.70 GECKO(9003) Metrics().GetZoom(): 1, endZoomToMetrics.GetZoom(): 1
 0:13.70 GECKO(9003) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: Flushed APZ repaints, waiting for callback...
 0:13.72 GECKO(9003) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: APZ flush done
 0:14.04 GECKO(9003) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: Flushed APZ repaints, waiting for callback...
 0:14.05 GECKO(9003) helper_zoomToFocusedInput_nozoom2.html | PromiseApzRepaintsFlushed: APZ flush done
 0:14.06 PASS helper_zoomToFocusedInput_nozoom2.html | zoomToFocusedInput input did not change resolution 1
 0:14.08 GECKO(9003) MEMORY STAT vsizeMaxContiguous not supported in this build configuration.
 0:14.09 GECKO(9003) MEMORY STAT | vsize 2409MB | residentFast 164MB | heapAllocated 13MB
 0:14.21 TEST_END: Test OK. Subtests passed 9/9. Unexpected 0

The next step to investigate would be to step through the logic in ZoomToRect() to see why endZoomToMetrics.GetZoom() still ends up as 1. It's set based on the targetZoom variable here.

(In reply to Botond Ballo [:botond] from comment #15)

The next step to investigate would be to step through the logic in ZoomToRect() to see why endZoomToMetrics.GetZoom() still ends up as 1. It's set based on the targetZoom variable here.

It becomes 1 in the if (aFlags & PAN_INTO_VIEW_ONLY) branch here. Up until that point it's 5.95349. It's initially set here (the if (aFlags & DISABLE_ZOOM_OUT) branch is taken as well).

(In reply to Razvan Cojocaru from comment #16)

It becomes 1 in the if (aFlags & PAN_INTO_VIEW_ONLY) branch here.

Ah, interesting!

It turns out this is another artifact of running the test on desktop, which I didn't realize before. This flag is set here if the pref formhelper.autozoom is set to false, and this pref only defaults to true on mobile. So this is something else that needs to be flipped when running the test on desktop.

(In reply to Botond Ballo [:botond] from comment #17)

(In reply to Razvan Cojocaru from comment #16)

It becomes 1 in the if (aFlags & PAN_INTO_VIEW_ONLY) branch here.

Ah, interesting!

It turns out this is another artifact of running the test on desktop, which I didn't realize before. This flag is set here if the pref formhelper.autozoom is set to false, and this pref only defaults to true on mobile. So this is something else that needs to be flipped when running the test on desktop.

Right. Now it's getting set to 1 here.

zoomAtDefaultScale.scale appears to be 1, which is the same as currentZoom.scale, FWIW.

(In reply to Razvan Cojocaru from comment #18)

Now it's getting set to 1 here.

Looking at some code history to understand the purpose of those branches, it seems that in bug 1250614 we limited zoom-to-focused-input to observe a maximum zoom level of 1x.

So basically, zoom-to-focused-input cannot be used to zoom from e.g. 1x to 2x, only from e.g. 0.5x to 1x. (This is geared towards sites that are not optimized for mobile but loaded on mobile. Such sites are often laid out into a default viewport width of 980px, which is then scaled down into a narrower screen width e.g. 320px, resulting in an initial zoom level < 1.)

So, to successfully trigger zooming, we should change our meta-viewport parameters from initial-scale=1, minimum-scale=1, maximum-scale=2 to something like initial-scale=0.5, minimum-scale=0.5, maximum-scale=1.

I took a look at our other helper_zoomToFocusedInput_* tests to see if they are affected by this issue, and it turns out none of them are actually testing successful zooming in -- they're either testing that we don't zoom in, or just testing the scroll-into-view part of the behaviour.

If you're amenable, I think there would be value in adding two tests:

  • One without user-scalable=no, where we assert that we do zoom in
  • One with user-scalable=no, where we assert that we don't zoom in

Thanks! The test is now behaving as expected (with minimum-scale=0.5, maximum-scale=1). user-scalable=no continues to work properly, so I'm confirming that that particular problem has indeed already been solved and is no longer current.

Sure, no problem with adding two tests. I can probably just add that a separate first patch and already submit it later today (and push to try).

On second thought, shouldn't we just add a single test, the one that tests that we do zoom in with user-scalable=yes, and modify the helper_zoomToFocusedInput_nozoom.html test with minimum-scale=0.5, maximum-scale=1 etc. (since it doesn't appear to test anything now, if I understand this correctly), and also call it with "prefs": [["dom.meta-viewport.enabled", true], ["formhelper.autozoom", true]]?

(In reply to Razvan Cojocaru from comment #22)

and modify the helper_zoomToFocusedInput_nozoom.html [...] (since it doesn't appear to test anything now, if I understand this correctly)

helper_zoomToFocusedInput_nozoom.html is a regression test for bug 1738696, whose diagnosis is related to the "dynamic toolbar" (Android address bar that can be hidden by scrolling) creating a small discrepancy between two quantities that allowed zoom-to-focused-input to trigger a small amount of zooming in violation of maximum-scale.

That, in combination with this comment, leads me to believe that this test is testing something (fairly specific), even if we can't trigger the affected scenario very easily, and so we should leave it as is. We can rename it to helper_zoomToFocusedInput_nozoom_bug1738696.html to better reflect that it's testing a specific scenario, as opposed to a more generic "no zooming" scenario.

Also rename helper_zoomToFocusedInput_nozoom.html to
helper_zoomToFocusedInput_nozoom_bug1738696.html, since it
appears to test behaviour specific to bug 1738696.

Assignee: nobody → rzvncj
Status: NEW → ASSIGNED

Adding leave-open keyword so the merging of the test patch doesn't close the bug.

Keywords: leave-open
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1d4f355a9db6
Add zoom-to-focused-input mochitests for user-scalable={yes,no}. r=botond

So now that the first part of the bug (user-scalable=no) appears to have been covered, any specific pointers on how to approach the touch-action part (I'm inclined to modify the helper_zoomToFocusedInput_(no)zoom.html mochitests but remove the user-scalable part and see how touch-action works with them)? And specific parts of the code I should be looking at (so far I found this)?

(In reply to Razvan Cojocaru from comment #28)

any specific pointers on how to approach the touch-action part (I'm inclined to modify the helper_zoomToFocusedInput_(no)zoom.html mochitests but remove the user-scalable part and see how touch-action works with them)?

Yeah, writing a mochitest that fails is a good starting point.

The test can have a <div> with touch-action: none, and an <input> element inside it, and assert that zoom-to-focused-input on that input element does not zoom in.

For good measure, we can give the test another <div> with default touch-action (auto), and another <input> element inside that, and assert that there we do zoom in (to make sure that the presence of a touch-action: none element on the page does not interfere with the ability to zoom elsewhere).

And specific parts of the code I should be looking at (so far I found this)?

I think it's easiest to put this logic in the content side, in nsDOMWindowUtils::ZoomToFocusedInput(). Once we determine the focused element, we can check if the touch-action that applies to that element allows double-tap zoom. To do this, we can call the function GetAllowedTouchBehaviorForPoint() (we can expose this in TouchActionHelper.h) on the midpoint of the element's bounding rect, and check if the returned touch behaviour flags contain AllowedTouchBehavior::DOUBLE_TAP_ZOOM.

(In reply to Botond Ballo [:botond] from comment #29)

and check if the returned touch behaviour flags contain AllowedTouchBehavior::DOUBLE_TAP_ZOOM.

Adding a bit of context here since the relation to double-tap zoom may not be obvious:

When touch-action was standardized, the manipulation value was added to give page authors a way to allow touch gestures that directly manipulate the page content (i.e. cause it to move along with / in the direction of the fingers -- namely, panning and pinching) but not less direct behaviours like double-tap zoom (which are only allowed with auto).

Since double-tap zoom was the only behaviour in this second category at the time, we used DOUBLE_TAP_ZOOM as the name of the flag used to indicate that behaviours in this category should be allowed.

With this change, we will be adding zoom-to-focused-input as another behaviour in this category alongside double-tap zoom. Perhaps it would be appropriate to rename the flag from DOUBLE_TAP_ZOOM to something more general as well (though I don't have a great idea for a name at the moment).

Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c6bc2ad84f28
Allow controlling zoom-to-focused-input behaviour using `touch-action`. r=botond

Should we remove leave-open?

(In reply to Razvan Cojocaru from comment #34)

Should we remove leave-open?

Indeed, thanks for the reminder.

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 111 Branch

And thank you for fixing!

Keywords: leave-open

(In reply to Botond Ballo [:botond] from comment #36)

And thank you for fixing!

Happy to do it, thanks for the prompt and very helpful review!

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: