As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 749974 - Items in Developer tools menu have inconsistent check state
: Items in Developer tools menu have inconsistent check state
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All Windows 7
: -- normal (vote)
: Firefox 15
Assigned To: Paul Rouget [:paul]
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on: 756759
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-28 12:50 PDT by Girish Sharma [:Optimizer]
Modified: 2012-06-15 00:05 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (3.46 KB, patch)
2012-05-02 23:10 PDT, Paul Rouget [:paul]
dcamp: review+
Details | Diff | Splinter Review
patch v2 (10.94 KB, patch)
2012-05-15 05:28 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v2.1 (12.16 KB, patch)
2012-05-16 01:21 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v2.2 (11.43 KB, patch)
2012-05-21 05:28 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v2.3 (11.43 KB, patch)
2012-05-21 06:23 PDT, Paul Rouget [:paul]
dao+bmo: review+
Details | Diff | Splinter Review
patch v2.4 (16.24 KB, patch)
2012-05-25 04:48 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review

Description User image Girish Sharma [:Optimizer] 2012-04-28 12:50:59 PDT
1) Open any developer tool via any method. Be it Menu entry from Developer tool menu, Shortcut, or context menu.
2) The checkbox for the corresponding item is checked in the developer tools menu
3) Now close the developer tool using a method other than the menu entry under developer tools menu.
4) The checkbox in (2) is still checked. Clicking on it will open that developer tool again.
Comment 1 User image Kevin Dangoor 2012-04-30 09:14:30 PDT
It works fine for me on the Mac. I tried a variety of tools with a variety of means for opening or closing the tool. Perhaps this is Windows-only?
Comment 2 User image Girish Sharma [:Optimizer] 2012-04-30 09:18:50 PDT
Yeah, I basically tested it for Firefox Menu, that is a windows only thing
Comment 3 User image Rob Campbell [:rc] (:robcee) 2012-05-01 09:28:10 PDT
and presumably on linux as well with the app menu enabled.
Comment 4 User image Girish Sharma [:Optimizer] 2012-05-01 09:34:29 PDT
Yes, but I cannot confirm this behavior on Linux as I do not use one.
Comment 5 User image Paul Rouget [:paul] 2012-05-01 09:40:21 PDT
I can reproduce on Linux.
Comment 6 User image Paul Rouget [:paul] 2012-05-02 23:10:08 PDT
Created attachment 620597 [details] [diff] [review]
patch v1
Comment 7 User image Dave Camp (:dcamp) 2012-05-03 08:37:27 PDT
Comment on attachment 620597 [details] [diff] [review]
patch v1

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

::: browser/devtools/highlighter/inspector.jsm
@@ +464,1 @@
>  

Shouldn't we be toggling the checked state of the underlying <command>?
Comment 8 User image Paul Rouget [:paul] 2012-05-03 09:13:40 PDT
(In reply to Dave Camp (:dcamp) from comment #7)
> Comment on attachment 620597 [details] [diff] [review]
> patch v1
> 
> Review of attachment 620597 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/highlighter/inspector.jsm
> @@ +464,1 @@
> >  
> 
> Shouldn't we be toggling the checked state of the underlying <command>?

I just tried. It doesn't work. When you click on the menuitem, because its type is checkbox, it will get its own "checked" value, that will override the broadcasted "checked" value.
Comment 9 User image Dão Gottwald [:dao] 2012-05-14 13:45:51 PDT
Comment on attachment 620597 [details] [diff] [review]
patch v1

>+    this.inspectMenuitem.setAttribute("checked", "true");

So why do you keep setting this?

>+    if (this.inspectAppMenuitem) this.inspectAppMenuitem.setAttribute("checked", "true");

new line after if (...)
Comment 10 User image Paul Rouget [:paul] 2012-05-15 03:53:06 PDT
So I figured out that setting the attribute to false instead of removing the attribute works. I'll switch to a <command> approach (I'll need to update the CSS as we only check the presence of the checked attribute).
Comment 11 User image Paul Rouget [:paul] 2012-05-15 05:28:08 PDT
Created attachment 624003 [details] [diff] [review]
patch v2

Using <command>. Before asking for review, I need to test on Windows and Mac.
Comment 12 User image Paul Rouget [:paul] 2012-05-16 01:21:25 PDT
Created attachment 624320 [details] [diff] [review]
patch v2.1

windows and mac fix
Comment 13 User image Dão Gottwald [:dao] 2012-05-17 07:34:59 PDT
Comment on attachment 624320 [details] [diff] [review]
patch v2.1

>--- a/browser/themes/gnomestripe/browser.css
>+++ b/browser/themes/gnomestripe/browser.css

> .highlighter-nodeinfobar-button {
>   -moz-appearance: none;
>   border: 0 solid hsla(210,8%,5%,.45);
>   padding: 0;
>   width: 26px;
>   min-height: 26px;
>+  background-image: none;
>+}
>+
>+.highlighter-nodeinfobar-button[checked=true] {
>+  background-color: transparent !important;
>+  border-color: hsla(210,8%,5%,.45) !important;
> }

Can you explain this change?
Comment 14 User image Paul Rouget [:paul] 2012-05-18 02:31:01 PDT
(In reply to Dão Gottwald [:dao] from comment #13)
> Comment on attachment 624320 [details] [diff] [review]
> patch v2.1
> 
> >--- a/browser/themes/gnomestripe/browser.css
> >+++ b/browser/themes/gnomestripe/browser.css
> 
> > .highlighter-nodeinfobar-button {
> >   -moz-appearance: none;
> >   border: 0 solid hsla(210,8%,5%,.45);
> >   padding: 0;
> >   width: 26px;
> >   min-height: 26px;
> >+  background-image: none;
> >+}
> >+
> >+.highlighter-nodeinfobar-button[checked=true] {
> >+  background-color: transparent !important;
> >+  border-color: hsla(210,8%,5%,.45) !important;
> > }
> 
> Can you explain this change?

The Inspect button in the infobar now uses the checked attribute broadcasted by the <command> element,
so this style is also used, and need to be overridden:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/gnomestripe/global/toolbarbutton.css#112
Comment 15 User image Dão Gottwald [:dao] 2012-05-19 03:09:31 PDT
(In reply to Paul Rouget [:paul] from comment #14)
> (In reply to Dão Gottwald [:dao] from comment #13)
> > Comment on attachment 624320 [details] [diff] [review]
> > patch v2.1
> > 
> > >--- a/browser/themes/gnomestripe/browser.css
> > >+++ b/browser/themes/gnomestripe/browser.css
> > 
> > > .highlighter-nodeinfobar-button {
> > >   -moz-appearance: none;
> > >   border: 0 solid hsla(210,8%,5%,.45);
> > >   padding: 0;
> > >   width: 26px;
> > >   min-height: 26px;
> > >+  background-image: none;
> > >+}
> > >+
> > >+.highlighter-nodeinfobar-button[checked=true] {
> > >+  background-color: transparent !important;
> > >+  border-color: hsla(210,8%,5%,.45) !important;
> > > }
> > 
> > Can you explain this change?
> 
> The Inspect button in the infobar now uses the checked attribute broadcasted
> by the <command> element,
> so this style is also used, and need to be overridden:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/gnomestripe/
> global/toolbarbutton.css#112

Judging by <http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/global/toolbarbutton.css#106>, it looks like the !important flags shouldn't really be needed...
Comment 16 User image Paul Rouget [:paul] 2012-05-21 05:06:16 PDT
(In reply to Dão Gottwald [:dao] from comment #15)
> (In reply to Paul Rouget [:paul] from comment #14)
> > (In reply to Dão Gottwald [:dao] from comment #13)
> > > Comment on attachment 624320 [details] [diff] [review]
> Judging by
> <http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/
> global/toolbarbutton.css#106>, it looks like the !important flags shouldn't
> really be needed...

I don't use the important flag for winstripe. Only for gnomestripe.
Comment 17 User image Paul Rouget [:paul] 2012-05-21 05:08:45 PDT
Oh, I see (just saw bug 756759). The !important flags shouldn't really be needed in toolkit.
I'll update my patch.
Comment 18 User image Paul Rouget [:paul] 2012-05-21 05:28:15 PDT
Created attachment 625615 [details] [diff] [review]
patch v2.2
Comment 19 User image Dão Gottwald [:dao] 2012-05-21 05:47:59 PDT
Comment on attachment 625615 [details] [diff] [review]
patch v2.2

>--- a/browser/themes/winstripe/browser.css
>+++ b/browser/themes/winstripe/browser.css

> .highlighter-nodeinfobar-button {
>   -moz-appearance: none;
>   border: 0 solid hsla(210,8%,5%,.45);
>   padding: 0;
>   width: 26px;
>   min-height: 26px;
>+  background-image: none;
> }

This looks like it needs an update for the patch in bug 756759.
Comment 20 User image Paul Rouget [:paul] 2012-05-21 06:23:40 PDT
Created attachment 625623 [details] [diff] [review]
patch v2.3
Comment 21 User image Paul Rouget [:paul] 2012-05-21 06:25:12 PDT
Comment on attachment 625623 [details] [diff] [review]
patch v2.3

Hmm, I didn't see the change in winstripe.
Comment 23 User image Ed Morley [:emorley] 2012-05-22 09:53:33 PDT
Sorry, backed out for failures in browser_toolbar_basic.js:
https://tbpl.mozilla.org/?tree=Fx-Team&rev=f789661fed75

{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/shared/test/browser_toolbar_basic.js | inspector button state 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/shared/test/browser_toolbar_basic.js | inspector button state 2
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/shared/test/browser_toolbar_basic.js | inspector button state 7
}

https://hg.mozilla.org/integration/fx-team/rev/512b928b22e7
Comment 24 User image Paul Rouget [:paul] 2012-05-25 04:48:42 PDT
Created attachment 627183 [details] [diff] [review]
patch v2.4

fixed tests
Comment 26 User image Rob Campbell [:rc] (:robcee) 2012-05-26 10:28:53 PDT
https://hg.mozilla.org/mozilla-central/rev/5ddd48e50e8c
Comment 27 User image Girish Sharma [:Optimizer] 2012-06-14 15:02:24 PDT
Can I reopen this bug for Debugger ?
Debugger has the same problem right now.
I open the debugger via Developer tools menu, close it via close button, check still there in developer tools menu.
Comment 28 User image Panos Astithas [:past] 2012-06-14 23:53:08 PDT
(In reply to Girish Sharma [:Optimizer] from comment #27)
> Can I reopen this bug for Debugger ?
> Debugger has the same problem right now.
> I open the debugger via Developer tools menu, close it via close button,
> check still there in developer tools menu.

There is already bug 760839 for that.
Comment 29 User image Girish Sharma [:Optimizer] 2012-06-15 00:05:59 PDT
Okay. may be mark that bug blocking this one ?

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