Closed Bug 1489653 Opened 6 years ago Closed 5 years ago

Support double-tap-to-zoom in Responsive Design Mode

Categories

(DevTools :: Responsive Design Mode, enhancement, P3)

enhancement

Tracking

(firefox76 fixed)

RESOLVED FIXED
Firefox 76
Tracking Status
firefox76 --- fixed

People

(Reporter: botond, Assigned: mtigley)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-needed, Whiteboard: [dt-q])

Attachments

(3 files, 2 obsolete files)

With bug 1290420 about to be fixed, one of the remaining issues that blocks testing a lot of mobile viewport compatibility issues in Responsive Design Mode is the ability simulate zooming in on a phone screen. Touch devices support two ways of triggering zooming: a pinch gesture, or a double tap. Pinch gestures might be tricky to simulate on desktop, but a double tap could easily be simulated via a double-click.
There's also one-touch-pinch as an additional way of triggering zooming.
Priority: -- → P3
Whiteboard: [dt-q]
Thinking about this a bit, it occurs to me that some of the issues that are holding us back from enabling pinch-zooming support on desktop, may crop up here too.
In particular, we'll need to either fix bug 1459312, or flip the layout.scroll.root-frame-containers pref in RDM as well. My preference would be the former.
Depends on: 1459312
No longer depends on: 1459312
Depends on: 1459312
Assignee: nobody → mtigley
Status: NEW → ASSIGNED

Simple test case: data:text/html,<div style="background-color:lightblue;height: 100px;width:100px">Double-tap this box<div>

Make sure that devtools.responsive.metaViewport.enabled pref is enabled.

Update:
Due to Fission work on Bug 1574885, there has been no progress on this. Once the Fission project for RDM starts to wrap up, I plan to revive work on this for the year, so let's keep this assigned.

WIP patch that utilizes windowUntils' sendNativeTouchTap to send touch events through the platform's gesture detection processing pipeline.

Attached file index.html (deleted) —

I've been using this to test the above patch.

Attachment #9127577 - Attachment description: Simulate double-tap gestures for in RDM → Simulate double-tap gestures in RDM

Needinfoing myself to look into why tapping on the grey box in the test case causes the compositor hit test to fail, leaving us with a null APZC here.

Flags: needinfo?(botond)

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

Needinfoing myself to look into why tapping on the grey box in the test case causes the compositor hit test to fail, leaving us with a null APZC here.

I haven't figured out the issue with the grey box yet, but I did discover another issue that affects clicking on any of the boxes, in cases where the device pixel ratio inside the RDM pane (which is determined by the device being emulated) is different from the device pixel ratio outside the RDM pane (which is determined by the system DPI and any full zoom level).

The calculation of window.mozInnerScreenX/Y uses the outer device pixel ratio, which means that when we convert mozInnerScreenX/Y to device pixels in our calculation of the coordinates to pass to sendNativeTouchTap(), we need to use the outer ratio as well.

I can't think of an obvious way to get the outer device ratio using existing APIs; I think we'll need to add a new nsIDOMWindowUtils API for it.

Even with the issue described in comment 9 fixed locally, double-clicking on the blue or green boxes does not work for me. However, we do get further -- we get as far as CalculateRectToZoomTo(), at which point the main-thread hit test fails to target the element being clicked on (it targets the document element instead).

Let's proceed with a fix for the issue described in comment 9, as it does make things better. The failure of the main-thread hit test will then require further investigation.

Depends on: 1617317

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

Let's proceed with a fix for the issue described in comment 9, as it does make things better.

Tracking this in bug 1617317.

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

Even with the issue described in comment 9 fixed locally, double-clicking on the blue or green boxes does not work for me. However, we do get further -- we get as far as CalculateRectToZoomTo(), at which point the main-thread hit test fails to target the element being clicked on (it targets the document element instead).

The failure of the main-thread hit test is also related to RDM's "override DPR".

As part of the double-tap mechanism, we round-trip the coordinates of the tap from CSS to screen pixels and back. The conversion from CSS to screen pixels happens in JS code in RDM (being added in this bug), and uses window.devicePixelRatio which takes into account the override. The conversion back from screen pixels to CSS pixels happens in BrowserChild:RecvHandleTap() and uses PresContext::CSSToDevPixelScale(), which does not take the override into account.

A naive attempt to modify PresContext::CSSToDevPixelScale() to take the RDM override into account does fix this, but regresses other things (such as the positioning of scrollbars in the RDM window).

Further experimentation has convinced me that the "override DPR" is actually ignored by pretty much everything, including for rendering purposes (i.e. content displayed in the RDM window is not scaled by the override DPR).

Making a further adjustment to the calculation in coordinatesRelativeToScreen() to ignore the override DPR completely makes things work correctly in terms of scales.

There is one additional issue with the coordinate calculations in the patch: the code in the patch is passing Event.clientX / Event.clientY as coordinates to coordinatesRelativeToScreen(). That function, as currently written, expects coordinates which are relative to the origin of the target element. However, clientX/clientY are relative to the layout viewport. As a result, the final coordinates end up containing the offset from the target element's origin to the layout viewport twice, which throws off the calculations for the green and grey squares. (For the blue square, the offset in question is only (8,8), so counting it twice tends not to make a difference.)

With that issue fixed as well, double-clicking on all three squares in the testcase works correctly for me.

The incoming coordinates, which come from Event.clientX/clientY, already
include this offset.

Depends on D63953

I've posted two patches which apply on top of https://phabricator.services.mozilla.com/D63335, and make adjustments to the calculations in coordinatesRelativeToScreen():

  • The first one works around the fact the RDM is not applying the override DPR, by also ignoring the override DPR in the calculations being added.
  • The second one fixes the issue related to passing in clientX/clientY coordinates described in comment 14.

With the patches in bug 1617317 + https://phabricator.services.mozilla.com/D63335 + these two tweaks, double-clicking on any of the three boxes (blue, green, or gray) correctly triggers a zoom-in animation for me.

Flags: needinfo?(botond)

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

With the patches in bug 1617317 + https://phabricator.services.mozilla.com/D63335 + these two tweaks, double-clicking on any of the three boxes (blue, green, or gray) correctly triggers a zoom-in animation for me.

Thanks Botond! I've updated https://phabricator.services.mozilla.com/D63335 with the changes you made. I'm also able to trigger a zoom-in animation for the three boxes at varying screen DPRs and zoom levels.

While I was testing this I noticed the zoom-in animation for the green and grey boxes seem incomplete. When I double-tap the blue box I get a full zoom animation, which isn't the case for the others.

Attached video incomplete_zoom.mp4 (deleted) —

Further investigation into the issue shown above is related to the code in RDM's touch-simulator code. Currently, sending the touch events follows immediately after the "dblclick" event. However, it seems there needs be a delay in between for the touch event to be registered correctly.

I'll update https://phabricator.services.mozilla.com/D63335 to have this delay between the mouse event and sending the native double events. Once this is done, this should fix the issue seen in Comment 19.

Attachment #9127577 - Attachment description: Simulate double-tap gestures in RDM → Bug 1489653 - Simulate double-tap gestures in RDM

Another issue I think needs to be addressed is ensuring that the coordinatesRelativeToScreen is using the correct values when a full-page zoom is applied to RDM. It seems the clientX / clientY and offsetX / offsetY values are being scaled with the current page zoom when performing a "zoom-out" animation. So we get this issue where the zoom animation does unexpected things.

Keywords: dev-doc-needed
Attachment #9127577 - Attachment description: Bug 1489653 - Simulate double-tap gestures in RDM → Bug 1489653 - Part 1: Simulate double-tap gestures in RDM
Blocks: 1282089
Attachment #9127577 - Attachment description: Bug 1489653 - Part 1: Simulate double-tap gestures in RDM → Bug 1489653 - Simulate double-tap gestures in RDM

Will create a follow-up bug that addresses testing for this feature.

Blocks: 1622094
Pushed by mtigley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c78f04bb9e27 Simulate double-tap gestures in RDM r=botond,bradwerth
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 76
Attachment #9128618 - Attachment is obsolete: true
Attachment #9128619 - Attachment is obsolete: true

Added a sentence about this to Using Responsive Design Mode, under the bullet about Touch Support.

Having a look at the code, this feature is behind the preference devtools.responsive.touchGestureSimulation.enabled and not enabled by default yet. So it should rather be added to the Experimental features in Firefox instead of Using Responsive Design Mode.

Sebastian

Flags: needinfo?(jswisher)

Thanks for the heads-up, :sebo! I reverted the change to "Using Responsive Design Mode", and added an item to the Developer Tools table in the Experimental Features article.

Flags: needinfo?(jswisher)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: