Closed
Bug 749974
Opened 12 years ago
Closed 12 years ago
Items in Developer tools menu have inconsistent check state
Categories
(DevTools :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 15
People
(Reporter: Optimizer, Assigned: paul)
References
Details
(Whiteboard: [fixed-in-fx-team])
Attachments
(1 file, 5 obsolete files)
16.24 KB,
patch
|
Details | Diff | Splinter Review |
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•12 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•12 years ago
|
||
Yeah, I basically tested it for Firefox Menu, that is a windows only thing
Comment 3•12 years ago
|
||
and presumably on linux as well with the app menu enabled.
Reporter | ||
Comment 4•12 years ago
|
||
Yes, but I cannot confirm this behavior on Linux as I do not use one.
Assignee | ||
Comment 5•12 years ago
|
||
I can reproduce on Linux.
Assignee | ||
Comment 6•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #620597 -
Flags: review?(dcamp)
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → paul
Comment 7•12 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•12 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•12 years ago
|
Attachment #620597 -
Flags: review?(dcamp) → review+
Comment 9•12 years ago
|
||
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•12 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•12 years ago
|
||
Using <command>. Before asking for review, I need to test on Windows and Mac.
Assignee | ||
Updated•12 years ago
|
Attachment #620597 -
Attachment is obsolete: true
Assignee | ||
Comment 12•12 years ago
|
||
windows and mac fix
Assignee | ||
Updated•12 years ago
|
Attachment #624320 -
Flags: review?(dao)
Comment 13•12 years ago
|
||
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•12 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
Comment 15•12 years ago
|
||
(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...
Assignee | ||
Comment 16•12 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•12 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•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #624320 -
Attachment is obsolete: true
Attachment #624320 -
Flags: review?(dao)
Assignee | ||
Updated•12 years ago
|
Attachment #624003 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #625615 -
Flags: review?(dao)
Comment 19•12 years ago
|
||
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•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #625615 -
Attachment is obsolete: true
Attachment #625615 -
Flags: review?(dao)
Assignee | ||
Comment 21•12 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•12 years ago
|
Attachment #625623 -
Flags: review?(dao) → review+
Assignee | ||
Updated•12 years ago
|
Whiteboard: [land-in-fx-team]
Assignee | ||
Comment 22•12 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/f789661fed75
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Comment 23•12 years ago
|
||
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•12 years ago
|
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Comment 24•12 years ago
|
||
fixed tests
Assignee | ||
Updated•12 years ago
|
Attachment #625623 -
Attachment is obsolete: true
Assignee | ||
Comment 25•12 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/5ddd48e50e8c
Whiteboard: [fixed-in-fx-team]
Comment 26•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5ddd48e50e8c
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
Reporter | ||
Comment 27•12 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 → ---
Comment 28•12 years ago
|
||
(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
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 29•12 years ago
|
||
Okay. may be mark that bug blocking this one ?
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•