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 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 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 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 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 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 Dave Garrett 2010-09-06 00:38:11 PDT
See also bug 588946.
Comment 6 Scoobidiver (away) 2010-09-06 02:10:11 PDT
*** Bug 588946 has been marked as a duplicate of this bug. ***
Comment 7 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 Biju 2010-09-11 14:51:24 PDT
Also please ensure we dont have issue 1 in bug 595106
Comment 9 Scoobidiver (away) 2010-09-12 00:00:26 PDT
*** Bug 595106 has been marked as a duplicate of this bug. ***
Comment 10 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 Scoobidiver (away) 2010-09-12 01:49:53 PDT
*** Bug 595106 has been marked as a duplicate of this bug. ***
Comment 12 Scoobidiver (away) 2010-09-17 04:06:47 PDT
*** Bug 597347 has been marked as a duplicate of this bug. ***
Comment 13 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 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 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 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 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 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 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 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 Matt Brubeck (:mbrubeck) 2012-01-12 08:26:40 PST
https://hg.mozilla.org/mozilla-central/rev/a157f6aab51b
Comment 23 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.