Items in Developer tools menu have inconsistent check state

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Developer Tools
RESOLVED FIXED
5 years ago
6 months ago

People

(Reporter: Optimizer, Assigned: paul)

Tracking

Trunk
Firefox 15
All
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

5 years ago
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

5 years ago
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?
(Reporter)

Comment 2

5 years ago
Yeah, I basically tested it for Firefox Menu, that is a windows only thing
and presumably on linux as well with the app menu enabled.
(Reporter)

Comment 4

5 years ago
Yes, but I cannot confirm this behavior on Linux as I do not use one.
(Assignee)

Comment 5

5 years ago
I can reproduce on Linux.
(Assignee)

Comment 6

5 years ago
Created attachment 620597 [details] [diff] [review]
patch v1
(Assignee)

Updated

5 years ago
Attachment #620597 - Flags: review?(dcamp)
(Assignee)

Updated

5 years ago
Assignee: nobody → paul

Comment 7

5 years ago
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>?
(Assignee)

Comment 8

5 years ago
(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.

Updated

5 years ago
Attachment #620597 - Flags: review?(dcamp) → review+
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 (...)
(Assignee)

Comment 10

5 years ago
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).
(Assignee)

Comment 11

5 years ago
Created attachment 624003 [details] [diff] [review]
patch v2

Using <command>. Before asking for review, I need to test on Windows and Mac.
(Assignee)

Updated

5 years ago
Attachment #620597 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
Created attachment 624320 [details] [diff] [review]
patch v2.1

windows and mac fix
(Assignee)

Updated

5 years ago
Attachment #624320 - Flags: review?(dao)
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?
(Assignee)

Comment 14

5 years ago
(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
(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...

Updated

5 years ago
Depends on: 756759
(Assignee)

Comment 16

5 years ago
(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.
(Assignee)

Comment 17

5 years ago
Oh, I see (just saw bug 756759). The !important flags shouldn't really be needed in toolkit.
I'll update my patch.
(Assignee)

Comment 18

5 years ago
Created attachment 625615 [details] [diff] [review]
patch v2.2
(Assignee)

Updated

5 years ago
Attachment #624320 - Attachment is obsolete: true
Attachment #624320 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
Attachment #624003 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #625615 - Flags: review?(dao)
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.
(Assignee)

Comment 20

5 years ago
Created attachment 625623 [details] [diff] [review]
patch v2.3
(Assignee)

Updated

5 years ago
Attachment #625615 - Attachment is obsolete: true
Attachment #625615 - Flags: review?(dao)
(Assignee)

Comment 21

5 years ago
Comment on attachment 625623 [details] [diff] [review]
patch v2.3

Hmm, I didn't see the change in winstripe.
Attachment #625623 - Flags: review?(dao)

Updated

5 years ago
Attachment #625623 - Flags: review?(dao) → review+
(Assignee)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
(Assignee)

Comment 22

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/f789661fed75
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
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
(Assignee)

Updated

5 years ago
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 24

5 years ago
Created attachment 627183 [details] [diff] [review]
patch v2.4

fixed tests
(Assignee)

Updated

5 years ago
Attachment #625623 - Attachment is obsolete: true
(Assignee)

Comment 25

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/5ddd48e50e8c
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/5ddd48e50e8c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
(Reporter)

Comment 27

5 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 29

5 years ago
Okay. may be mark that bug blocking this one ?
You need to log in before you can comment on or make changes to this bug.