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)

enhancement
Not set
normal

Tracking

(firefox59 wontfix, firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

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
Mentor: nchevobbe
Severity: normal → enhancement
Keywords: good-first-bug
Hi Nicolas,
I've taken a stab at this issue.
Is this what you had in mind?
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 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+
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
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
Assignee: nobody → abhinav.koppula
Status: NEW → ASSIGNED
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
Attachment #8932440 - Flags: review+
Attachment #8932440 - Flags: review?(nchevobbe)
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+
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.
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();```
Attachment #8932440 - Flags: review+ → review?(nchevobbe)
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 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?
> 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
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/891aa6495eb9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
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
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 ?
Flags: needinfo?(aravindarulkalai)
Product: Firefox → DevTools
clearing flag
Flags: needinfo?(aravindarulkalai)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: