1647735 - When current simulated viewport is larger than the browser screen, items cannot be clicked successfully with Touch Simulation activated

Status: VERIFIED FIXED

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

Description

[Clara Guerrero ( Need Info Brindusa Tot Please)]

Attachment: touch simulation on resize.mov

[Affected versions]:
Nightly 79.0a1 (2020-06-22)

[Affected platforms]:
MacOs 10.14
Win 10 pro

[Preconditions]
In about:config,
set devtools.responsive.metaViewport.enabled = true
set devtools.responsive.browserUI.enabled = true

Steps :

1- Launch the Firefox browser
2- Reach wikipedia.
3- Hit Cmd+option+M in order to start Responsive Design Mode, have both "reload when touch simulation is toggled" CHECKED, and Enable Touch Simulation.
4- Enlarge current viewport, so that it is bigger than browser screen.
5- click for example items listed at the left of the screen

Expected Results :
items should be clickable both with touch simulation on and off

Actual Results :
items cannot be clicked successfully with Touch Simulation activated

note: items can be clicked if deactivating touch simulation (related to https://bugzilla.mozilla.org/show_bug.cgi?id=1625999)

Comment 1

[Micah [:mtigley] (she/her)]

Thank you for reporting this! I am also able to reproduce the issue when devtools.responsive.browserUI.enabled = false on Firefox 79 Nightly. So I don't think it is specific to the new browser UI for RDM.

Using mozgression GUI, I was able to narrow down the problem to Bug 1631568. So we can start investigating the issue based on the work there.

Comment 2

[Micah [:mtigley] (she/her)]

I should also note that RDM currently has issues transforming touch points, so the issue here can be a combination of what was introduced in Bug 1631568 and fixing the transformed touch points in RDM.

Comment 3

[Botond Ballo [:botond]]

I believe this is the same underlying issue as bug 1637135.

In that bug, I described potential solution options in bug 1637135 comment 20. We went with option (1), and I did note that this option would leave the bug unfixed for RDM if both of the following are true:

• the page being viewed does not use a meta-viewport tag with width=device-width (and therefore a fallback ICB size of 980px gets used)
• the width of the simulated device window in RDM is larger than 980px

I said at the time that I expect this combination to be uncommon. If it's turning out that this combination is in fact somewhat common, we can explore other solution options (notably (2)) from that comment.

Comment 4

[Micah [:mtigley] (she/her)]

Thanks Botond for weighing in on this! If this issue is closely related to what Bug 1637135 details, then I'd like to try to have RDM's touch simulation imitate the experience of a real mobile phone using GeckoView as much as possible. I briefly looked over your second approach at https://bugzilla.mozilla.org/show_bug.cgi?id=1637135#c20:

2. Where a page does not explicitly specify an ICB width via a meta viewport tag, change our fallback ICB width from 980px to max(980px, screen width).
   • This would prevent the problematic frame trees from arising in the first place, and fix this bug.
   • There is some evidence that Chrome does something similar, namely they use a fallback ICB width of 1280px on some form factors.
   • While I think this change is unlikely to have adverse effects, it's still a behaviour change with the possibility of unintended consequences, making this a riskier approach than (1).
   • (Pages that explicitly set an ICB width narrower than the screen width, e.g. that put width=980 in the meta viewport tag on a 1280px width device, would continue to be broken. However, I expect such pages are very rare.)

To your third point, I wonder if we could just add another check for if RDM is active to avoid introducing new bugs if we decide to change the fallback ICB width to max(980px, screen width). Would that be enough?

Comment 5

[Botond Ballo [:botond]]

(In reply to Micah Tigley [:mtigley] from comment #4)

To your third point, I wonder if we could just add another check for if RDM is active to avoid introducing new bugs if we decide to change the fallback ICB width to max(980px, screen width). Would that be enough?

Yes, that would be a good way to mitigate the risk of this change. If we don't notice any issues with it after the change to RDM hits release, we can apply the change for actual mobile browsing as well (so that the two remain consistent).

If you'd like to give this a try, I believe the relevant code is around here with browser_viewport_desktopWidth() being the preference where the 980px comes from, and displaySize.width being the "screen width" from the above description.

Comment 6

[Micah [:mtigley] (she/her)]

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

If you'd like to give this a try, I believe the relevant code is around here with browser_viewport_desktopWidth() being the preference where the 980px comes from, and displaySize.width being the "screen width" from the above description.

Thanks Botond! I'll give this a try.

Comment 7

[Micah [:mtigley] (she/her)]

Attachment: Bug 1647735 - In RDM, ICB fallback width should be the simulated device's width if larger than the desktop viewport default. r=botond

Comment 8

[Micah [:mtigley] (she/her)]

Attachment: Bug 1647735 - Add a test for checking that ICB fallback width uses the larger viewport width. r=bradwerth

Depends on D82311

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb88c365b828
In RDM, ICB fallback width should be the simulated device's width if larger than the desktop viewport default. r=botond
https://hg.mozilla.org/integration/autoland/rev/deb2f679f4fb
Add a test for checking that ICB fallback width uses the larger viewport width. r=bradwerth

Comment 10

[Razvan Maries]

Backed out for perma failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=deb2f679f4fbfdd5a251cddf58fedfdebe621045&failure_classification_id=2

Logs:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=309151475&repo=autoland&lineNumber=9604
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=309151724&repo=autoland&lineNumber=1377
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=309151099&repo=autoland&lineNumber=1603
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=309151120&repo=autoland&lineNumber=1984
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=309151816&repo=autoland&lineNumber=8749

Backout: https://hg.mozilla.org/integration/autoland/rev/36ee3f87d10ef445a406e7f84dbdea6cf05d280d

Comment 11

[Botond Ballo [:botond]]

Looks like the failures are segfaults at the following location:

libxul.so!mozilla::dom::Document::GetViewportInfo(mozilla::gfx::IntSizeTyped<mozilla::ScreenPixel> const&) [nsIDocShell.h: : 679 + 0x0]

It looks like there is some inlining happening (since the function name and file name do not match) which obscures the exact location of the crash, but my guess is that GetDocShell() is returning null which then causes the call to GetTouchEventsOverride() to crash. Adding a null check to GetDocShell() should fix it.

Comment 12

[Micah [:mtigley] (she/her)]

I've updated https://phabricator.services.mozilla.com/D82311 to do a null check on GetDocShell() as Botond suggested. I'll attempt to land again after my Try run finishes.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc88ab9001b8
In RDM, ICB fallback width should be the simulated device's width if larger than the desktop viewport default. r=botond
https://hg.mozilla.org/integration/autoland/rev/274ca4f49646
Add a test for checking that ICB fallback width uses the larger viewport width. r=bradwerth

Comment 14

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/dc88ab9001b8
https://hg.mozilla.org/mozilla-central/rev/274ca4f49646

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:mtigley, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 16

[Micah [:mtigley] (she/her)]

Comment on attachment 9161513 [details]
Bug 1647735 - In RDM, ICB fallback width should be the simulated device's width if larger than the desktop viewport default. r=botond

Beta/Release Uplift Approval Request

• User impact if declined: Touchpoints will not be registered for simulated screen widths greater than 980px when touch simulation is enabled in Responsive Design Mode.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's a minor change to the ICB fallback width, which is only applied when Responsive Design Mode is active.
• String changes made/needed:

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9161513 [details]
Bug 1647735 - In RDM, ICB fallback width should be the simulated device's width if larger than the desktop viewport default. r=botond

RDM-only bugfix w/ a new test. Approved for 79.0b8.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d1397dcc127d
https://hg.mozilla.org/releases/mozilla-beta/rev/aba0e5e650d7

Comment 19

[Clara Guerrero ( Need Info Brindusa Tot Please)]

Hi,
This issue is no longer reproducible in beta 79.0b8 (64-bit) nor nightly 80.0a1 (2020-07-16) (64-bit)
I'm updating flags accordingly.
Best,
Clara