Closed Bug 920742 Opened 7 years ago Closed 7 years ago

Touch events are not fired for correctly when nested iframes are present

Categories

(Firefox OS Graveyard :: Gaia, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.2 C3(Oct25)

People

(Reporter: kgrandon, Assigned: paul)

References

Details

Attachments

(1 file, 2 obsolete files)

If an app launches a nested frame, touch events are not being fired for it.
Attached file Github pull request (obsolete) —
Hi Vivien, could you review this one?

I'm not entirely sure *why* the events need to be dispatched to the topmost container, I would assume it has something to do with mozbrowser. This seemed to fix my issue of touch events within nested iframes. If you can think of a better way, please let me know.
Attachment #810118 - Flags: review?(21)
Blocks: 920752
Is this a regression? It sounds like it is. Does this need to block 1.2?
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
(In reply to Jason Smith [:jsmith] from comment #2)
> Is this a regression? It sounds like it is. Does this need to block 1.2?

This is for developer workflow only and is not needed for 1.2. QA verification is probably also not necessary.
I thought paul moves some of the touch handler code in the platform. Let's check with him.
Flags: needinfo?(paul)
Is that with regular desktop? desktop + debug profile? Is touch event simulation activated in the responsive mode?

We copied touch-events.js in /browser/devtools/shared/ with some minor modifications.
We also changed how touch events are enabled or not: https://hg.mozilla.org/mozilla-central/rev/acd1d8427718

I don't think that would change anything on how events are propagated.

Also, in your patch, you might want to use nsIDocShellTreeItem.sameTypeRootTreeItem instead of looping.
Flags: needinfo?(paul)
Comment on attachment 810118 [details]
Github pull request

Pau(In reply to Paul Rouget [:paul] from comment #5)
> Is that with regular desktop? desktop + debug profile? Is touch event
> simulation activated in the responsive mode?

This is with a desktop + debug profile.

> We copied touch-events.js in /browser/devtools/shared/ with some minor
> modifications.
> We also changed how touch events are enabled or not:
> https://hg.mozilla.org/mozilla-central/rev/acd1d8427718

Nice! If this is in the platform now, then we should be able to patch that, and clean up gaia a bit.

Clearing the review as we should refactor the patch, and ideally enable the platform touch events if possible. If anyone wants to take this and modify the platform code to use the root for the touch events, feel free to :)
Attachment #810118 - Flags: review?(21)
(In reply to Kevin Grandon :kgrandon from comment #6)
> Clearing the review as we should refactor the patch, and ideally enable the
> platform touch events if possible. If anyone wants to take this and modify
> the platform code to use the root for the touch events, feel free to :)

I can look.

Can you explain what is "the topmost iframe for an app"? In Firefox Desktop, should that be the mozbrowser / mozapp iframe? Or should it be the <xul:browser> (the Firefox tab)?
(In reply to Paul Rouget [:paul] from comment #7)
> Can you explain what is "the topmost iframe for an app"? In Firefox Desktop,
> should that be the mozbrowser / mozapp iframe? Or should it be the
> <xul:browser> (the Firefox tab)?

I noticed that some events were only working if they were dispatched to the mozbrowser iframe, and not iframes nested within them. It seems odd to me, but that's what was happening. The patch I uploaded to iterate through parent iframes seems to have fixed this issue.
Comment on attachment 810118 [details]
Github pull request

Note: I filed bug 921531 to track using the platform touch event library.

It's likely that this patch should now be done in the platform.
Attachment #810118 - Attachment is obsolete: true
Depends on: 921531
Attached patch bug-920742.patch (obsolete) — Splinter Review
Hi Paul - 

This patch seems to fix the problem I was seeing where touch events do not work inside of a nested iframe. If you think there is a better way of doing it, I would be totally fine with you taking over the patch. I was unable to see how to leverage sameTypeRootTreeItem instead of looping though. Please review this if you have time.

Thanks!!
Attachment #812288 - Flags: review?(paul)
Hi Kevin. I'll look at this soon (after the summit).
Comment on attachment 812288 [details] [diff] [review]
bug-920742.patch

Looping is fragile.

Can you try something like that?

> function getTopWindow(win) {
>    let docShell = win.QueryInterface(Ci.nsIInterfaceRequestor)
>                   .getInterface(Ci.nsIWebNavigation)
>                   .QueryInterface(Ci.nsIDocShellTreeItem);
>    let topDocShell = docShell.sameTypeRootTreeItem;
>    topDocShell.QueryInterface(Ci.nsIDocShell); // Not sure this line is needed
>    return topDocShell.contentViewer.DOMDocument.defaultView;
> }

(not tested)
Attachment #812288 - Flags: review?(paul)
Hi Paul, unfortunately I can't seem to get your example to work and am having a hard time finding documentation on this stuff.

I'd love to either find someone to take this patch over, or sit down with you in the next few days and peer program this.
Whiteboard: [c= p=2 s= u=] → [c= p=2 s=2013.10.18 u=]
I'll take care of that.
Assignee: kgrandon → paul
Depends on: 925199
Hi Paul, good news! I tried the latest patch you supplied me (http://pastebin.mozilla.org/3227651), and it seemed to work fine! Perhaps I was not setting the window object correctly when trying it.
Summary: Mouse events are not fired for correctly when nested iframes are present → Touch events are not fired for correctly when nested iframes are present
Attachment #812288 - Attachment is obsolete: true
Attached patch Patch V1Splinter Review
Attachment #815259 - Flags: review?
Attachment #815259 - Flags: review? → review?(mratcliffe)
(commit message in the patch is wrong)
Attachment #815259 - Flags: review?(mratcliffe) → review+
Target Milestone: --- → 1.2 C3(Oct25)
Whiteboard: [c= p=2 s=2013.10.18 u=] → [c= p=2 s= u=]
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/a352097e570f
Keywords: checkin-needed
Whiteboard: [c= p=2 s= u=] → [c= p=2 s= u=][fixed-in-fx-team]
Whiteboard: [c= p=2 s= u=][fixed-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a352097e570f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
You need to log in before you can comment on or make changes to this bug.