Closed Bug 1647735 Opened 3 months ago Closed 2 months ago

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

Categories

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

defect

Tracking

(firefox-esr68 unaffected, firefox-esr78 wontfix, firefox77 disabled, firefox78 wontfix, firefox79 verified, firefox80 verified)

VERIFIED FIXED
Firefox 80
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- wontfix
firefox77 --- disabled
firefox78 --- wontfix
firefox79 --- verified
firefox80 --- verified

People

(Reporter: clara.guerrero, Assigned: mtigley)

References

(Blocks 4 open bugs, Regressed 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(3 files)

[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)

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.

Severity: -- → S3
Priority: -- → P3
Regressed by: 1631568

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.

Blocks: rdm-touch

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.

See Also: → 1637135

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:

  1. 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?

(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.

(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.

Assignee: nobody → mtigley
Status: NEW → ASSIGNED
Pushed by mtigley@mozilla.com:
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

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.

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.

Flags: needinfo?(mtigley)
Pushed by mtigley@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 80

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.

Flags: needinfo?(mtigley)

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:
Flags: needinfo?(mtigley)
Attachment #9161513 - Flags: approval-mozilla-beta?
Attachment #9161892 - Flags: approval-mozilla-beta?
Flags: qe-verify+

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.

Attachment #9161513 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9161892 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

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

Status: RESOLVED → VERIFIED
Blocks: 1654697
You need to log in before you can comment on or make changes to this bug.