Last Comment Bug 583218 - "Web Console" menu item should be checked when the Web Console is open
: "Web Console" menu item should be checked when the Web Console is open
Status: RESOLVED FIXED
: ux-consistency
Product: Firefox
Classification: Client Software
Component: Menus (show other bugs)
: Trunk
: All All
-- normal (vote)
: Firefox 12
Assigned To: Lucio Valentin
:
: Jared Wein [:jaws] (please needinfo? me)
Mentors:
: 588946 597347 (view as bug list)
Depends on: 719850
Blocks: FirefoxButton
  Show dependency treegraph
 
Reported: 2010-07-30 04:23 PDT by Scoobidiver (away)
Modified: 2012-01-24 04:09 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
A patch with the implementation for the feature requested. (3.80 KB, patch)
2012-01-10 12:36 PST, Lucio Valentin
no flags Details | Diff | Splinter Review
A correction of previous patch reviewed by dao. (3.54 KB, patch)
2012-01-11 05:12 PST, Rogério Gonçalves
dao+bmo: review+
Details | Diff | Splinter Review
Second correction of previous patch reviewed by dao. (3.57 KB, patch)
2012-01-11 06:10 PST, Rogério Gonçalves
dao+bmo: review+
Details | Diff | Splinter Review

Description User image Scoobidiver (away) 2010-07-30 04:23:11 PDT
User-Agent:       Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b3pre) Gecko/20100729 Minefield/4.0b3pre
Build Identifier: Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b3pre) Gecko/20100729 Minefield/4.0b3pre

As the Web console is not a stand alone window that can be closed by a x button, there must be a V on the left of the "Web console" menu when it is opened (idem "Inspect").

Reproducible: Always
Comment 1 User image Henrik Skupin (:whimboo) 2010-08-23 12:56:07 PDT
This should be a trivial thing to fix. See the solution on bug 584033. Vlado, are you interested?
Comment 2 User image Tan Wei Lin Jason [:Ryuji] 2010-08-24 08:31:58 PDT
As of Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5pre) Gecko/20100824 Minefield/4.0b5pre This issue appear to be fixed.
Comment 3 User image Scoobidiver (away) 2010-08-24 09:02:03 PDT
It is not fixed.

With the classic menu, the Web console menu isn't checked at all.
With the button menu, the Web console menu is not checked according to the selected tab.
Comment 4 User image Vlado Valastiak [:wladow] @ Mozilla.sk 2010-08-24 10:23:39 PDT
(In reply to comment #1)
> This should be a trivial thing to fix. See the solution on bug 584033. Vlado,
> are you interested?

It isn't that trivial, but still should be very easy to fix. I can take a look at this, but it won't be sooner than late this week or early next week. If someone wants to work on this, feel free to do so.
Comment 5 User image Dave Garrett 2010-09-06 00:38:11 PDT
See also bug 588946.
Comment 6 User image Scoobidiver (away) 2010-09-06 02:10:11 PDT
*** Bug 588946 has been marked as a duplicate of this bug. ***
Comment 7 User image Dietrich Ayala (:dietrich) 2010-09-07 06:10:55 PDT
Not going to hold the release for this, blocking-. Would love to get a fix though, if you have the time now Vlado.
Comment 8 User image Biju 2010-09-11 14:51:24 PDT
Also please ensure we dont have issue 1 in bug 595106
Comment 9 User image Scoobidiver (away) 2010-09-12 00:00:26 PDT
*** Bug 595106 has been marked as a duplicate of this bug. ***
Comment 10 User image Scoobidiver (away) 2010-09-12 01:48:09 PDT
Add to comment 3 :
Issue 1 of bug 595106 comment 0 : at Firefox menu it don't automatically uncheck when user close Web Console using the new [X] button.
Comment 11 User image Scoobidiver (away) 2010-09-12 01:49:53 PDT
*** Bug 595106 has been marked as a duplicate of this bug. ***
Comment 12 User image Scoobidiver (away) 2010-09-17 04:06:47 PDT
*** Bug 597347 has been marked as a duplicate of this bug. ***
Comment 13 User image Lucio Valentin 2012-01-10 12:36:53 PST
Created attachment 587429 [details] [diff] [review]
A patch with the implementation for the feature requested.
Comment 14 User image Dão Gottwald [:dao] 2012-01-10 13:27:57 PST
Comment on attachment 587429 [details] [diff] [review]
A patch with the implementation for the feature requested.

>                   <menuitem id="webConsole"
>+							type="checkbox"

nit: indentation is off (you're using tabs instead of spaces)

>+    // Fix Bug 583218 - "Web console" in menu isn't checked (classic) or is wrongly checked (button)

This comment can be omitted.

>+    let webConsoleMenuitem = chromeDocument.getElementById("Tools:WebConsole");
>+    webConsoleMenuitem.setAttribute("checked", true);

chromeDocument.getElementById("Tools:WebConsole").setAttribute("checked", "true");

>+    let webConsoleMenuitem = chromeDocument.getElementById("Tools:WebConsole");
>+    webConsoleMenuitem.setAttribute("checked", false);

chromeDocument.getElementById("Tools:WebConsole").removeAttribute("checked");
Comment 15 User image Dão Gottwald [:dao] 2012-01-10 13:30:07 PST
Looks good otherwise! Can you attach a new patch with these changes made? I'll review it.
Comment 16 User image Rogério Gonçalves 2012-01-11 05:12:21 PST
Created attachment 587660 [details] [diff] [review]
A correction of previous patch reviewed by dao.
Comment 17 User image Rogério Gonçalves 2012-01-11 05:17:12 PST
Dao,
These lines:

>+    let webConsoleMenuitem = chromeDocument.getElementById("Tools:WebConsole");
>+    webConsoleMenuitem.setAttribute("checked", false);

Was changed to:
chromeDocument.getElementById("Tools:WebConsole").setAttribute("checked", false);

This suggestion doesn't work:
chromeDocument.getElementById("Tools:WebConsole").removeAttribute("checked");
Comment 18 User image Dão Gottwald [:dao] 2012-01-11 05:28:17 PST
Comment on attachment 587660 [details] [diff] [review]
A correction of previous patch reviewed by dao.

>--- a/browser/base/content/browser-appmenu.inc	Wed Jan 11 10:41:46 2012 -0200
>+++ b/browser/base/content/browser-appmenu.inc	Wed Jan 11 10:49:21 2012 -0200
>@@ -179,7 +179,7 @@
>         <menupopup id="appmenu_webDeveloper_popup">
>           <menuitem id="appmenu_webConsole"
>                     label="&webConsoleCmd.label;"
>-                    oncommand="HUDConsoleUI.toggleHUD();"
>+                    command="Tools:WebConsole"
>                     key="key_webConsole"/>

I missed this on the first patch. You should add type="checkbox" here, even though it seems to work without it...
Comment 19 User image Rogério Gonçalves 2012-01-11 06:10:31 PST
Created attachment 587677 [details] [diff] [review]
Second correction of previous patch reviewed by dao.
Comment 20 User image Dão Gottwald [:dao] 2012-01-11 06:39:05 PST
Comment on attachment 587677 [details] [diff] [review]
Second correction of previous patch reviewed by dao.

thanks!
Comment 22 User image Matt Brubeck (:mbrubeck) 2012-01-12 08:26:40 PST
https://hg.mozilla.org/mozilla-central/rev/a157f6aab51b
Comment 23 User image Rogério Gonçalves 2012-01-24 04:09:02 PST
Scoobidiver, We are working to fix the bug 719850, I've submitted a patch for review.

Note You need to log in before you can comment on or make changes to this bug.