Support double-tap-to-zoom in Responsive Design Mode
Categories
(DevTools :: Responsive Design Mode, enhancement, P3)
Tracking
(firefox76 fixed)
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)
Comment 1•6 years ago
|
||
Updated•6 years ago
|
Updated•6 years ago
|
Reporter | ||
Comment 2•6 years ago
|
||
Reporter | ||
Comment 3•6 years ago
|
||
Reporter | ||
Updated•6 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 4•5 years ago
|
||
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.
Assignee | ||
Comment 5•5 years ago
|
||
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.
Assignee | ||
Comment 6•5 years ago
|
||
WIP patch that utilizes windowUntils' sendNativeTouchTap
to send touch events through the platform's gesture detection processing pipeline.
Assignee | ||
Comment 7•5 years ago
|
||
I've been using this to test the above patch.
Updated•5 years ago
|
Reporter | ||
Comment 8•5 years ago
|
||
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.
Reporter | ||
Comment 9•5 years ago
|
||
(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.
Reporter | ||
Comment 10•5 years ago
|
||
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.
Reporter | ||
Comment 11•5 years ago
|
||
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.
Reporter | ||
Comment 12•5 years ago
|
||
(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.
Reporter | ||
Comment 13•5 years ago
|
||
(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).
Reporter | ||
Comment 14•5 years ago
|
||
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.
Reporter | ||
Comment 15•5 years ago
|
||
Reporter | ||
Comment 16•5 years ago
|
||
The incoming coordinates, which come from Event.clientX/clientY, already
include this offset.
Depends on D63953
Reporter | ||
Comment 17•5 years ago
|
||
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.
Assignee | ||
Comment 18•5 years ago
|
||
(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.
Assignee | ||
Comment 19•5 years ago
|
||
Assignee | ||
Comment 20•5 years ago
|
||
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.
Updated•5 years ago
|
Assignee | ||
Comment 21•5 years ago
|
||
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.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 22•5 years ago
|
||
Will create a follow-up bug that addresses testing for this feature.
Comment 23•5 years ago
|
||
Comment 24•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Updated•5 years ago
|
Comment 25•5 years ago
|
||
Added a sentence about this to Using Responsive Design Mode, under the bullet about Touch Support.
Comment 26•5 years ago
|
||
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
Comment 27•5 years ago
|
||
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.
Description
•