Closed Bug 1239459 Opened 4 years ago Closed 3 years ago

Toggle touch event simulation

Categories

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

defect

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Iteration:
49.2 - May 23
Tracking Status
firefox49 --- fixed

People

(Reporter: jryans, Assigned: gl)

References

Details

(Whiteboard: [multiviewport] [mvp-rdm])

Attachments

(2 files, 3 obsolete files)

Touch events button to turn on simulation of mobile touch events
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
Priority: -- → P3
Blocks: 1254388
Priority: P3 → P1
Whiteboard: [multiviewport] → [multiviewport] [mvp-rdm]
QA Contact: mihai.boldan
Priority: P1 → P2
Depends on: 1240896
Assignee: nobody → gl
Status: NEW → ASSIGNED
Iteration: --- → 48.2 - Apr 4
Priority: P2 → P1
Assignee: gl → nobody
Status: ASSIGNED → NEW
Iteration: 48.2 - Apr 4 → ---
Priority: P1 → P2
This bug depends on bug 1240896 because you need a browser with a message manager before you can do much here.  It might be better to choose a different bug until that one is resolved.
Assignee: nobody → gl
Status: NEW → ASSIGNED
Iteration: --- → 48.3 - Apr 25
Priority: P2 → P1
One piece of this involves "enabling" touch events at all.  Assuming this bug copies the approach of legacy RDM, access to touch events can only be changed for the entire Firefox session (since it's a pref), but we've previously agreed it's important for RDM features to only affect the current tab.

Bug 970346 discusses adding a docshell flag instead so it would only affect one tab.  I'll put this bug up for triage.
See Also: → 970346
Iteration: 48.3 - Apr 25 → 49.1 - May 9
Attached patch 1239459.patch (obsolete) — Splinter Review
Looking for some general feedback. I don't think this patch needs much more beyond unit tests and maybe a check to reload, but I don't think the reload is actually necessary(?).
Attachment #8747286 - Flags: feedback?(jryans)
Comment on attachment 8747286 [details] [diff] [review]
1239459.patch

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

::: devtools/client/responsive.html/actions/index.js
@@ +44,5 @@
>    // Update the device modal open state.
>    "UPDATE_DEVICE_MODAL_OPEN",
>  
> +  // Update the touch simulation enabled state.
> +  "UPDATE_TOUCH_SIMULATION_ENABLED",

I am not sure what to think about this UPDATE_* naming for boolean actions.  I sent a mail to the team list to get some more feedback.  When I read "UPDATE_*", it feels wrong to me at first since it's named in terms of "change state to X" instead of "user requested action Y" like some of the earlier actions.  It seems like the UPDATE_* naming (taken to the the extreme) could lead to every action starting with UPDATE_* since each action wants to change some kind of state, which is a form of update, but by then the word "update" doesn't really mean anything anymore.

I'll keep thinking about this and hopefully finish feedback tomorrow.
Blocks: 1269882
Comment on attachment 8747286 [details] [diff] [review]
1239459.patch

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

We do indeed need to reload for the event handlers to be bound properly, but we can work that part out in a follow up.  I filed bug 1269882 for this.

Overall it seems reasonable!  I think some simple unit tests from the new UI things would be good.  As discussed at the standup, we can leave coverage of actual touch simulation to the existing legacy RDM test that we'll eventually port over.

Also needs ui-review.

::: devtools/client/responsive.html/actions/index.js
@@ +44,5 @@
>    // Update the device modal open state.
>    "UPDATE_DEVICE_MODAL_OPEN",
>  
> +  // Update the touch simulation enabled state.
> +  "UPDATE_TOUCH_SIMULATION_ENABLED",

Sounds like most people like this naming style, so let's keep it as-is.

::: devtools/client/responsive.html/app.js
@@ +85,5 @@
> +  onUpdateTouchSimulationEnabled() {
> +    let { isTouchEnabled } = this.props.touchSimulation;
> +    this.props.dispatch(updateTouchSimulationEnabled(!isTouchEnabled));
> +
> +    window.postMessage({

Since we're sending this change through Redux and it's natural for Redux action creators to contain the logic to make the action happen, I think the `postMessage` should move to the action creator.  (See screenshot as an example, or other complex things in Memory tool.)

@@ +86,5 @@
> +    let { isTouchEnabled } = this.props.touchSimulation;
> +    this.props.dispatch(updateTouchSimulationEnabled(!isTouchEnabled));
> +
> +    window.postMessage({
> +      type: "touch-simulation-toggle",

Might as well make this sound active (since it's a request to make a change) and use the same name as in Redux, so "update-touch-simulation".

::: devtools/client/responsive.html/manager.js
@@ +225,5 @@
>      toolWindow.addEventListener("message", this);
>      yield waitForMessage(toolWindow, "init");
>      toolWindow.addInitialViewport(contentURI);
>      yield waitForMessage(toolWindow, "browser-mounted");
> +    let browser = toolWindow.document.querySelector("iframe.browser");

To ease working with multiple viewports, we should track an array of simulators, since we'll need one for each viewport.

When you get a message to start simulation, use `querySelectorAll` and make a simulator for each browser found since the button is currently in the global toolbar.  I guess we'll leave issues about adding a new viewport while simulation is already enabled for some future day. :)

@@ +236,5 @@
>      this.browserWindow = null;
>      this.tab = null;
>      this.inited = null;
>      this.toolWindow = null;
> +    this.touchEventSimulator = null;

It is important to stop the simulators on close.  They manipulate browser state that we need to reset on exit.

::: devtools/client/responsive.html/types.js
@@ +89,5 @@
> + * Touch simulation.
> + */
> +exports.touchSimulation = {
> +
> +  isTouchEnabled: PropTypes.bool.isRequired,

I think this can be shortened to `isEnabled` or `enabled`, since the touch part seems redundant.  (We already have `displayed` as a boolean state name...  Would prefer to have one style for such things!)
Attachment #8747286 - Flags: feedback?(jryans) → feedback+
Attached patch 1239459.patch WIP (obsolete) — Splinter Review
Attached patch 1239459.patch [1.0] (obsolete) — Splinter Review
I left the window.postMessage in app.js since it causes problems with xpcshell tests with an undefined window if it is moved to actions/
Attachment #8747286 - Attachment is obsolete: true
Attachment #8749819 - Attachment is obsolete: true
Attachment #8750055 - Flags: review?(jryans)
Attachment #8750055 - Flags: ui-review?(hholmes)
Comment on attachment 8750055 [details] [diff] [review]
1239459.patch [1.0]

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

Looks good, sorry there was so much trouble with the multiple simulator approach last week.  We can investigate more in the future.

::: devtools/client/responsive.html/manager.js
@@ +243,5 @@
>      let loaded = waitForDocLoadComplete(browserWindow.gBrowser);
>      tabBrowser.goBack();
>      yield loaded;
> +
> +    yield this.touchEventSimulator.stop();

Move this before the previous step that navigates the tab back.  We want to have everything back to normal before navigating the tab back.
Attachment #8750055 - Flags: review?(jryans) → review+
Iteration: 49.1 - May 9 → 49.2 - May 23
Attachment #8750055 - Attachment is obsolete: true
Attachment #8750055 - Flags: ui-review?(hholmes)
Attachment #8750834 - Flags: ui-review?(hholmes)
Attachment #8750834 - Flags: review+
Attachment #8750834 - Flags: ui-review?(hholmes) → ui-review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c3ff04109a8a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Since we aren't reloading yet, this is pretty hard to test manually.  Let's do testing once reload is in place.
Flags: qe-verify+ → qe-verify-
QA Contact: mihai.boldan
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.