1065714 - Right-click context menu should not be shown on the Hello panel nor the Hello conversation window

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P3)

Description

[Jared Wein [:jaws] (please needinfo? me)]

STR:
Open the Loop panel
Right-click on a blank area outside of the Call URL textbox

Expected:
Nothing happens

Actual:
Get a content context menu that offers to View Source, View Page Info, etc.

This menu is confusing and non-native for browser chrome. The View Source menuitem doesn't work, and the View Page Info doesn't provide any value to the user since this is considered part of the browser.

I think I saw this same bug with the chat window, but I can't get the chat window to work right now to confirm.

Comment 1

[Mark Banner (:standard8)]

This is kinda a duplicate of bug 1029861, but I'm going to add it as a dependency, as we may need Loop-specific things in the menus.

Comment 2

[Maire Reavy [:mreavy]]

Do we need to uplift this in to Fx34?  Don't get me wrong.  I'd take this fix if it were available today, but I think other bugs are more pressing.  (I'm asking if you're ok with my removing loop-uplift.)

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Its presence or interaction within won't cause any functional issues, so we can remove it from the loop-uplift tracking, although it would be nice to get it fixed.

Comment 4

[Maire Reavy [:mreavy]]

Thanks, Jared.

Comment 6

[Mark Banner (:standard8)]

Note: Mike says the key to fixing this would be to disable the context menu on the browser element that's being created in PanelFrame.jsm.

Probably just for Loop for now.

Comment 7

[Mark Banner (:standard8)]

Sevaan: If we just remove the context menu, is that ok with you?

Comment 8

[Sevaan Franks [:sevaan]]

Yes please.

Comment 9

[Fernando Campo (:fcampo)]

Attachment: Right-click context menu should not be shown on Panel nor Conversation

So far I've been unable to create a reliable test to check that the context menu is not being created, any advice is welcome

Also, lets not overload Mark with more reviews :)

Comment 10

[Ed Lee :Mardak]

Comment on attachment 8682445 [details] [diff] [review]
Right-click context menu should not be shown on Panel nor Conversation

>+++ b/browser/components/loop/content/js/panel.jsx
>+    contextMenu: function(e) {
I believe the naming convention is handleContextMenu.
>+      e.preventDefault();
>+      return false;
fcampo: is the return false required? It sounds like from 0.14, return false won't actually do what you want.
https://facebook.github.io/react/docs/events.html#syntheticevent

>+          <div className="fte-get-started-container"
>+               onContextMenu={this.contextMenu}>
We haven't been using => in jsx files... but if we did, I would have suggested:

onContextMenu={e => e.preventDefault()}>

:( ;)


dmose: for test, I wonder if checking for focus is reliable? Some reason I feel that Linux is probably going to do something different. Any suggestions for a test? Or just skip covering this with a unit test?

Comment 11

[Mark Banner (:standard8)]

The "simple" unit test would be to check that e.preventDefault is called when simulating a context click.

Though we probably want to do something more from the mochitest side to ensure that this integrates with the browser as expected (this is a slightly different solution to comment 6, which possibly wouldn't have required a test, but in some ways is nicer as its the content code controlling it).

Comment 12

[Fernando Campo (:fcampo)]

Attachment: Right-click context menu should not be shown on Panel nor Conversation

Tests added (following :Standard8 advice, only checking that preventDefault is called)
Changed function name following convention
Decided not to start using arrow functions to follow same code pattern than it's used (I think it's easier to read with function call in the prop)

Comment 13

[Ed Lee :Mardak]

Comment on attachment 8683637 [details] [diff] [review]
Right-click context menu should not be shown on Panel nor Conversation

I'll land this.

Comment 14

[Ed Lee :Mardak]

Comment on attachment 8683637 [details] [diff] [review]
Right-click context menu should not be shown on Panel nor Conversation

>+++ b/browser/components/loop/test/desktop-local/panel_test.js
>+        TestUtils.Simulate.contextMenu(
>+            view.getDOMNode(),
>+++ b/browser/components/loop/test/desktop-local/roomViews_test.js
>+    it("should NOT show the context menu on right click", function() {
>+        var prevent = sandbox.stub();
Fyi, I fixed the indent when landing.

Comment 15

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/bbdb386545e290c40780aae83218b52ce6708bcd
Bug 1065714 - Right-click context menu should not be shown on Panel nor Conversation [r=Mardak]

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bbdb386545e2