1290420 - Support viewport render mode / <meta viewport> in RDM

Status: RESOLVED FIXED

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

Description

[Benoit Chabod [:bigben]]

Mobile browsers render pages in a virtual "window" (the viewport), usually wider than the screen, so they don't need to squeeze every page layout into a tiny window (which would break many non-mobile-optimized sites). Users can pan and zoom to see different areas of the page. [1]

This behaviour can be controlled using a meta tag, but even pages without this tag can render differently on same size displays if you use a viewport or not.

Bug 1282089 adds support for real touch simulation in RDM (double click to zoom, drag to scroll, etc), so it makes sense to add a way to enable "viewport render mode" in our tool as well.

I've already found a way to enable this for the RDM document only [2], so all we really need is some UI.

[1]: https://developer.mozilla.org/en/docs/Mozilla/Mobile/Viewport_meta_tag#Background
[2]: https://bugzilla.mozilla.org/show_bug.cgi?id=1282089#c4

Comment 1

[Benoit Chabod [:bigben]]

By the way, we need to be careful with the vocabulary here.
The RDM's viewport (the iframe containing the content) and the mobile viewport can be confused easily, I guess we could say that the goal here is to turn the RDM viewport into a real mobile viewport (when this mode is enabled).

Comment 2

[Benoit Chabod [:bigben]]

Attachment: Part 1: Allow per-document control of the meta-viewport preference

Okay, here's the first part of this bug, this is a platform patch to add a docshell attribute that controls the dom.meta-viewport.enabled, similarly to touch events in bug 970346.

Ryan, I saw in bug 1285566 that you want to use a DevTools actor to flip platform attributes in the RDM content. I guess we should do the same thing here?
Maybe we need some kind of general DevTools actor that we can use to flip things in the child process in the future... But a frame script would also work. What do you think?

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8777772 [details] [diff] [review]
Part 1: Allow per-document control of the meta-viewport preference

Review of attachment 8777772 [details] [diff] [review]:
-----------------------------------------------------------------

Hopefully our discussion on IRC answered your question about frame scripts vs. actors.  In general, I think we should prefer an actor for these emulation features, since that gives us more flexibility for the future with remote devices.

::: layout/base/nsPresShell.cpp
@@ +981,5 @@
> +    // Check if the docshell overrides the GFX preference
> +    bool mobileDocShell = false;
> +    nsCOMPtr<nsIDocShell> docShell(mPresContext->GetDocShell());
> +    if (docShell) {
> +      docShell->GetForceMetaViewport(&mobileDocShell);

This lets you enable it if it would have been off, but there's no way to disable if it would have been on.  Do we want both?

Also "override" might be a better word than "force".  See the recent additions for touch events[1].

[1]: https://dxr.mozilla.org/mozilla-central/rev/1576e7bc1bec7232e9e4ba78cce62526b1a6380b/docshell/base/nsIDocShell.idl#1121

Comment 4

[Benoit Chabod [:bigben]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> This lets you enable it if it would have been off, but there's no way to
> disable if it would have been on.  Do we want both?

You're right, while we're at it, I will add a three-state override similar to the touch events one.
Regarding the RDM part of this bug, I'm gonna wait for bug 1285566 to land though, because we're gonna need the EmulationActor here.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Since Benoit's internship has ended, I am going to assume he may not be working on this anymore.  If that's incorrect, feel free to pick it up again!

Comment 7

[Botond Ballo [:botond]]

I'd like to get this across the finish line, as it would allow me to do a lot of the testing and iteration I need to do for bug 1123938 and its dependencies using a desktop build rather than an Android build.

Comment 8

[Botond Ballo [:botond]]

Attachment: Bug 1290420 - Add a docshell flag for overriding meta viewport handling.

Review commit: https://reviewboard.mozilla.org/r/207966/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207966/

Comment 9

[Botond Ballo [:botond]]

Attachment: Bug 1290420 - Respect the metaViewportOverride flag on the docshell.

Review commit: https://reviewboard.mozilla.org/r/207968/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207968/

Comment 10

[Botond Ballo [:botond]]

Attachment: Bug 1290420 - Set the metaViewportOverride flag when viewing a device with touch event support in Responsive Design Mode.

Review commit: https://reviewboard.mozilla.org/r/207970/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207970/

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8777772 [details] [diff] [review]
Part 1: Allow per-document control of the meta-viewport preference

(An updated version of this patch can be found in the MozReview patch series I posted.)

Comment 12

[Botond Ballo [:botond]]

I posted some WIP patches. They implement the direction described in comment 4, and include the devtools bits and bits for respecting the docshell flag in relevant places in platform code.

Unfortunately, they do not work as intended yet, because we make the decision to create a MobileViewportManager or not at pres shell initialization time, and devtools changes the flag too late for it to be seen at that time.

To fix this, we may need to make devtools do something that triggers re-creating the pres shell when it changes the flag, or alternately allow creating or destroying a MobileViewportManager after the pres shell has already been initialized.

Comment 14

[:Harald Kirschner :digitarald]

+Martin, this is also blocking GeckoView testing & compat work.

Maybe Brad or Birtles' team could help.

Comment 15

[Martin Balfanz [:mbalfanz]]

Looking into it

Comment 16

[Brad Werth [:bradwerth]]

(In reply to Botond Ballo [:botond] [on PTO, back Aug 27] from comment #12)
> Unfortunately, they do not work as intended yet, because we make the
> decision to create a MobileViewportManager or not at pres shell
> initialization time, and devtools changes the flag too late for it to be
> seen at that time.
> 
> To fix this, we may need to make devtools do something that triggers
> re-creating the pres shell when it changes the flag, or alternately allow
> creating or destroying a MobileViewportManager after the pres shell has
> already been initialized.

I'm rebasing the patches and adding a piece to create the MobileViewportManager on demand. I'm not clear on if there is danger in retaining the MVM when and if the override is disabled -- I assume not since the MVM isn't normally destroyed until the nsPresShell is destroyed.

Comment 17

[Brad Werth [:bradwerth]]

Attachment: Bug 1290420 Part 1: Add a docshell flag for overriding meta viewport handling. r=smaug!

Comment 18

[Brad Werth [:bradwerth]]

Attachment: Bug 1290420 Part 2: Respect the metaViewportOverride flag on the docshell. r=dholbert!

Depends on D3373

Comment 19

[Brad Werth [:bradwerth]]

Attachment: Bug 1290420 Part 3: Set the metaViewportOverride flag when viewing a device with touch event support in Responsive Design Mode. r=gl!

Depends on D3375

Comment 20

[Brad Werth [:bradwerth]]

Attachment: Bug 1290420 Part 4: Make the PresShell create a MobileViewportManager on demand. r=smaug!

Depends on D3376

Comment 21

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 9000120 [details]
Bug 1290420 Part 3: Set the metaViewportOverride flag when viewing a device with touch event support in Responsive Design Mode. r=gl!

Gabriel [:gl] (ΦωΦ) has approved the revision.

Comment 22

[Brad Werth [:bradwerth]]

With review changes now complete, I'd like to hear from Botond on whether the patch works well enough to unblock the issues noted in comment 7.

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 9000119 [details]
Bug 1290420 Part 2: Respect the metaViewportOverride flag on the docshell. r=dholbert!

Daniel Holbert [:dholbert] has approved the revision.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9000121 [details]
Bug 1290420 Part 4: Make the PresShell create a MobileViewportManager on demand. r=smaug!

Olli Pettay [:smaug] has approved the revision.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9000117 [details]
Bug 1290420 Part 1: Add a docshell flag for overriding meta viewport handling. r=smaug!

Olli Pettay [:smaug] has approved the revision.

Comment 26

[Botond Ballo [:botond]]

Attachment: Testcase

Thanks for picking this up, Brad!

I created a small test page (attached) that can be used to test if the patches work.

STR:
  1. Load the test page in RDM
  2. Enable touch simulation, or select a device that supports
     touch, and whose screen width is < 500 px.

Expected results with RDM ignoring meta viewport:
  The rectangles render in fewer than five columns (often two
  or three, depending on the screen width)

Expected results with RDM respecting meta viewport:
  The rectangles render in five columns

With current nightly, I get the first behaviour ("Expected results with RDM ignoring meta viewport"). With the patch series applied, I get the second behaviour ("Expected results with RDM respecting meta viewport"), suggesting that the patches do indeed fix the issue!

I do have a minor concern about the implementation of the Part 4 patch, which I'll write up in Phabricator.

Comment 27

[Brad Werth [:bradwerth]]

(In reply to Botond Ballo [:botond] from comment #26)
> Created attachment 9006705 [details]
> Testcase
> 
> Thanks for picking this up, Brad!
> 
> I created a small test page (attached) that can be used to test if the
> patches work.

As I see how this is invoked, it seems clear to me that we have to make the override reversible. The current patches don't permit that. I'll update the patches to make that possible.

Comment 28

[Botond Ballo [:botond]]

(In reply to Brad Werth [:bradwerth] from comment #27)
> As I see how this is invoked, it seems clear to me that we have to make the
> override reversible. The current patches don't permit that. I'll update the
> patches to make that possible.

Ah, good point!

Comment 29

[Botond Ballo [:botond]]

Comment on attachment 9000121 [details]
Bug 1290420 Part 4: Make the PresShell create a MobileViewportManager on demand. r=smaug!

Botond Ballo [:botond] has approved the revision.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9000121 [details]
Bug 1290420 Part 4: Make the PresShell create a MobileViewportManager on demand. r=smaug!

Olli Pettay [:smaug] has been removed from the revision.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9000121 [details]
Bug 1290420 Part 4: Make the PresShell create a MobileViewportManager on demand. r=smaug!

Olli Pettay [:smaug] has approved the revision.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/755a32f27943
Part 1: Add a docshell flag for overriding meta viewport handling. r=smaug
https://hg.mozilla.org/integration/autoland/rev/a6eb531e1216
Part 2: Respect the metaViewportOverride flag on the docshell. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/1d9dad3046ee
Part 3: Set the metaViewportOverride flag when viewing a device with touch event support in Responsive Design Mode. r=gl
https://hg.mozilla.org/integration/autoland/rev/210fdc42d29f
Part 4: Make the PresShell create a MobileViewportManager on demand. r=botond,smaug

Comment 33

[Mike Taylor [:miketaylr]]

Hard to understate how happy this makes me.

Comment 34

[Daniel Varga [:dvarga]]

Backed out 4 changesets (bug 1290420) for devtools failures on rowser_styleeditor_media_sidebar_links.js on a CLOSED TREE

Push: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=210fdc42d29fd4a8d18d7d730a7fc48c37e447fb

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=198957632&repo=autoland&lineNumber=4426

[task 2018-09-12T23:28:18.856Z] 23:28:18     INFO - TEST-PASS | devtools/client/shared/test/browser_telemetry_button_responsive.js | DEVTOOLS_RESPONSIVE_OPENED_COUNT correct. - 
[task 2018-09-12T23:28:18.858Z] 23:28:18     INFO - TEST-PASS | devtools/client/shared/test/browser_telemetry_button_responsive.js | DEVTOOLS_RESPONSIVE_TIME_ACTIVE_SECONDS has at least one entry. - 
[task 2018-09-12T23:28:18.859Z] 23:28:18     INFO - Buffered messages finished
[task 2018-09-12T23:28:18.860Z] 23:28:18     INFO - TEST-UNEXPECTED-FAIL | devtools/client/shared/test/browser_telemetry_button_responsive.js | A promise chain failed to handle a rejection: Connection closed, pending request to server1.conn4.child1/emulationActor19, type clearTouchEventsOverride failed
[task 2018-09-12T23:28:18.861Z] 23:28:18     INFO - 
[task 2018-09-12T23:28:18.862Z] 23:28:18     INFO - Request stack:
[task 2018-09-12T23:28:18.863Z] 23:28:18     INFO - request@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1351:14
[task 2018-09-12T23:28:18.864Z] 23:28:18     INFO - generateRequestMethods/</frontProto[name]@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1505:14
[task 2018-09-12T23:28:18.865Z] 23:28:18     INFO - updateTouchSimulation@resource://devtools/shared/base-loader.js -> resource://devtools/client/responsive.html/manager.js:650:22
[task 2018-09-12T23:28:18.866Z] 23:28:18     INFO - destroy@resource://devtools/shared/base-loader.js -> resource://devtools/client/responsive.html/manager.js:405:29
[task 2018-09-12T23:28:18.867Z] 23:28:18     INFO - async*closeIfNeeded@resource://devtools/shared/base-loader.js -> resource://devtools/client/responsive.html/manager.js:157:31
[task 2018-09-12T23:28:18.868Z] 23:28:18     INFO - async*toggle@resource://devtools/shared/base-loader.js -> resource://devtools/client/responsive.html/manager.js:66:48

Failure log 2: https://treeherder.mozilla.org/logviewer.html#?job_id=198957632&repo=autoland&lineNumber=4426

[task 2018-09-12T23:28:18.856Z] 23:28:18     INFO - TEST-PASS | devtools/client/shared/test/browser_telemetry_button_responsive.js | DEVTOOLS_RESPONSIVE_OPENED_COUNT correct. - 
[task 2018-09-12T23:28:18.858Z] 23:28:18     INFO - TEST-PASS | devtools/client/shared/test/browser_telemetry_button_responsive.js | DEVTOOLS_RESPONSIVE_TIME_ACTIVE_SECONDS has at least one entry. - 
[task 2018-09-12T23:28:18.859Z] 23:28:18     INFO - Buffered messages finished
[task 2018-09-12T23:28:18.860Z] 23:28:18     INFO - TEST-UNEXPECTED-FAIL | devtools/client/shared/test/browser_telemetry_button_responsive.js | A promise chain failed to handle a rejection: Connection closed, pending request to server1.conn4.child1/emulationActor19, type clearTouchEventsOverride failed
[task 2018-09-12T23:28:18.861Z] 23:28:18     INFO - 
[task 2018-09-12T23:28:18.862Z] 23:28:18     INFO - Request stack:
[task 2018-09-12T23:28:18.863Z] 23:28:18     INFO - request@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1351:14
[task 2018-09-12T23:28:18.864Z] 23:28:18     INFO - generateRequestMethods/</frontProto[name]@resource://devtools/shared/base-loader.js -> resource://devtools/shared/protocol.js:1505:14
[task 2018-09-12T23:28:18.865Z] 23:28:18     INFO - updateTouchSimulation@resource://devtools/shared/base-loader.js -> resource://devtools/client/responsive.html/manager.js:650:22
[task 2018-09-12T23:28:18.866Z] 23:28:18     INFO - destroy@resource://devtools/shared/base-loader.js -> resource://devtools/client/responsive.html/manager.js:405:29
[task 2018-09-12T23:28:18.867Z] 23:28:18     INFO - async*closeIfNeeded@resource://devtools/shared/base-loader.js -> resource://devtools/client/responsive.html/manager.js:157:31
[task 2018-09-12T23:28:18.868Z] 23:28:18     INFO - async*toggle@resource://devtools/shared/base-loader.js -> resource://devtools/client/responsive.html/manager.js:66:48

Backout:

Comment 35

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/458e5b24da2f
Backed out 4 changesets for devtools failures on rowser_styleeditor_media_sidebar_links.js on a CLOSED TREE

Comment 36

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e82b8695f330ef196f45eedfca44ff013e201b6c

Comment 37

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be0a147430b66ae89bc9a6dbaae0bef23a921233

Comment 38

[Brad Werth [:bradwerth]]

Botond, I could use your help in diagnosing the test failures. I'm not experienced in debugging protocol issues with the promise chains, and some of the failures are in calls to clearTouchEventsOverride, which wasn't intended to be changed by these patches (but the spec was altered in Part 3). Can you give me any insight into what's going on?

Comment 39

[Brad Werth [:bradwerth]]

I see the failure now. In part 3 the code for updateTouchSimulation() is supposed to return a promise, and is instead OR-ing together the values of two unresolved promises. I'll change this into a promise chain and that should resolve it.

Comment 40

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34543ce6d8b0ae4911e530ed7a1b77427a1aac5a

Comment 41

[Brad Werth [:bradwerth]]

Attachment: Bug 1290420 Part 5: Change test function waitForViewportResizeTo to check viewport size.

Comment 42

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d9e5abbcdca9f99c50ccd07508de5790f4ae993

Comment 43

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=64b1171b9a56657be789db04eb1ba4318b3e5063

Comment 45

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3af2e785aafae66737ed80447b775a8168d0b07

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec06f75313de
Part 1: Add a docshell flag for overriding meta viewport handling. r=smaug
https://hg.mozilla.org/integration/autoland/rev/e887b26390ec
Part 2: Respect the metaViewportOverride flag on the docshell. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/c23a9b7119c4
Part 3: Set the metaViewportOverride flag when viewing a device with touch event support in Responsive Design Mode. r=gl
https://hg.mozilla.org/integration/autoland/rev/e6cfd555f0d1
Part 4: Make the PresShell create a MobileViewportManager on demand. r=botond,smaug
https://hg.mozilla.org/integration/autoland/rev/3acd60dbb363
Part 5: Change test function waitForViewportResizeTo to check viewport size. r=gl

Comment 47

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/ec06f75313de
https://hg.mozilla.org/mozilla-central/rev/e887b26390ec
https://hg.mozilla.org/mozilla-central/rev/c23a9b7119c4
https://hg.mozilla.org/mozilla-central/rev/e6cfd555f0d1
https://hg.mozilla.org/mozilla-central/rev/3acd60dbb363

Comment 48

[Oana Botisan, Desktop QA]

Attachment: touch screen simulation - issue.png

I managed to reproduce the issue using an older version of Nightly from 2017-04-14 on Windows 10 x64 by using the steps from comment 26. 
I verified the fix using latest Nightly 64.0a1 on Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13. The bug is still reproducing. I don't get 5 columns. I get 3-4 from which the last one in the first row is cut in half. 
Please look at the attached image.

Comment 49

[Brad Werth [:bradwerth]]

(In reply to Oana Botisan from comment #48)
> The bug is still reproducing. I don't get 5
> columns. I get 3-4 from which the last one in the first row is cut in half. 
> Please look at the attached image.

I believe your screenshot shows the correct behavior. With touch simulation / meta viewport on, you have a row of five squares (of which you can see three-and-a-half without scrolling) and a row of two squares. That's the expected results from the steps to reproduce in comment 26.

Comment 50

[Oana Botisan, Desktop QA]

Yeah, I think you might be right. Sorry... for a second I lost my train of thought. Thank you for your response. 

But I did notice two other things besides this:
1. On macOS and Ubuntu the first time on a clean profile, when you disable touch simulation, nothing happens, but after the touch simulation is enabled again, everything works as expected. 
2. I think it will be a good idea to make horizontal scrolling on the page, if it's possible. But I did notice that scrolling by using the keyboards doesn't work. Is that a known issue?

Comment 51

[Brad Werth [:bradwerth]]

(In reply to Oana Botisan from comment #50)
> Yeah, I think you might be right. Sorry... for a second I lost my train of
> thought. Thank you for your response. 
> 
> But I did notice two other things besides this:
> 1. On macOS and Ubuntu the first time on a clean profile, when you disable
> touch simulation, nothing happens, but after the touch simulation is enabled
> again, everything works as expected. 
> 2. I think it will be a good idea to make horizontal scrolling on the page,
> if it's possible. But I did notice that scrolling by using the keyboards
> doesn't work. Is that a known issue?

I encourage you to open bugs (separately) on these issues. That way you can provide the Steps to Reproduce and screenshots that match your experience, which makes it much easier for us to confirm and fix the issue. I don't know if there are other bugs open that match what you are describing.

Comment 52

[Gyula Palko[:gpalko]]

Removing the qe-verify flag, since this was verified, based on comment 49 and comment 50