Closed
Bug 1421225
Opened 6 years ago
Closed 6 years ago
Clicking on a console.group Message should toggle the group
Categories
(DevTools :: Console, enhancement)
DevTools
Console
Tracking
(firefox59 wontfix, firefox60 fixed)
RESOLVED
FIXED
Firefox 60
People
(Reporter: nchevobbe, Assigned: abhinav.koppula, Mentored)
References
Details
(Keywords: good-first-bug)
Attachments
(1 file)
Steps to reproduce: 1. Open the console 2. Evaluate `console.group();console.log("in group")` 3. Click on the "<no group label>" Expected results: The group collapses Actual results: Nothing happens. You need to specifically click the arrow to toggle the group --- Clicking anywhere on the message should toggle the group, so we should put the onClick prop from [1] on the container [2] [1] https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/devtools/client/webconsole/new-console-output/components/Message.js#173-179 [2] https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/devtools/client/webconsole/new-console-output/components/Message.js#243-245
Reporter | ||
Updated•6 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•6 years ago
|
||
Hi Nicolas, I've taken a stab at this issue. Is this what you had in mind?
Reporter | ||
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8932440 [details] Bug 1421225 - Clicking on a console.group Message should toggle the group. https://reviewboard.mozilla.org/r/203492/#review208956 ::: devtools/client/webconsole/new-console-output/components/Message.js:242 (Diff revision 1) > + onClick: function () { > + if (open) { > + dispatch(actions.messageClose(messageId)); > + } else { > + dispatch(actions.messageOpen(messageId)); > - } > + } we need to check that the message is `collapsible` before adding the listener.
Attachment #8932440 -
Flags: review?(nchevobbe) → review-
Comment hidden (mozreview-request) |
Reporter | ||
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8932440 [details] Bug 1421225 - Clicking on a console.group Message should toggle the group. https://reviewboard.mozilla.org/r/203492/#review209054 The patch looks good to me, thanks Abhinav ! It would be nice if we could have some tests on this. We need to check that clicking the group name actually toggle it. You can add a mocha test similar to this one https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/devtools/client/webconsole/new-console-output/test/components/console-api-call.test.js#244 , but with targetting the label, and not the arrow button.
Attachment #8932440 -
Flags: review?(nchevobbe) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
Hi Nicolas, Have added the test as you mentioned. Also, there was a test failing due to my patch - PageError component: displays a [Learn more] link the reason(I think) being that there was no prevention of propagation of event in onLearnMoreClick due to which even the parent's(message) click handler was being invoked and it failed with this error: 1) PageError component: displays a [Learn more] link: TypeError: dispatch is not a function at dom.div.onClick (new-console-output/components/Message.js:246:11) I have stopped the propagation and now all tests pass. I hope I handled it the right way. Thanks Abhinav
Reporter | ||
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8932440 [details] Bug 1421225 - Clicking on a console.group Message should toggle the group. https://reviewboard.mozilla.org/r/203492/#review211324 a small comment, but i think it is good. let's oush to try to see if everything is okay ::: devtools/client/webconsole/new-console-output/test/components/console-api-call.test.js:244 (Diff revision 3) > - it("toggle the group when the collapse button is clicked", () => { > + function toggleConsoleGroup(className) { > const store = setupStore([]); > store.dispatch = sinon.spy(); > const message = stubPreparedMessages.get("console.group('bar')"); > > let wrapper = mount(Provider({store}, > ConsoleApiCall({ > message, > open: true, > dispatch: store.dispatch, > serviceContainer, > }) > )); > - wrapper.find(".theme-twisty.open").simulate("click"); > + wrapper.find(className + ".open").simulate("click"); > let call = store.dispatch.getCall(0); > expect(call.args[0]).toEqual({ > id: message.id, > type: MESSAGE_CLOSE > }); > > wrapper = mount(Provider({store}, > ConsoleApiCall({ > message, > open: false, > dispatch: store.dispatch, > serviceContainer, > }) > )); > - wrapper.find(".theme-twisty").simulate("click"); > + wrapper.find(className).simulate("click"); > call = store.dispatch.getCall(1); > expect(call.args[0]).toEqual({ > id: message.id, > type: MESSAGE_OPEN > }); > + } > + > + it("toggle the group when the collapse button is clicked", () => { > + toggleConsoleGroup(".theme-twisty"); > + }); > + > + it("toggle the group when the group name is clicked", () => { > + toggleConsoleGroup(".startGroup"); > }); > }); i'd rather keep thing simple here and have some duplication with this kind of refactor, things can be hard to fix or evolve in the future
Reporter | ||
Updated•6 years ago
|
Assignee: nobody → abhinav.koppula
Status: NEW → ASSIGNED
Reporter | ||
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8932440 [details] Bug 1421225 - Clicking on a console.group Message should toggle the group. https://reviewboard.mozilla.org/r/203492/#review211372 one thing i'm seeing: clicking on the location of a group message also tiggle the group visibility, which is something we don't want and one small nit, can we remove the cursor on the expand arrow ? we don't have such things for the object inspector
Assignee | ||
Updated•6 years ago
|
Attachment #8932440 -
Flags: review+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8932440 -
Flags: review?(nchevobbe)
Reporter | ||
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8932440 [details] Bug 1421225 - Clicking on a console.group Message should toggle the group. https://reviewboard.mozilla.org/r/203492/#review211772 This is looking good, thanks Abhinav. I have some questions and a test request, and we'll be able to ship this on next review round ::: devtools/client/shared/components/tabs/Tabs.js:195 (Diff revision 4) > onClickTab(index, event) { > this.setActive(index); > > if (event) { > event.preventDefault(); > + event.stopPropagation(); why do we need this? ::: devtools/client/webconsole/new-console-output/components/Message.js:99 (Diff revision 4) > + e.stopPropagation(); > + e.preventDefault(); do we need this for console.group ? It is unlikely we have a learn more button in a group message. ::: devtools/client/webconsole/new-console-output/test/components/console-api-call.test.js:280 (Diff revision 4) > + it("toggle the group when the group name is clicked", () => { > + const store = setupStore([]); > + store.dispatch = sinon.spy(); > + const message = stubPreparedMessages.get("console.group('bar')"); can we have a similar test to make sure clicking on the location of a group message does not dispatch ? ::: devtools/client/webconsole/new-console-output/test/components/console-api-call.test.js:300 (Diff revision 4) > + wrapper = mount(Provider({store}, > + ConsoleApiCall({ > + message, > + open: false, > + dispatch: store.dispatch, > + serviceContainer, > + }) > + )); do we need to mount a second time ? can't we reuse the wrapper ?
Attachment #8932440 -
Flags: review?(nchevobbe) → review+
Assignee | ||
Comment 12•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8932440 [details] Bug 1421225 - Clicking on a console.group Message should toggle the group. https://reviewboard.mozilla.org/r/203492/#review211772 > do we need to mount a second time ? can't we reuse the wrapper ? I think we are creating a new wrapper because we want to change the value of `open` from `true` to `false` and hence I don't think we can reuse the same wrapper. let wrapper = mount(Provider({store}, ConsoleApiCall({ message, open: true, dispatch: store.dispatch, serviceContainer, }) )); Let me know if I missed something.
Assignee | ||
Comment 13•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8932440 [details] Bug 1421225 - Clicking on a console.group Message should toggle the group. https://reviewboard.mozilla.org/r/203492/#review211772 Thanks for the review, Nicolas. I have mocked store.dispatch -> store.dispatch = sinon.spy(); after which I could get away with the unncessary e.stopPropagation and e.preventDefault. I have added a test to check if clicking the location link toggles the group by expecting call to be null. ```let call = store.dispatch.getCall(0); expect(call).toNotExist();```
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8932440 -
Flags: review+ → review?(nchevobbe)
Reporter | ||
Comment 15•6 years ago
|
||
mozreview-review |
Comment on attachment 8932440 [details] Bug 1421225 - Clicking on a console.group Message should toggle the group. https://reviewboard.mozilla.org/r/203492/#review220158 Thanks Abhinav ! I have a few comments, but only small things. Otherwise, this is working well. Thanks for adding the tests, they look good to me. ::: devtools/client/themes/common.css:602 (Diff revision 5) > background-image: url("chrome://devtools/skin/images/controls.png"); > background-size: 56px 28px; > } > > .theme-twisty { > - cursor: pointer; > + cursor: default; not sure if we want this change to happen. This component is used all over the toolbox. Maybe we can only override it in webconsole.css, for the new frontend ? ::: devtools/client/webconsole/new-console-output/components/Message.js (Diff revision 5) > - onClick: function () { > - if (open) { > - dispatch(actions.messageClose(messageId)); > - } else { > - dispatch(actions.messageOpen(messageId)); > - } > - }, I think we should remove the onClick props in the CollapseButton component ::: devtools/client/webconsole/new-console-output/test/components/console-api-call.test.js:309 (Diff revision 5) > type: MESSAGE_OPEN > }); > }); > + > + it("toggle the group when the group name is clicked", () => { > + const store = setupStore([]); you should be able to omit the `[]` ::: devtools/client/webconsole/new-console-output/test/components/console-api-call.test.js:345 (Diff revision 5) > + type: MESSAGE_OPEN > + }); > + }); > + > + it("doesn't toggle the group when the location link is clicked", () => { > + const store = setupStore([]); you should be able to omit the `[]` ::: devtools/client/webconsole/new-console-output/test/components/page-error.test.js:74 (Diff revision 5) > expect(text.startsWith("Error: Long error Long error")).toBe(true); > }); > > it("displays a [Learn more] link", () => { > const store = setupStore([]); > - > + store.dispatch = sinon.spy(); why do we need to mock dispatch here ? I don't think we use it to assert things later
Attachment #8932440 -
Flags: review?(nchevobbe) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8932440 [details] Bug 1421225 - Clicking on a console.group Message should toggle the group. https://reviewboard.mozilla.org/r/203492/#review220158 Hi Nicolas, Thanks for the review. I have addressed the comments. I've also made one change of adding stopPropagation in Tabs.js reason being that it was causing the event to propagate to the message element which was causing the message to collapse. Also, there is already a mochitest `browser_webconsole_network_messages_expand` which started failing(timing out) because of the issue mentioned above since on clicking "Cookies" tab it was causing the collapse of the message. Do you think that this test is enough to prevent such a regression or should I enhance this test further?
Reporter | ||
Comment 18•6 years ago
|
||
> Do you think that this test is enough to prevent such a regression or should I enhance this test further?
It should be enough then
Reporter | ||
Comment 19•6 years ago
|
||
mozreview-review |
Comment on attachment 8932440 [details] Bug 1421225 - Clicking on a console.group Message should toggle the group. https://reviewboard.mozilla.org/r/203492/#review220428 ::: devtools/client/shared/components/tabs/Tabs.js:195 (Diff revision 6) > onClickTab(index, event) { > this.setActive(index); > > if (event) { > event.preventDefault(); > + event.stopPropagation(); I tested manually and the problem still persist if you click on the panel "body". Maybe we can keep the onClick on the icon, and add another one on this element https://searchfox.org/mozilla-central/rev/b7e3ec2468d42fa59d86c03ec7afeb209813f1d4/devtools/client/webconsole/new-console-output/components/Message.js#256 , so it does not get triggered when clicking on "attachment" (e.g. the network panel when expanded).
Attachment #8932440 -
Flags: review?(nchevobbe) → review-
Comment hidden (mozreview-request) |
Reporter | ||
Comment 21•6 years ago
|
||
mozreview-review |
Comment on attachment 8932440 [details] Bug 1421225 - Clicking on a console.group Message should toggle the group. https://reviewboard.mozilla.org/r/203492/#review221176 This looks good to me, thanks a lot Abhinav. I only have a small nit, but this is good to land with a green TRY (I just pushed) ::: devtools/client/webconsole/new-console-output/components/Message.js:102 (Diff revision 7) > onLearnMoreClick(e) { > let {exceptionDocURL} = this.props; > this.props.serviceContainer.openLink(exceptionDocURL, e); > } > > + onMessageBodyClick(e) { nit: maybe this could be renamed toggleMessage or something similar ? The name here is not ideal because it can be triggered also by a click on the arrow.
Attachment #8932440 -
Flags: review?(nchevobbe) → review+
Comment hidden (mozreview-request) |
Comment 23•6 years ago
|
||
Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/891aa6495eb9 Clicking on a console.group Message should toggle the group. r=nchevobbe
Comment 24•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/891aa6495eb9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Updated•6 years ago
|
Comment 26•6 years ago
|
||
Version:NIghtly 60 OS:Windows 10 While testing this bug,the group doesn't collapse while clicking group console. The bug is reproduced and needs to be fixed. Ststus:NOT FIXED
Reporter | ||
Comment 27•6 years ago
|
||
It does work for me on 60.0a1 (2018-02-07) (64-bit) Can you tell me exactly what you are doing so I can reproduce the issue ?
Reporter | ||
Updated•6 years ago
|
Flags: needinfo?(aravindarulkalai)
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•