Closed Bug 1307239 Opened 8 years ago Closed 7 years ago

Fix context menu items in new console frontend

Categories

(DevTools :: Console, defect, P2)

Product:
DevTools
Component:
Console
Type:
defect

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: bgrins, Assigned: jdescottes)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug 1307239 - add context menu to new console frontend;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Bug 1307239 - Fix openNewTabAndConsole to use URL provided as argument;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details 
The context menu isn't working correctly in the new frontend.  In some cases menu items aren't being removed when they should, and in other cases they just don't work when selecting them.
Can i work on this bug??
(In reply to Richa from comment #1)
> Can i work on this bug??

Hi Richa, thanks for the interest.  This bug isn't really a good first bug, but we have a dashboard that should help you find a good one at http://firefox-dev.tools/.
Priority: P1 → P2
Blocks: 1318315
Stealing the bug :)
Assignee: bgrinstead → jdescottes
Comment on attachment 8822242 [details]
Bug 1307239 - Fix openNewTabAndConsole to use URL provided as argument;

review flag got in unwillingly...
Attachment #8822242 - Flags: review?(chevobbe.nicolas)
Comment on attachment 8821921 [details]
Bug 1307239 - add context menu to new console frontend;

https://reviewboard.mozilla.org/r/101000/#review101790

Some minotr comments, looks good!

::: devtools/client/webconsole/new-console-output/components/console-output.js:30
(Diff revision 4)
>      serviceContainer: PropTypes.shape({
>        attachRefToHud: PropTypes.func.isRequired,
>      }),

Add openContextMenu function in propTypes

::: devtools/client/webconsole/new-console-output/components/console-output.js:61
(Diff revision 4)
> +  onContextMenu(e) {
> +    this.props.serviceContainer.openContextMenu(e);
> +  },

I don't think we're preventing the default behaviour of context menu anywhere. 
Don't this also show the regular context menu ?
Maybe we don't see it here because the console is still wrapped in a XUL document ?

::: devtools/client/webconsole/new-console-output/utils/context-menu.js:45
(Diff revision 4)
> +    click: () => {
> +      let evalString = `{ let i = 0;
> +        while (this.hasOwnProperty("temp" + i) && i < 1000) {
> +          i++;
> +        }
> +        this["temp" + i] = _self;

out of curiosity, what does `_self` refers to here ?
Attachment #8821921 - Flags: review+
Comment on attachment 8822241 [details]
Bug 1307239 - add option to toggle new console frontend

https://reviewboard.mozilla.org/r/101212/#review101788

::: devtools/client/framework/toolbox-options.xhtml:50
(Diff revision 1)
> +        <label title="&options.screenshot.clipboard.tooltip;">
> +          <input type="checkbox"
> +                 id="devtools-new-webconsole"
> +                 data-pref="devtools.webconsole.new-frontend-enabled"/>
> +          <span>Enable new console frontend</span>
> +        </label>

I'm curious why you added this, since the new console will (hopefully) be enabled everywhere ?
Comment on attachment 8822242 [details]
Bug 1307239 - Fix openNewTabAndConsole to use URL provided as argument;

https://reviewboard.mozilla.org/r/101214/#review101794

Wow, thanks for fixing that !
I guess we were "lucky" to follow the same pattern in all the tests
Attachment #8822242 - Flags: review+
Attachment #8822241 - Attachment is obsolete: true
Attachment #8822243 - Attachment is obsolete: true
Comment on attachment 8821921 [details]
Bug 1307239 - add context menu to new console frontend;

https://reviewboard.mozilla.org/r/101000/#review101790

> out of curiosity, what does `_self` refers to here ?

It's automagically bound to the object corresponding to the actor provided to jsterm.requestEvaluation:
http://searchfox.org/mozilla-central/source/devtools/client/webconsole/jsterm.js#486-509
Comment on attachment 8822241 [details]
Bug 1307239 - add option to toggle new console frontend

https://reviewboard.mozilla.org/r/101212/#review101788

> I'm curious why you added this, since the new console will (hopefully) be enabled everywhere ?

Don't worry that's just a case of me using mozreview as a backup for my changes :) 

I don't intend to actually push this changeset for review. 
It makes it easier for me to compare the old and the new console quickly. A better hack would be to be able to have both running as separate tools!
Let's go for a first review round! As discussed on IRC, the "Copy" menu item is not on-par with the old console yet, but we can already check the approach I used so far.
Comment on attachment 8821921 [details]
Bug 1307239 - add context menu to new console frontend;

Ah I wanted to ask for a re-review here, as the patch changed quite a bit.
Attachment #8821921 - Flags: review+ → review?(chevobbe.nicolas)
Comment on attachment 8821921 [details]
Bug 1307239 - add context menu to new console frontend;

https://reviewboard.mozilla.org/r/101000/#review103634

Overall this looks good.
I'm still unsure about having the context menu opening in 2 parts (`addContextMenuInfo` and `showContextMenu`), since we have to manage a state for the context menu in addition to the console Redux state. But that can be something we think about in the future, see if it can be integrated into Redux and if it makes sense. I'd say that you patch is good as is, so r+ !

::: devtools/client/webconsole/new-console-output/components/grip-message-body.js:63
(Diff revision 6)
>      onDOMNodeMouseOver = (object) => serviceContainer.highlightDomElement(object);
>      onDOMNodeMouseOut = serviceContainer.unHighlightDomElement;
>    }
>  
> -  return (
> +  return dom.span({
> +    onContextMenu: (e) => props.serviceContainer.addContextMenuInfo({ grip })

Doesn't this bubble up to the `Message` component `onContextMenu` listener too ?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_entire_message.js:37
(Diff revision 6)
> +    yield waitForClipboardPromise(() => copyMenuItem.click(), (data) => {
> +      console.log(data);
> +      clipboardText = data;
> +      return clipboardText === message.textContent;
> +    });
> +
> +    ok(clipboardText, "Clipboard text was found and saved");

From what I understand from the waitForClipboardPromise function i think we could simplify this ?

The function returns a Promise, resolving if the second argument function returns true, or rejects.

Given this, I think we could do something like : 
```
yield waitForClipboardPromise(
  () => copyMenuItem.click(), 
  data =>  data === message.textContent;
);
ok(true, "Clipboard text was found and saved");
```

I can be wrong I can't remember if the test fails if a yielded promise is rejected.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_entire_message.js:38
(Diff revision 6)
> +    let copyMenuItem = menuPopup.querySelector("#console-menu-copy");
> +    ok(copyMenuItem, "copy menu item is enabled");
> +
> +    let clipboardText;
> +    yield waitForClipboardPromise(() => copyMenuItem.click(), (data) => {
> +      console.log(data);

I think we can remove this.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_entire_message.js:45
(Diff revision 6)
> +    // The rest of this test was copied from the old console frontend. The copy menu item
> +    // logic is not on par with the one from the old console yet.

Maybe we could add a TODO saying that it should be done when the logic here is the same as the one in the old console (or even better file a bug for that and add the reference here)

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_link_location.js:12
(Diff revision 6)
> +// messages and copies the expected URL.
> +
> +"use strict";
> +
> +const TEST_URI = "http://example.com/browser/devtools/client/webconsole/" +
> +  "new-console-output/test/mochitest/test-console.html?_date=" + Date.now();

I'm curious what the `_date` parameter is for ?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_copy_link_location.js:56
(Diff revision 6)
> +  copyURLItem = menuPopup.querySelector(CONTEXT_MENU_ID);
> +  ok(copyURLItem, "Copy url menu item is available in context menu");
> +
> +  info("Click on Copy URL menu item and wait for clipboard to be updated");
> +  yield waitForClipboardPromise(() => copyURLItem.click(), TEST_URI);
> +  yield hideContextMenu(hud);

maybe we could have an `ok(true, "Expected text was copied into the clipboard")`or something alike ?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_open_url.js:71
(Diff revision 6)
> +    gBrowser.tabContainer.addEventListener("TabOpen", function onTabOpen(evt) {
> +      gBrowser.tabContainer.removeEventListener("TabOpen", onTabOpen, true);
> +      let newTab = evt.target;
> +      newTab.linkedBrowser.addEventListener("load", function onTabLoad() {
> +        newTab.linkedBrowser.removeEventListener("load", onTabLoad, true);
> +        resolve(newTab);
> +      }, true);
> +    }, true);

We could make good use of the `once` option on addEventListener here , instead of removing the listener in the callback

```
gBrowser.tabContainer.addEventListener("TabOpen", function onTabOpen(evt) {
  let newTab = evt.target;
  newTab.linkedBrowser.addEventListener("load", function onTabLoad() {
    resolve(newTab);
  }, {
    once: true, 
    capture: true
  });
}, {once: true})
```

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_store_as_global.js:7
(Diff revision 6)
> +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Test the Store as global variable menu item of the webconsole is enabled only when
> +// clicking on messages that can are associated with an object actor.

nit: s/that can are associated/that are associated
Attachment #8821921 - Flags: review?(chevobbe.nicolas) → review+
Blocks: 1329606
Comment on attachment 8821921 [details]
Bug 1307239 - add context menu to new console frontend;

https://reviewboard.mozilla.org/r/101000/#review103634

> Doesn't this bubble up to the `Message` component `onContextMenu` listener too ?

It does, but I need to know on which grip the context menu was opened. If you log say `console.log(1, {a: 0}, 2, [1, 2])`, the behaviour of the context menu is expected to be different if I right click on 1, 2, on the object or on the array. That's why I started introducing this 2-step mechanism where each component has the chance to say "what was right clicked" so that the console can then decide what should be available in the context menu, and what each command should do.

For instance in my example above, if I use "store as global variable" when clicking on the "Array" part, it should store the array, but if I click on the object inside the array, it should store the object.

However I just saw that something that used to work with the previous console no longer works here. `console.log([1, {a : 1}])` logs `Array [ 1, Object]` but there's no way to actually store the object to a variable, it always takes the array. Will investigate a bit.

> From what I understand from the waitForClipboardPromise function i think we could simplify this ?
> 
> The function returns a Promise, resolving if the second argument function returns true, or rejects.
> 
> Given this, I think we could do something like : 
> ```
> yield waitForClipboardPromise(
>   () => copyMenuItem.click(), 
>   data =>  data === message.textContent;
> );
> ok(true, "Clipboard text was found and saved");
> ```
> 
> I can be wrong I can't remember if the test fails if a yielded promise is rejected.

That's correct, clipboardText is actually supposed to be used in the next part of the test, when you want to really assert what was copied (which is the part I kept commented out here. I can remove it for now.

> Maybe we could add a TODO saying that it should be done when the logic here is the same as the one in the old console (or even better file a bug for that and add the reference here)

Opened https://bugzilla.mozilla.org/show_bug.cgi?id=1329606 & updated the comment.

> I'm curious what the `_date` parameter is for ?

Honestly not sure, this is coming from the existing test and I didn't check if it was mandatory. 
I assumed it was used in order to avoid cache issues? I can try to remove it.

> We could make good use of the `once` option on addEventListener here , instead of removing the listener in the callback
> 
> ```
> gBrowser.tabContainer.addEventListener("TabOpen", function onTabOpen(evt) {
>   let newTab = evt.target;
>   newTab.linkedBrowser.addEventListener("load", function onTabLoad() {
>     resolve(newTab);
>   }, {
>     once: true, 
>     capture: true
>   });
> }, {once: true})
> ```

When I tried to use it, it didn't work here, I assumed addEventListener was implemented differently on those XUL elements ? 
I'll give it another shot.
I think I should have a better solution, using data attributes to know if the context menu has an actor id available. Will upload for review later today.
Comment on attachment 8821921 [details]
Bug 1307239 - add context menu to new console frontend;

https://reviewboard.mozilla.org/r/101000/#review103634

> When I tried to use it, it didn't work here, I assumed addEventListener was implemented differently on those XUL elements ? 
> I'll give it another shot.

I confirm this doesn't work here.
Comment on attachment 8821921 [details]
Bug 1307239 - add context menu to new console frontend;

https://reviewboard.mozilla.org/r/101000/#review103634

> It does, but I need to know on which grip the context menu was opened. If you log say `console.log(1, {a: 0}, 2, [1, 2])`, the behaviour of the context menu is expected to be different if I right click on 1, 2, on the object or on the array. That's why I started introducing this 2-step mechanism where each component has the chance to say "what was right clicked" so that the console can then decide what should be available in the context menu, and what each command should do.
> 
> For instance in my example above, if I use "store as global variable" when clicking on the "Array" part, it should store the array, but if I click on the object inside the array, it should store the object.
> 
> However I just saw that something that used to work with the previous console no longer works here. `console.log([1, {a : 1}])` logs `Array [ 1, Object]` but there's no way to actually store the object to a variable, it always takes the array. Will investigate a bit.

I came up with another approach, much more similar to the first one you reviewed. The `addContextMenuInfo` logic is gone, we now add a data-attribute on variables-view-link elements containing the actor id. This is retrieved when building the context menu.

I also modified a test to check the use case I mentioned in my previous comment.
Comment on attachment 8821921 [details]
Bug 1307239 - add context menu to new console frontend;

Sorry for flagging you again! 

The main changes are in :
* webconsole/new-console-output/components/variables-view-link.js
* webconsole/new-console-output/utils/context-menu.js
* webconsole/new-console-output/components/console-output.js
* webconsole/new-console-output/components/message.js
* webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_store_as_global.js
Attachment #8821921 - Flags: review+ → review?(chevobbe.nicolas)
Comment on attachment 8821921 [details]
Bug 1307239 - add context menu to new console frontend;

https://reviewboard.mozilla.org/r/101000/#review103822

Looks good to me

::: devtools/client/shared/components/reps/grip.js:209
(Diff revision 8)
> -          span({className: "objectBox objectBox-object"},
> +          span({ className: "objectBox objectBox-object" },
>              this.getTitle(object),
>              objectLink({
>                className: "objectLeftBrace",
>                object: object
>              }, "")
>            )
>          );
>        }
>  
>        return (
> -        span({className: "objectBox objectBox-object"},
> +        span({ className: "objectBox objectBox-object" },

weird, mozReview's telling me there is whitespace change here. Can't really see it, but can you rollback the changes on this file, since there aren't changes other than the whitespaces ones ?

::: devtools/client/webconsole/new-console-output/components/grip-message-body.js:60
(Diff revision 8)
> -  return (
> +  return typeof grip === "string"
>      // @TODO once there is a longString rep, also turn off quotes for those.
> -    typeof grip === "string"
> -      ? StringRep({
> +    ? StringRep({
> -        object: grip,
> +      object: grip,
> -        useQuotes: false,
> +      useQuotes: false,
> -        mode: props.mode,
> +      mode: props.mode,
> -        style: styleObject
> +      style: styleObject
> -      })
> +    })
> -      : Rep({
> +    : Rep({
> -        object: grip,
> +      object: grip,
> -        objectLink: VariablesViewLink,
> +      objectLink: VariablesViewLink,
> -        onDOMNodeMouseOver,
> +      onDOMNodeMouseOver,
> -        onDOMNodeMouseOut,
> +      onDOMNodeMouseOut,
> -        defaultRep: Grip,
> +      defaultRep: Grip,
> -        mode: props.mode,
> +      mode: props.mode,
> -      })
> +    });
> -  );
>  }

can you rollback the changes in here too ? I will soon have a patch getting rid of the test, and you don't have anything specific to the context menu here. Thanks :)

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_context_menu_store_as_global.js:6
(Diff revision 8)
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Test the Store as global variable menu item of the webconsole is enabled only when

nit: change 'Test the Store as global variable menu item' to 'Test the "Store as global variable" menu item' to make it a little easier to read
Attachment #8821921 - Flags: review?(chevobbe.nicolas) → review+
Comment on attachment 8821921 [details]
Bug 1307239 - add context menu to new console frontend;

https://reviewboard.mozilla.org/r/101000/#review103634

> I confirm this doesn't work here.

erf :/
Comment on attachment 8821921 [details]
Bug 1307239 - add context menu to new console frontend;

https://reviewboard.mozilla.org/r/101000/#review103822

Thanks for the review! Landing.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eef566d8459a
Fix openNewTabAndConsole to use URL provided as argument;r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/76b323718975
add context menu to new console frontend;r=nchevobbe
Sure, I really like the changes here :)
https://hg.mozilla.org/mozilla-central/rev/eef566d8459a
https://hg.mozilla.org/mozilla-central/rev/76b323718975
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
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: