Closed Bug 879293 Opened 11 years ago Closed 11 years ago

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

Categories

(DevTools :: Console, defect, P3)

24 Branch
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 28

People

(Reporter: evold, Assigned: gl)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [good first bug][lang=xul][mentor=msucan])

Attachments

(1 file, 2 obsolete files)

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..
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...
(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.
(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.
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?
(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
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.
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.
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.
Let's do what Wraithan suggests and make cmd/option-click do a toggle.
Priority: -- → P3
Whiteboard: [good first bug][lang=xul][mentor=msucan]
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!
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.
Assignee: nobody → gabriel.luong
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Hi Gabriel, are you still working on this bug? If not, we can free it up for someone else to do.
Flags: needinfo?(gabriel.luong)
Totally forgot about this bug. Currently, working and making progress on it. Thanks for the reminder mjh563!
Flags: needinfo?(gabriel.luong)
Attached patch 879293.patch (obsolete) — Splinter Review
Still need to add test cases to browser/devtools/webconsole/tests/browser_webconsole_bug_601667_filter_buttons.js
Attachment #826655 - Flags: review?(mihai.sucan)
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?
Attachment #826655 - Flags: review?(mihai.sucan)
Attached patch Added test cases. (obsolete) — Splinter Review
::: 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!
Attachment #827190 - Flags: review?(mihai.sucan)
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.
Attachment #827190 - Flags: review?(mihai.sucan)
Attached patch 879293.patchSplinter Review
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.
Attachment #826655 - Attachment is obsolete: true
Attachment #827190 - Attachment is obsolete: true
Attachment #829833 - Flags: review?(mihai.sucan)
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.
Attachment #829833 - Flags: review?(mihai.sucan) → review+
Landed: https://hg.mozilla.org/integration/fx-team/rev/44acc55647b6
Whiteboard: [good first bug][lang=xul][mentor=msucan] → [good first bug][lang=xul][mentor=msucan][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/44acc55647b6
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=xul][mentor=msucan][fixed-in-fx-team] → [good first bug][lang=xul][mentor=msucan]
Target Milestone: --- → Firefox 28
We need MDN docs to mention that alt-click on toolbar buttons (filters) allows for quick toggle off for all other categories.
Keywords: dev-doc-needed
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: