This should only work for non-primitive items (no string, number, null, undefined, Infinity, …). As a first step, it could also contain the actorId of the clicked object.

Comment on attachment 8933429 [details] Bug 1419081 - Open the sidebar when doing ctrl-click on an ObjectInspector. https://reviewboard.mozilla.org/r/204358/#review210086 Thanks Mike, it's working well r- because I'd rather display a lot more content than the actorId actually, so we can: - already have the mechanism to pass the grip around - see any scrolling issue Also, now that we have a way to open the sidebar, I think we can remove the "Toggle Sidebar" button in the filter bar. And this need more testing now : - simple click does not trigger the sidebar - ctrl click does open it - ctrl click on primitives (string, number, …) does not open it - ctrl-click on another object when the sidebar is visible updates the content of the sidebar - ctrl-click on something else in the message does not update the sidebar - ctrl-click on a message with multiple logged object (console.log(obj1, obj2, …)), updates the content of the sidebar - ctrl-click on the same object when the sidebar is visible should not trigger a re-render (in the reducer, we need to return the old state) - closing the sidebar and ctrl-click on an object re-open it Some of these tests can be done in mocha, other in mochitest if needed. ::: devtools/client/webconsole/new-console-output/actions/ui.js:19 (Diff revision 2) > INITIALIZE, > PERSIST_TOGGLE, > PREFS, > SELECT_NETWORK_MESSAGE_TAB, > SIDEBAR_TOGGLE, > + SIDEBAR_SHOW, maybe SHOW_OBJECT_IN_SIDEBAR would be more meaningful ::: devtools/client/webconsole/new-console-output/components/GripMessageBody.js:91 (Diff revision 2) > : null; > } > > + function onClickCapture(e) { > + if (grip.actor && (e.ctrlKey || e.metaKey)) { > + dispatch(actions.sidebarShow(grip.actor)); let's pass the whole grip to the action so we can easily use it later ::: devtools/client/webconsole/new-console-output/components/GripMessageBody.js:136 (Diff revision 2) > onInspectIconClick, > defaultRep: Grip, > }); > } > > - return ObjectInspector(objectInspectorProps); > + return dom.div({onClickCapture}, ObjectInspector(objectInspectorProps)); we should only add the onClick listener if the sideBarVisible pref is enabled ::: devtools/client/webconsole/new-console-output/components/SideBar.js:34 (Diff revision 2) > } > > render() { > let { > sidebarVisible, > + actorId, here we should expect a "grip" prop ::: devtools/client/webconsole/new-console-output/components/SideBar.js:50 (Diff revision 2) > onClick: this.onClickSidebarToggle > }) > ), > dom.aside({ > className: "sidebar-contents" > - }, "Sidebar WIP") > + }, "Sidebar WIP", dom.br(), `Actor ID: ${actorId}`) let's remove the "Sidebar WIP", and put in the sidebar the result of JSON.stringify(grip, null, 2). This will allow us to see if there is any scrolling issue.

Comment on attachment 8933429 [details] Bug 1419081 - Open the sidebar when doing ctrl-click on an ObjectInspector. https://reviewboard.mozilla.org/r/204358/#review210508 this is going in the right direction. I still have some comments, but this might be the last round of review :) ::: devtools/client/webconsole/new-console-output/components/GripMessageBody.js:66 (Diff revision 3) > serviceContainer, > useQuotes, > escapeWhitespace, > mode = MODE.LONG, > + dispatch, > + sidebarToggle, why do we need this ? ::: devtools/client/webconsole/new-console-output/components/GripMessageBody.js:139 (Diff revision 3) > + ? dom.span({onClickCapture}, ObjectInspector(objectInspectorProps)) > + : ObjectInspector(objectInspectorProps); i am afraid this will add complexity to the markup and cause some issue with the css. We should do : if (showObjectInSidebar && grip.actor) { objectInspectorProps.onClickCapture = e => { if (e.ctrlKey || e.metaKey) { dispatch(actions.showObjectInSidebar(grip)); e.stopPropagation(); } } } This way, we only add the listener if the grip is not a primitive ::: devtools/client/webconsole/new-console-output/reducers/ui.js:30 (Diff revision 3) > initialized: false, > networkMessageActiveTabId: PANELS.HEADERS, > persistLogs: false, > sidebarVisible: false, > timestampsVisible: true, > + grip: null we could name it gripInSidebar or something alike so it's more meaningful ::: devtools/client/webconsole/new-console-output/reducers/ui.js:43 (Diff revision 3) > case SIDEBAR_TOGGLE: > return Object.assign({}, state, {sidebarVisible: !state.sidebarVisible}); Here we may want to reset the `grip` property as well. ::: devtools/client/webconsole/new-console-output/reducers/ui.js:43 (Diff revision 3) > return Object.assign({}, state, {persistLogs: !state.persistLogs}); > case TIMESTAMPS_TOGGLE: > return Object.assign({}, state, {timestampsVisible: action.visible}); > case SELECT_NETWORK_MESSAGE_TAB: > return Object.assign({}, state, {networkMessageActiveTabId: action.id}); > case SIDEBAR_TOGGLE: it seems like we will only close this to close the sidebar now (opening will be done with SHOW_OBJECT_IN_SIDEBAR), what do you think of renaming the action SIDEBAR_CLOSE ? ::: devtools/client/webconsole/new-console-output/reducers/ui.js:48 (Diff revision 3) > case SIDEBAR_TOGGLE: > return Object.assign({}, state, {sidebarVisible: !state.sidebarVisible}); > case INITIALIZE: > return Object.assign({}, state, {initialized: true}); > case MESSAGES_CLEAR: > return Object.assign({}, state, {sidebarVisible: false}); same here, we might want to clear the grip property ::: devtools/client/webconsole/new-console-output/reducers/ui.js:50 (Diff revision 3) > case INITIALIZE: > return Object.assign({}, state, {initialized: true}); > case MESSAGES_CLEAR: > return Object.assign({}, state, {sidebarVisible: false}); > + case SHOW_OBJECT_IN_SIDEBAR: > + return Object.assign({}, state, {sidebarVisible: true, grip: action.grip}); we shouldn't return a new state if action.grip is the same as state.grip. That way, we don't re-render the sidebar: if (action.grip === state.grip) { return state; } return Object.assign({}, state, {sidebarVisible: true, grip: action.grip}); ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_close_sidebar.js:65 (Diff revision 3) > + > + let objectNode = hud.ui.outputNode.querySelector(".object-inspector"); > let wrapper = hud.ui.document.querySelector(".webconsole-output-wrapper"); > let onSidebarShown = waitForNodeMutation(wrapper, { childList: true }); > - toggleButton.click(); > + > + EventUtils.synthesizeMouseAtCenter(objectNode, {ctrlKey: true}, hud.ui.window); shouldn't we test cmd on osx as well ? maybe we could have something similar to https://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_closing_after_completion.js#34 in order to send the right properties ? ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_sidebar.js:11 (Diff revision 3) > // Test that the sidebar is hidden when the console is cleared. > > "use strict"; > > -const TEST_URI = "data:text/html;charset=utf8,"; > +const TEST_URI = > + "data:text/html;charset=utf8,<script>console.log({a:1},100,{b:1});</script>"; let's add a "foo" string as well as null and undefined and false console.log({a:1},100,{b:1}, "foo", false, null, undefined) ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_sidebar.js:21 (Diff revision 3) > - info("Click the clear console button"); > - let clearButton = hud.ui.document.querySelector(".devtools-button"); > - clearButton.click(); > + let objectA = findMessage(hud, "a: 1", ".object-inspector"); > + let objectB = findMessage(hud, "b: 1", ".object-inspector"); > + let primitive = findMessage(hud, "100", ".objectBox"); this is a bit misleading to use findMessage to get the object inspectors. We could do this in 2 steps: ``` let message = await findMessage(hud, "foo"); let [objectA, objectB] = message.querySelectorAll("".object-inspector""); … ``` ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_sidebar.js:57 (Diff revision 3) > -async function showSidebar(hud) { > +async function showSidebar(hud, node, expectMutation) { > - let toggleButton = hud.ui.document.querySelector(".webconsole-sidebar-button"); > let wrapper = hud.ui.document.querySelector(".webconsole-output-wrapper"); > let onSidebarShown = waitForNodeMutation(wrapper, { childList: true }); > - toggleButton.click(); > + > + EventUtils.synthesizeMouseAtCenter(node, {ctrlKey: true}, hud.ui.window); we should differentiate osx from the other os to fire with meta = true ::: devtools/client/webconsole/new-console-output/test/store/ui.test.js:57 (Diff revision 3) > + expect(store.getState().ui.sidebarVisible).toEqual(true); > + expect(store.getState().ui.grip).toEqual(message.parameters[0]); > + let state = store.getState().ui; > + > + store.dispatch(actions.showObjectInSidebar(message.parameters[0])); > + expect(store.getState().ui).toEqual(state); weird, was this test green ?

Comment on attachment 8933429 [details] Bug 1419081 - Open the sidebar when doing ctrl-click on an ObjectInspector. https://reviewboard.mozilla.org/r/204358/#review210508 > why do we need this ? It's the pref for whether the sidebar should be visible or not. Unless there's a better way to do it, it gets passed down from ConsoleOutput so the sidebar isn't displayed unless devtools.webconsole.sidebarToggle is true. > i am afraid this will add complexity to the markup and cause some issue with the css. > We should do : > > if (showObjectInSidebar && grip.actor) { > objectInspectorProps.onClickCapture = e => { > if (e.ctrlKey || e.metaKey) { > dispatch(actions.showObjectInSidebar(grip)); > e.stopPropagation(); > } > } > } > > This way, we only add the listener if the grip is not a primitive This doesn't appear to work. I think the ObjectInspector needs to attach the onClickCapture handler. > weird, was this test green ? Yes, it was. I found it kind of strange as well but it's the reason I didn't explicitly return the same state for the same grip in the reducer.

(In reply to Mike Park [:mparkms] from comment #6) > Comment on attachment 8933429 [details] > Bug 1419081 - Open the sidebar when doing ctrl-click on an ObjectInspector. > > https://reviewboard.mozilla.org/r/204358/#review210508 > > > why do we need this ? > > It's the pref for whether the sidebar should be visible or not. Unless > there's a better way to do it, it gets passed down from ConsoleOutput so the > sidebar isn't displayed unless devtools.webconsole.sidebarToggle is true. Oh right, I forgot about this. Could this be renamed to something more meaningful (sideBarTogglePref for example) ? > > i am afraid this will add complexity to the markup and cause some issue with the css. > > We should do : > > > > if (showObjectInSidebar && grip.actor) { > > objectInspectorProps.onClickCapture = e => { > > if (e.ctrlKey || e.metaKey) { > > dispatch(actions.showObjectInSidebar(grip)); > > e.stopPropagation(); > > } > > } > > } > > > > This way, we only add the listener if the grip is not a primitive > > This doesn't appear to work. I think the ObjectInspector needs to attach the > onClickCapture handler. Okay I see. We should thus add something in the ObjectInspector to deal with Ctrl + click. This means we would be blocked on a new reps bundle release, which is not great. I think what we should now is : - have a simple ">" button next to the object inspector to call the object in sidebar action - file a bug on devtools-core to allow passing an onCtrlClick prop to the ObjectInspector - file a bug on bugzilla, blocked on the next reps bundle release (Bug 1419479 if we are fast enough), to wire the ctrl + click. What do you think of this ? > > weird, was this test green ? > > Yes, it was. I found it kind of strange as well but it's the reason I didn't > explicitly return the same state for the same grip in the reducer. This seems like an issue to me :) Could you roll-back the code change in the reducer (only when we check we don't have the same grip), and make this test fail ? There's either something wrong in the test, or in the code somewhere else.

Comment on attachment 8933429 [details] Bug 1419081 - Open the sidebar when doing ctrl-click on an ObjectInspector. https://reviewboard.mozilla.org/r/204358/#review210670 Thanks for the changes :) Let's discuss my comments on adding a button until we can properly pass onCtrlClick to the objectInspector ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_sidebar.js:24 (Diff revision 6) > - clearButton.click(); > - await waitFor(() => findMessages(hud, "").length == 0); > - let sidebar = hud.ui.document.querySelector(".sidebar"); > - ok(!sidebar, "Sidebar hidden after clear console button clicked"); > + let number = findMessage(hud, "100", ".objectBox"); > + let string = findMessage(hud, "foo", ".objectBox"); > + let bool = findMessage(hud, "false", ".objectBox"); > + let nullMessage = findMessage(hud, "null", ".objectBox"); this could be retrieved by using message.querySelector as well, each type should have a specific class you can target

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #10) > > > i am afraid this will add complexity to the markup and cause some issue with the css. > > > We should do : > > > > > > if (showObjectInSidebar && grip.actor) { > > > objectInspectorProps.onClickCapture = e => { > > > if (e.ctrlKey || e.metaKey) { > > > dispatch(actions.showObjectInSidebar(grip)); > > > e.stopPropagation(); > > > } > > > } > > > } > > > > > > This way, we only add the listener if the grip is not a primitive > > > > This doesn't appear to work. I think the ObjectInspector needs to attach the > > onClickCapture handler. > > Okay I see. We should thus add something in the ObjectInspector to deal with > Ctrl + click. This means we would be blocked on a new reps bundle release, > which is not great. > I think what we should now is : > - have a simple ">" button next to the object inspector to call the object > in sidebar action > - file a bug on devtools-core to allow passing an onCtrlClick prop to the > ObjectInspector > - file a bug on bugzilla, blocked on the next reps bundle release (Bug > 1419479 if we are fast enough), to wire the ctrl + click. > > What do you think of this ? Will the button be temporary? If it's not, I feel like it should be a separate bug. If it is temporary, why don't we just do the context menu entry (bug 1419083) first, then come back to this when the reps bundle has been updated?

clearing the needinfo as speak about this on slack (short: let's do the context menu entry bug)

Comment on attachment 8969630 [details] Bug 1419081 - Cmd/Ctrl + click should put object in the sidebar; . https://reviewboard.mozilla.org/r/238422/#review244190 Need patch from Bug 1452566

Comment on attachment 8969630 [details] Bug 1419081 - Cmd/Ctrl + click should put object in the sidebar; . https://reviewboard.mozilla.org/r/238424/#review245272 As discussed on Slack: This place doesn't work on Win https://searchfox.org/mozilla-central/source/devtools/client/shared/components/reps/reps.js#5645 Honza

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #19) > Comment on attachment 8969630 [details] > Bug 1419081 - Cmd/Ctrl + click should put object in the sidebar; . > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/238424/diff/3-4/ Looks like the test added in this patch is failing on linux.

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #20) > (In reply to Nicolas Chevobbe [:nchevobbe] from comment #19) > > Comment on attachment 8969630 [details] > > Bug 1419081 - Cmd/Ctrl + click should put object in the sidebar; . > > > > Review request updated; see interdiff: > > https://reviewboard.mozilla.org/r/238424/diff/3-4/ > > Looks like the test added in this patch is failing on linux. It was a silly mistake in the test, everything looks good on TRY now

Comment on attachment 8969630 [details] Bug 1419081 - Cmd/Ctrl + click should put object in the sidebar; . https://reviewboard.mozilla.org/r/238424/#review261800 Thanks for taking over this issue Nicolas! Looks good to me, just two minor inline comments Also, works for me, tested in Win10. One UI comment. It's probably out of scope of this bug, but the default width of the Side bar feels a bit small (and it isn't remembered in prefs). I'll attach a screenshot of what I see on Win10. R+ assuming try is green Honza ::: devtools/client/webconsole/actions/ui.js:79 (Diff revision 5) > }; > } > > -function showObjectInSidebar(actorId, messageId) { > +// Given an actor string and a messageId, the function will dispatch a > +// SHOW_OBJECT_IN_SIDEBAR action of the actor represents one of the parameter displayed > +// in the message whose id is messageId. Shouldn't we rather use jsdoc formatted comments like: /** * @param ... */ For commenting functions? ::: devtools/client/webconsole/test/mochitest/browser_webconsole_object_ctrl_click.js:67 (Diff revision 5) > + type: "click", > + [isMacOS ? "metaKey" : "ctrlKey"]: true > + }, cNode, hud.ui.window); > + > + objectInspectors = [...sidebarContents.querySelectorAll(".tree")]; > + is(objectInspectors.length, 1, "There is still only one object inspectors"); nit: plural typo "only one object inspectors" => "only one object inspector"

We should make sure that - at least preserving the width - is covered by an existing report. Honza

Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b835280b7c59 Cmd/Ctrl + click should put object in the sidebar; r=Honza.

Thanks for the review Honza. I fixed the 2 comments, and created Bug 1473556 for the sidebar width persistence.