Closed Bug 1074180 Opened 10 years ago Closed 9 years ago

[e10s] touch events button doesn't work in responsive mode

Categories

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

Product:
DevTools
Component:
Responsive Design Mode
Type:
defect

Tracking

(e10s+, firefox40 wontfix, firefox41 fixed, firefox42 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 42
Tracking Flags:
Tracking Status
e10s + ---
firefox40 --- wontfix
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: harth, Assigned: jryans)

References

Details

(Whiteboard: [e10s-m7])

Attachments

(3 files)

MozReview Request: Bug 1074180 - Fix linting issues in touch-events.js. r=ochameau
(deleted), text/x-review-board-request
jryans
: review+
ritu
: approval-mozilla-aurora+
Details
MozReview Request: Bug 1074180 - Avoid TypeError with touch events on desktop. r=ochameau
(deleted), text/x-review-board-request
jryans
: review+
ritu
: approval-mozilla-aurora+
Details
MozReview Request: Bug 1074180 - Convert touch simulator to frame script for e10s. r=ochameau
(deleted), text/x-review-board-request
jryans
: review+
ritu
: approval-mozilla-aurora+
Details 
Error under e10s:

"this.touchEventHandler is undefined"
Blocks: dte10s
Blocks: 937169
These DevTools bugs should probably block e10s from riding to Aurora.
tracking-e10s: + → m6+
devtools bugs will block the e10s merge to Aurora, but blassey would like them to be tracked by dte10s meta bug 875871, not the tracking-e10s=m6+ flag.
tracking-e10s: m6+ → +
Whiteboard: [e10s-m6]
Paul, correct me if I'm wrong, but this sounds like something that should be fixed as part of bug 1067145, don't you agree?
Blocks: 1067145
No longer blocks: dte10s
Flags: needinfo?(paul)
Whiteboard: [e10s-m6] → [e10s-m7]
(In reply to Eddy Bruel [:ejpbruel] from comment #3)
> Paul, correct me if I'm wrong, but this sounds like something that should be
> fixed as part of bug 1067145, don't you agree?

bug 1067145 disables touch events in E10S mode.
Flags: needinfo?(paul)
(In reply to Paul Rouget [:paul] from comment #4)
> (In reply to Eddy Bruel [:ejpbruel] from comment #3)
> > Paul, correct me if I'm wrong, but this sounds like something that should be
> > fixed as part of bug 1067145, don't you agree?
> 
> bug 1067145 disables touch events in E10S mode.

I assume that's a temporary fix? Surely we want touch events to work in E10s eventually. In that case, this should be the followup bug for that.
Flags: needinfo?(paul)
(In reply to Eddy Bruel [:ejpbruel] from comment #5)
> (In reply to Paul Rouget [:paul] from comment #4)
> > (In reply to Eddy Bruel [:ejpbruel] from comment #3)
> > > Paul, correct me if I'm wrong, but this sounds like something that should be
> > > fixed as part of bug 1067145, don't you agree?
> > 
> > bug 1067145 disables touch events in E10S mode.
> 
> I assume that's a temporary fix?

Yes.

> Surely we want touch events to work in E10s
> eventually. In that case, this should be the followup bug for that.

Right. It will happen here I guess.
Flags: needinfo?(paul)
FYI, some devs were alarmed by this bug when we started pushing e10s on developer edition:

  https://twitter.com/brianleroux/status/619594859096117248

Should the priority of this be raised?
Flags: needinfo?(paul)
Pinging Jeff as well, since Paul is no longer focused on DevTools.
Flags: needinfo?(jgriffiths)
I think we should try to get this working for the 42 cycle. Joe - who is the best person to look at this?
Flags: needinfo?(paul)
Flags: needinfo?(jwalker)
Flags: needinfo?(jgriffiths)
Priority: -- → P1
It seems Paul wrote much of the original code, but he's no longer focused on DevTools.

I think either myself or Alex would be good options.

I'll give it a try.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Flags: needinfo?(jwalker)
In responsivedesign.jsm (code executed in the parent process), touch simulation is controlled via `touchEventHandler.start/stop`.

This needs to go and be replaced by something like: mm.sendAsyncMessage("ResponsiveMode:StartTouch");

Client side code is located in responsivedesign-child.js

From here, I guess it's just a matter of loading toolkit/devtools/touch-events.js and listen to ResponsiveMode:*Touch events, and do the `touchEventHandler.start/stop` calls.
Awesome, thanks Paul! I really think we need this for 42.
Target Milestone: --- → Firefox 42
Bug 1074180 - Fix linting issues in touch-events.js. r=ochameau
Attachment #8633787 - Flags: review?(poirot.alex)
Bug 1074180 - Avoid TypeError with touch events on desktop. r=ochameau
Attachment #8633788 - Flags: review?(poirot.alex)
Bug 1074180 - Convert touch simulator to frame script for e10s. r=ochameau
Attachment #8633789 - Flags: review?(poirot.alex)
Comment on attachment 8633787 [details]
MozReview Request: Bug 1074180 - Fix linting issues in touch-events.js. r=ochameau

https://reviewboard.mozilla.org/r/13261/#review11941

Ship It!
Attachment #8633787 - Flags: review?(poirot.alex) → review+
Attachment #8633788 - Flags: review?(poirot.alex) → review+
Comment on attachment 8633788 [details]
MozReview Request: Bug 1074180 - Avoid TypeError with touch events on desktop. r=ochameau

https://reviewboard.mozilla.org/r/13263/#review11943

Ship It!
Attachment #8633789 - Flags: review?(poirot.alex) → review+
Comment on attachment 8633789 [details]
MozReview Request: Bug 1074180 - Convert touch simulator to frame script for e10s. r=ochameau

https://reviewboard.mozilla.org/r/13265/#review11937

Thanks!!
Works great on Firefox.
Did you gave it a try on b2g-desktop or mulet to ensure that gaia still works fine?

::: toolkit/devtools/touch/simulator.js:29
(Diff revision 1)
> +    // Maybe browser is a frame

nit: a frame -> an iframe?

::: toolkit/devtools/touch/moz.build
(Diff revision 1)
> -    'server/actors/highlighter.css'

mozreview is really disturbing with this "copied" feature!!
(In reply to Alexandre Poirot [:ochameau] from comment #21)
> Comment on attachment 8633789 [details]
> MozReview Request: Bug 1074180 - Convert touch simulator to frame script for
> e10s. r=ochameau
> 
> https://reviewboard.mozilla.org/r/13265/#review11937
> 
> Thanks!!
> Works great on Firefox.
> Did you gave it a try on b2g-desktop or mulet to ensure that gaia still
> works fine?

Yes, I've tested b2g-desktop and mulet, works fine.

> 
> ::: toolkit/devtools/touch/simulator.js:29
> (Diff revision 1)
> > +    // Maybe browser is a frame
> 
> nit: a frame -> an iframe?

Fixed.

> ::: toolkit/devtools/touch/moz.build
> (Diff revision 1)
> > -    'server/actors/highlighter.css'
> 
> mozreview is really disturbing with this "copied" feature!!

Haha, that's actually my fault, that's what the patch says...  I turn on some special settings to ensure that the touch-events.js rename appeared as a move (which MozReview noticed and even cleaned up the whitespace changes!), but then it also created this copy.

I'll tweak my settings again so it's only the move.
Should we uplift this? If it isn't risky, I'd like to get it in 41.
Flags: needinfo?(jryans)
(In reply to Jeff Griffiths (:canuckistani) from comment #25)
> Should we uplift this? If it isn't risky, I'd like to get it in 41.

Yes, I plan to request it once it lands in m-c.
Flags: needinfo?(jryans)
Comment on attachment 8633787 [details]
MozReview Request: Bug 1074180 - Fix linting issues in touch-events.js. r=ochameau

Bug 1074180 - Fix linting issues in touch-events.js. r=ochameau
Attachment #8633787 - Flags: review+
Comment on attachment 8633788 [details]
MozReview Request: Bug 1074180 - Avoid TypeError with touch events on desktop. r=ochameau

Bug 1074180 - Avoid TypeError with touch events on desktop. r=ochameau
Attachment #8633788 - Flags: review+
Comment on attachment 8633789 [details]
MozReview Request: Bug 1074180 - Convert touch simulator to frame script for e10s. r=ochameau

Bug 1074180 - Convert touch simulator to frame script for e10s. r=ochameau
Attachment #8633789 - Flags: review+
Comment on attachment 8633787 [details]
MozReview Request: Bug 1074180 - Fix linting issues in touch-events.js. r=ochameau

Approval Request Comment
[Feature/regressing bug #]: e10s now offered in DE
[User impact if declined]: touch events fail in e10s, confusing users
[Describe test coverage new/current, TreeHerder]: On m-c, updated tests
[Risks and why]: Low, DevTools only
[String/UUID change made/needed]: None
Attachment #8633787 - Flags: review+
Attachment #8633787 - Flags: approval-mozilla-aurora?
Comment on attachment 8633788 [details]
MozReview Request: Bug 1074180 - Avoid TypeError with touch events on desktop. r=ochameau

Approval Request Comment
[Feature/regressing bug #]: e10s now offered in DE
[User impact if declined]: touch events fail in e10s, confusing users
[Describe test coverage new/current, TreeHerder]: On m-c, updated tests
[Risks and why]: Low, DevTools only
[String/UUID change made/needed]: None
Attachment #8633788 - Flags: review+
Attachment #8633788 - Flags: approval-mozilla-aurora?
Comment on attachment 8633789 [details]
MozReview Request: Bug 1074180 - Convert touch simulator to frame script for e10s. r=ochameau

Approval Request Comment
[Feature/regressing bug #]: e10s now offered in DE
[User impact if declined]: touch events fail in e10s, confusing users
[Describe test coverage new/current, TreeHerder]: On m-c, updated tests
[Risks and why]: Low, DevTools only
[String/UUID change made/needed]: None
Attachment #8633789 - Flags: review+
Attachment #8633789 - Flags: approval-mozilla-aurora?
Comment on attachment 8633787 [details]
MozReview Request: Bug 1074180 - Fix linting issues in touch-events.js. r=ochameau

Try push looks good and has been in m-c for a few days now. Let's land it in Aurora.
Attachment #8633787 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8633788 [details]
MozReview Request: Bug 1074180 - Avoid TypeError with touch events on desktop. r=ochameau

Try push looks good and has been in m-c for a few days now. Let's land it in Aurora.
Attachment #8633788 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8633789 [details]
MozReview Request: Bug 1074180 - Convert touch simulator to frame script for e10s. r=ochameau

Try push looks good and has been in m-c for a few days now. Let's land it in Aurora.
Attachment #8633789 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: