879293 - Isolating log messages now requires multiple clicks and use to require only one

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

When I open the console now, and try to view only log messages, I have to do like 5+ clicks to turn everything else off..

With the old console I could press one button to get this..

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

IMO a single click should select only one item and a shift click would add items to the selection, I think this is pretty typical behavior.  It is how selecting files is done in a file browser for instance.  This also would preserve the previous existing/previous functionality so that people aren't forced to have to learn new functionality..

This would also mean that the console UI would behave the same as the preferences window still, which is how tabs work too...

Comment 2

[Girish Sharma [:Optimizer]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #1)
> IMO a single click should select only one item and a shift click would add
itym ctrl click would add items to selection, like it happens for multiple select options.

> items to the selection, I think this is pretty typical behavior.  It is how
> selecting files is done in a file browser for instance.  This also would
Shift click selection selects a range of files b/w the two clicks. Ctrl click adds individual files to selection.

Comment 3

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Girish Sharma [:Optimizer] from comment #2)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #1)
> > IMO a single click should select only one item and a shift click would add
> itym ctrl click would add items to selection, like it happens for multiple
> select options.
> 
> > items to the selection, I think this is pretty typical behavior.  It is how
> > selecting files is done in a file browser for instance.  This also would
> Shift click selection selects a range of files b/w the two clicks. Ctrl
> click adds individual files to selection.

right I had my shift and cmd buttons mixed up.

Comment 4

[Mihai Sucan [:msucan]]

I am not sure what is proposed here.

Would you like us to allow a modifier+Click on the filter buttons such that only the messages of the category you click on will show? Such that you do not need to click every button manually to turn off categories you don't need to see.

Which modifier do you prefer?

Comment 5

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Mihai Sucan [:msucan] from comment #4)
> I am not sure what is proposed here.
> 
> Would you like us to allow a modifier+Click on the filter buttons such that
> only the messages of the category you click on will show? Such that you do
> not need to click every button manually to turn off categories you don't
> need to see.

Close, what I want is the other way around.  cmd+click adds the selected type, and a click will select only that type.

> Which modifier do you prefer?

accel

Comment 7

[Wraithan (Chris McDonald) [:wraithan]]

As they are currently button, I'd advocate for the opposite of what :erikvold is asking. The expectation with buttons is that you can enable and disable them with a single unmodified click.

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

Our definitions of enable in this context are different then, and I'd advocate reusing the previous established behaviour that people already know instead of something new they will have to learn.

Comment 9

[Mihai Sucan [:msucan]]

I tend to agree with Chris here. We had these filter buttons in the Web Console for some time already and we did not have complaints about their behavior. I would like to not break expected behavior for our users.

Comment 10

[Rob Campbell [:rc] (:robcee)]

Let's do what Wraithan suggests and make cmd/option-click do a toggle.

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

Hi Mihai, I want to give this bug a shot. I am aware of the xul docs, and will go through them tomorrow. Can you point me in the right direction of what needs to be changed in the source code? Many thanks!

Comment 12

[Mihai Sucan [:msucan]]

Gabriel, thanks for your interest in taking this bug.

Overview of the workflow:

1. Get the firefox source:

https://developer.mozilla.org/en-US/docs/Developer_Guide/Source_Code/Mercurial

2. Build firefox:

https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

3. Make a patch:

https://developer.mozilla.org/en-US/docs/Creating_a_patch

4. Submit the patch:

https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch

If you read through the source code about something you do not know about, you may find documentation here:

1. Mozilla Developer Network - has a ton of info about XUL elements, HTML, JS, DOM, Gecko-specific APIs and more.

http://developer.mozilla.org/

2. MXR (Mozilla Cross-Reference) is a source code search engine - search for symbols you want to learn about, eg. nsIDocument.

http://mxr.mozilla.org/mozilla-central/

3. DXR is a smart source code search tool, similar to MXR but sometimes better.

http://dxr.mozilla.org


Here's an overview of how you can fix this bug:

1. you can see the xul toolbarbuttons for each category of messages in browser/devtools/webconsole/webconsole.xul.

2. in webconsole.js (same folder) you can see the WCF__initUI() and WCF__initFilterButtons() function which add the "click" event listeners for those buttons. Also see the WCF__toggleFilter() function which is the event handler.

3. you need to change the code of the event handler to match the desired behavior. Since there was a bit of discussion about it, the summary is: we would like to allow Accel+Click on the filter buttons such that only the messages of the category you click on will show - toggle all other categories off. Such that you do not need to click every button manually to turn off categories you don't need to see.

Thank you! Please let me know if you have more questions.

Comment 13

[mjh563]

Hi Gabriel, are you still working on this bug? If not, we can free it up for someone else to do.

Comment 14

[Gabriel Luong [:gl] (ΦωΦ)]

Totally forgot about this bug. Currently, working and making progress on it. Thanks for the reminder mjh563!

Comment 15

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 879293.patch

Still need to add test cases to browser/devtools/webconsole/tests/browser_webconsole_bug_601667_filter_buttons.js

Comment 17

[Mihai Sucan [:msucan]]

Comment on attachment 826655 [details] [diff] [review]
879293.patch

Review of attachment 826655 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for your patch Gabriel!

This is working almost fine, but if I try to Ctrl+click of NET, then CSS, then JS, etc... it doesn't toggle between the buttons. You might want to look at the patch for bug 909758 and see how that was done - very close to what you have.

You should go ahead and also write a test. Thanks!

::: browser/devtools/webconsole/webconsole.js
@@ +720,5 @@
>     *        The event that triggered the filter change.
>     */
>    _toggleFilter: function WCF__toggleFilter(aEvent)
>    {
> +    let accelKey = aEvent.ctrlKey || aEvent.metaKey;

Let's use the altKey.

::: browser/devtools/webconsole/webconsole.xul
@@ +48,5 @@
>      <command id="cmd_close" oncommand="goDoCommand('cmd_close');" disabled="true"/>
>    </commandset>
>    <keyset id="consoleKeys">
> +    <key id="key_fullZoomReduce" key="&fullZoomReduceCmd.commandkey;" command="cmd_fullZoomReduce" modifiers="accel"/>
> +    <key key="&fullZoomReduceCmd.commandkey2;" command="cmd_fullZoomReduce" modifiers="accel"/>

Are these changes need for this bug?

Comment 18

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Added test cases.

Comment 19

[Gabriel Luong [:gl] (ΦωΦ)]

::: browser/devtools/webconsole/webconsole.xul
@@ +48,5 @@
>      <command id="cmd_close" oncommand="goDoCommand('cmd_close');" disabled="true"/>
>    </commandset>
>    <keyset id="consoleKeys">
> +    <key id="key_fullZoomReduce" key="&fullZoomReduceCmd.commandkey;" command="cmd_fullZoomReduce" modifiers="accel"/>
> +    <key key="&fullZoomReduceCmd.commandkey2;" command="cmd_fullZoomReduce" modifiers="accel"/>
These changes are not needed for this bug. I was removing extra whitespaces between the attributes. This can be removed from the patch if necessary.

Regarding test cases, I think I can improve it by re-enabling the filter buttons that were turned off after isolating, but perhaps that should be considered a separate bug since I would refactor the code to get the subbutton into its own function for the 2 test functions.

Regarding the patch, the main ideas:
- Get the modifier key, and uncheck all the other filter buttons. Set the state for the associate filters (menu items) to false for these filter buttons. 
- Check and enable all the filters for the target button
- Otherwise, handle as usual

Thanks for the review Mihai!

Comment 20

[Mihai Sucan [:msucan]]

Comment on attachment 827190 [details] [diff] [review]
Added test cases.

Review of attachment 827190 [details] [diff] [review]:
-----------------------------------------------------------------

Good work. Thanks for the update!

I would r+ the patch, but I see the following tests fail:

TEST-UNEXPECTED-FAIL | browser_webconsole_bug_762593_insecure_passwords_web_console_warning.js | Clicking the Insecure Passwords Warning node opens the desired page

TEST-UNEXPECTED-FAIL | browser_webconsole_bug_846918_hsts_invalid-headers.js | Clicking the Learn More Warning node opens the desired page

Maybe the Security category of messages remains disabled after the filter tests. Please make sure the prefs are reset after the test.

::: browser/devtools/webconsole/test/browser_webconsole_bug_601667_filter_buttons.js
@@ +25,5 @@
>  
> +  testIsolateFilterButton("net");
> +  testIsolateFilterButton("css");
> +  testIsolateFilterButton("js");
> +  testIsolateFilterButton("logging");

Add a call for "security" as well.

@@ +151,5 @@
> +  }
> +  ok(subbutton, "we have the subbutton for category " + aCategory);
> +
> +  // Turn on all the filters by alt clicking the main part of the button
> +  altClickButton(subbutton);

Can't we just alt+click the targetButton instead of the subbutton? Try it. If it doesn't work, let it be.

::: browser/devtools/webconsole/webconsole.js
@@ +814,5 @@
> +   *        Button with drop down items to be toggled.
> +   * @param Boolean aState
> +   *        True if the menu item is being toggled on, and false otherwise.
> +   */
> +  setMenuStatus: function WCF_setMenuState(aTarget, aState)

Please make this a private method.

Comment 21

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 879293.patch

Updates since last patch:
- Made _setMenuState private
- Added call for "security" filter for both testMenuFilterButton and testIsolateFilterButton

Added in test cases:
- Check to ensure the menu items are on after isolating for the target filter button
- Refactor out the code to get the subbutton to getMainButton()
- Turned back on all the filter buttons at the end of each test

Thanks for the review Mihai! This should hopefully be the last revision.

Comment 22

[Mihai Sucan [:msucan]]

Comment on attachment 829833 [details] [diff] [review]
879293.patch

Patch looks good! r+

Try push: https://tbpl.mozilla.org/?tree=Try&rev=117554d0a712

If this is green, we can land the patch.

Comment 23

[Mihai Sucan [:msucan]]

Landed: https://hg.mozilla.org/integration/fx-team/rev/44acc55647b6

Comment 24

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

https://hg.mozilla.org/mozilla-central/rev/44acc55647b6

Comment 25

[Mihai Sucan [:msucan]]

We need MDN docs to mention that alt-click on toolbar buttons (filters) allows for quick toggle off for all other categories.