Support viewport render mode / <meta viewport> in RDM

RESOLVED FIXED in Firefox 64

Status

P3
normal
RESOLVED FIXED
2 years ago
5 days ago

People

(Reporter: bigben, Assigned: bradwerth)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs)

Trunk
Firefox 64
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [rdm-v2][triage][dt-q])

Attachments

(7 attachments, 4 obsolete attachments)

46 bytes, text/x-phabricator-request
smaug
: review+
Details | Review
46 bytes, text/x-phabricator-request
dholbert
: review+
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
botond
: review+
smaug
: review+
Details | Review
529 bytes, text/plain
Details
46 bytes, text/x-phabricator-request
Details | Review
50.21 KB, image/png
Details
(Reporter)

Description

2 years ago
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
(Reporter)

Comment 1

2 years ago
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).
(Reporter)

Updated

2 years ago
Assignee: nobody → bchabod
Status: NEW → ASSIGNED
(Reporter)

Updated

2 years ago
See Also: → bug 1282089
Priority: -- → P3
Whiteboard: [multiviewport][reserve-rdm]

Updated

2 years ago
Flags: qe-verify?
(Reporter)

Comment 2

2 years ago
Created attachment 8777772 [details] [diff] [review]
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?
(Reporter)

Updated

2 years ago
Flags: needinfo?(jryans)
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(jryans)
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

Updated

2 years ago
QA Contact: mihai.boldan
(Reporter)

Comment 4

2 years ago
(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.
(Reporter)

Updated

2 years ago
Depends on: 1285566
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!
Assignee: be.chabod → nobody
Status: ASSIGNED → NEW
Blocks: 1317772
Summary: Support viewport render mode in RDM → Support viewport render mode / <meta viewport> in RDM
Whiteboard: [multiviewport][reserve-rdm] → [rdm-v2][triage]
Duplicate of this bug: 1351293

Comment 7

10 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

10 months ago
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.)
Attachment #8777772 - Attachment is obsolete: true

Comment 12

10 months ago
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.

Updated

4 months ago
Product: Firefox → DevTools
status-firefox50: affected → ---
Duplicate of this bug: 774055
+Martin, this is also blocking GeckoView testing & compat work.

Maybe Brad or Birtles' team could help.
Flags: needinfo?(mbalfanz)
Looking into it
Flags: needinfo?(mbalfanz)
(Assignee)

Comment 16

2 months ago
(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.
Assignee: nobody → bwerth
(Assignee)

Comment 17

2 months ago
Created attachment 9000117 [details]
Bug 1290420 Part 1: Add a docshell flag for overriding meta viewport handling. r=smaug!
(Assignee)

Comment 18

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

Depends on D3373
(Assignee)

Comment 19

2 months ago
Created 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!

Depends on D3375
(Assignee)

Comment 20

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

Depends on D3376
(Assignee)

Updated

2 months ago
Attachment #8937278 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8937279 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8937280 - Attachment is obsolete: true
Whiteboard: [rdm-v2][triage] → [rdm-v2][triage][dt-q]
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.
Attachment #9000120 - Flags: review+
(Assignee)

Comment 22

2 months ago
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.
Flags: needinfo?(botond)
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.
Attachment #9000119 - Flags: review+

Comment 24

2 months ago
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.
Attachment #9000121 - Flags: review+

Comment 25

2 months ago
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.
Attachment #9000117 - Flags: review+
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.

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.
Flags: needinfo?(botond)
(Assignee)

Comment 27

2 months ago
(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.
(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 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.
Attachment #9000121 - Flags: review+
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.
Attachment #9000121 - Flags: review+
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.
Attachment #9000121 - Flags: review+

Comment 32

a month ago
Pushed by bwerth@mozilla.com:
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
Hard to understate how happy this makes me.
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:
Flags: needinfo?(bwerth)

Comment 35

a month ago
Backout by dvarga@mozilla.com:
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
Blocks: 1493094
(Assignee)

Comment 38

28 days ago
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?
Flags: needinfo?(bwerth) → needinfo?(botond)

Updated

28 days ago
Flags: needinfo?(gl)
(Assignee)

Comment 39

26 days ago
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.
Flags: needinfo?(botond)

Updated

24 days ago
Flags: needinfo?(gl)
(Assignee)

Comment 41

24 days ago
Created attachment 9012952 [details]
Bug 1290420 Part 5: Change test function waitForViewportResizeTo to check viewport size.
(Assignee)

Updated

20 days ago
Duplicate of this bug: 883005

Updated

19 days ago
Attachment #9012952 - Attachment description: Bug 1290420 Part 5: Change test function waitForViewportResizeTo to actually check viewport size, not content size. → Bug 1290420 Part 5: Change test function waitForViewportResizeTo to check viewport size.

Comment 46

18 days ago
Pushed by bwerth@mozilla.com:
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
Depends on: 1496609

Comment 48

5 days ago
Created attachment 9017857 [details]
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.

Updated

5 days ago
Flags: needinfo?(bwerth)
(Assignee)

Comment 49

5 days ago
(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.
Flags: needinfo?(bwerth)

Comment 50

5 days ago
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?
You need to log in before you can comment on or make changes to this bug.