609866 - Fennec fires contextmenu event in content during panning/tapping

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Panning/Zooming, defect)

Description

[navapramuk]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8 GTB7.1
Build Identifier: 

On Touch screen mobile devices fennec does not recognize properly the panning and treats that as a right click and triggers JS alert added on the page to inform user right click is not allowed 

Reproducible: Always

Steps to Reproduce:
1. Open that attached test page with Fennec on N900
2. Try to pan on the page using finger and stylus
3.
Actual Results:  
JS alert message is triggered during panning

Expected Results:  
Panning should be smooth and Right click disallowed alert should not trigger

Comment 1

[navapramuk]

Attachment: test page

Comment 2

[navapramuk]

About:firefox shows Mozilla/5.0 (Maemo;Linux armV71;rv:2.0b8pre)Gecko/20101103 Firefox/4.0b8pre Fennec/4.0b2Pre

Comment 3

[navapramuk]

This issue is also seen on Fennec-Qt version N900

About:firefox shows Mozilla/5.0 (Maemo;Linux armV71;rv:2.0b8pre)Gecko/20101022
Firefox/4.0b8pre Fennec/4.0b2Pre

Comment 4

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Confirmed on android and maemo:

Mozilla/5.0 (Maemo;Linux armv71; rv:2.0b8pre) Gecko/20101108 Firefox/4.0b8pre Fennec/4.0b3pre
Mozilla/5.0 (Android; Linux armv71; rv2.0b8pre) Gecko/20101108 Firefox/4.0b8pre Fennec/4.0b3pre
Mozilla/5.0 (X11; Linux i686; rv:2.0b8pre) Gecko/20101108 Firefox/4.0b8pre Fennec/4.0b3pre

Placed html at : 
http://people.mozilla.com/~nhirata/html_tp/RightClickDisable.html

Note: does not occur in Desktop Firefox

Comment 5

[navapramuk]

Issue reproducible with on N900

latest-mozilla-central-maemo5-qt/fennec_4.0~b3~20101112073342_armel.deb

Comment 6

[Wesley Johnston (:wesj)]

We are intentionally firing a contextmenu event on mousedown for pages so that we can build and cache the context menu results:

http://mxr.mozilla.org/mobile-browser/source/chrome/content/content.js#415

Anyone who watches for context menu events will pick this up really early in the life of the click.

I think we can do this a bit differently and instead just call straight into ContextHandler.onContextMenu (modified to take an element instead of an event)

http://mxr.mozilla.org/mobile-browser/source/chrome/content/content.js#716

Then we could fire a contextmenu event when the actual contextmenu is shown later, and allow the page to call preventDefault on it. But all of this was done to save messages and time, so maybe its best to not give pages this event at all.

This should also fix Bug 609975.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Remove contextmenu event

This removes all of our instances firing the context menu event.

It actually looks like we were trying to cancel the event if preventDefault was called using getPreventDefault (a jquery method?). We could do that, but sites are not going to want a contextmenu every time a mousedown is performed anyway.

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Wes, I'm taking the patch because I've dup with bug 619824 (in some ways) and I want to continue to fire the contextmenu event (see bug 578018).

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 501060 [details] [diff] [review]
Patch

We seem to have "Browser:ContextMenu" sent from chrome and sent from content. Should we rename the content message -> "Browser:ContextMenu:Return" ?

The only issue is any confusion it might create. If we consider the two messages to be two separate messages, then keeping them is ok. If we consider them to be a single "RPC" style message, we should add ":Return" to the content message.

Thoughts?

r+ but let's discuss before pushing.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #11)
> Comment on attachment 501060 [details] [diff] [review]
> Patch
> 
> We seem to have "Browser:ContextMenu" sent from chrome and sent from content.
> Should we rename the content message -> "Browser:ContextMenu:Return" ?
> 
> The only issue is any confusion it might create. If we consider the two
> messages to be two separate messages, then keeping them is ok. If we consider
> them to be a single "RPC" style message, we should add ":Return" to the content
> message.
> 
> Thoughts?
> 
> r+ but let's discuss before pushing.

I think we should rename one of the message to something else, because the Browser:ContextMenu from the chrome side does not always means it will be a response, when using :Return sounds like it in my opinion.

What do you think of Browser:MouseLong / Content:ContextMenu ?

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #12)

> What do you think of Browser:MouseLong / Content:ContextMenu ?

Browser:MouseLong wins for me

Comment 14

[Wesley Johnston (:wesj)]

Comment on attachment 501060 [details] [diff] [review]
Patch

I like the name changes too (i.e. Browser:MouseLong / Content:ContextMenu). If I'm reading this right, we're no longer passing the context menu information to the parent process on tapdown, and we need to update the comments, right?

I'm not sure about this, but I'm guessing that sites like Google Docs depend on the contextmenu event being initialized with initMouseEvent so that they can position the popup relative to the mouse?

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #14)
> Comment on attachment 501060 [details] [diff] [review]
> Patch
> 
> I like the name changes too (i.e. Browser:MouseLong / Content:ContextMenu). If
> I'm reading this right, we're no longer passing the context menu information to
> the parent process on tapdown, and we need to update the comments, right?
> 

I thought to have updated it, do I have missed one?

> I'm not sure about this, but I'm guessing that sites like Google Docs depend on
> the contextmenu event being initialized with initMouseEvent so that they can
> position the popup relative to the mouse?

I guess you're right this will probably affect the context menu of some HTML editor using contentEditable. We could do that in an other bug, good catch.

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mobile-browser/rev/56701d9f69bd

Comment 17

[Wesley Johnston (:wesj)]

I think the ASCII art should be:

// [parent] mousedown -> ,..* -> TapLong -> Browser:MouseLong
// [child]  Browser:MouseLong -> contextmenu -> Browser:ContextMenu
// [parent] Browser:ContextMenu

And this whole comment is mostly wrong now:

// * = Here some time will elapse. Although we get the context menu we need
//     ASAP, we do not act on the context menu until we receive a LongTap.
//     This is so we can show the context menu as soon as we know it is
//     a long tap, without waiting for child process.

We can fix it somewhere else though... assuming I'm not just confused.

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #17)
> I think the ASCII art should be:
> 
> // [parent] mousedown -> ,..* -> TapLong -> Browser:MouseLong
> // [child]  Browser:MouseLong -> contextmenu -> Browser:ContextMenu
> // [parent] Browser:ContextMenu
> 
> And this whole comment is mostly wrong now:
> 
> // * = Here some time will elapse. Although we get the context menu we need
> //     ASAP, we do not act on the context menu until we receive a LongTap.
> //     This is so we can show the context menu as soon as we know it is
> //     a long tap, without waiting for child process.
> 
> We can fix it somewhere else though... assuming I'm not just confused.

You're not, I've missed this comment.

Comment 20

[Aaron Train [:aaronmt]]

Verified fixed based on attached test-case

Comment 21

[Ioana Chiorean]

https://litmus.mozilla.org/show_test.cgi?id=15214