583218 - "Web Console" menu item should be checked when the Web Console is open

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Scoobidiver (away)]

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][⌚️UTC+2]]

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]]

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)]

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]]

(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]

See also bug 588946.

Comment 7

[Dietrich Ayala (:dietrich)]

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]

Also please ensure we dont have issue 1 in bug 595106

Comment 10

[Scoobidiver (away)]

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 13

[Lucio Valentin]

Attachment: A patch with the implementation for the feature requested.

Comment 14

[Dão Gottwald [:dao]]

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]]

Looks good otherwise! Can you attach a new patch with these changes made? I'll review it.

Comment 16

[Rogério Gonçalves]

Attachment: A correction of previous patch reviewed by dao.

Comment 17

[Rogério Gonçalves]

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]]

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]

Attachment: Second correction of previous patch reviewed by dao.

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 587677 [details] [diff] [review]
Second correction of previous patch reviewed by dao.

thanks!

Comment 21

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a157f6aab51b

Comment 22

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/a157f6aab51b

Comment 23

[Rogério Gonçalves]

Scoobidiver, We are working to fix the bug 719850, I've submitted a patch for review.