Closed
Bug 840241
Opened 12 years ago
Closed 11 years ago
The inspector-inspect-toolbutton should toggle between turning on/off the picking mode
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 25
People
(Reporter: jaws, Assigned: cpaul37)
Details
(Keywords: polish, Whiteboard: [good-first-bugs][lang=js][mentor=paul])
Attachments
(2 files, 5 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
paul
:
review+
|
Details | Diff | Splinter Review |
If I click on the inspector button in the toolbar, the inspect mode is entered. I expected that clicking it again would exit the inspect mode.
Hi, I'd like to take crack at this one. I'm new to this project, any advice/guidance would be appreciated. Thanks, Phil
Reporter | ||
Comment 2•12 years ago
|
||
Hi Phil, I found the code for the button in two places: 1) http://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector.xul#65 2) http://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/Highlighter.jsm#406 The command attribute is a reference to the name of the function that should be executed when the user clicks on the button. In this case, the command is "inspector.highlighter.unlockAndFocus()" and "this.unlockAndFocus", respectively. Those reference the same function, which is defined at: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/Highlighter.jsm#310 Within unlockAndFocus, you can call |this.hide()| to exit the Inspect mode if there is nothing locked and then return. Let me know if you need help getting your Firefox build up and running. You can join #introduction on irc.mozilla.org and ask questions there too. There is almost always someone online that is willing to help, but it may take them a little bit of time to respond. Hope that helps!
Assignee: nobody → mrphilh
Status: NEW → ASSIGNED
Comment 3•12 years ago
|
||
> clicking it again would exit the inspect mode
What do you mean by that? Hiding the highlighter or locking it?
Jared, Could you clarify this behavior that you are seeing? Are you talking about the Inspect button that is located on the Developer Toolbar that runs across the bottom of the window, or are you referring to the Inspect button that can be added to the Navigation Bar when Firebug is installed? I'm able to recreate the behavior you are talking about with the Firebug Inspect button but not the developer tools one. Is this the same behavior you are experiencing or am I missing something? Thanks, Phil
Reporter | ||
Comment 5•12 years ago
|
||
I am talking about the Inspect button located in the native developer tools at the bottom of the window. See this screenshot, http://screencast.com/t/pIbaXi3Pe (In reply to Paul Rouget [:paul] from comment #3) > > clicking it again would exit the inspect mode > > What do you mean by that? Hiding the highlighter or locking it? Hiding the highlighter.
Comment 6•12 years ago
|
||
> (In reply to Paul Rouget [:paul] from comment #3)
> > > clicking it again would exit the inspect mode
> >
> > What do you mean by that? Hiding the highlighter or locking it?
>
> Hiding the highlighter.
Some definitions, just to make sure we are talking about the same thing:
- the highlighter is the rectangle + bar floating above the page (the bar is named infobar)
- the inspector is the UI located below the page
- the highlighter is "locked" or "unlocked". "unlocked" mean that moving the mouse over the page selects nodes. "locked" mean you can't select a node with the mouse on the page (but you can via the DOM panel)
I understand the need of hiding the highlighter. But using the lock/unlock button is not the right way to do it.
So maybe we could do that:
- in the dropdown menu in the infobar, we add a "hide highlighter" menuitem
- clicking on this menuitem would have 2 consequences:
- hide the highlighter immediately
- when we restart the inspector/toolbox, we don't show the highlighter by default
- clicking again on the highlighter button from the inspector panel would start the highlighter
How does that sound? Would that fit your needs? Also, is it comprehensible?
We also have plans to move the highlighter at the toolbox level (the lock/unlock button would be on the top right corner of the toolbox), that might have some impact on what we want to do here.
Reporter | ||
Comment 7•12 years ago
|
||
Sorry, the terminology here is indeed confusing. I don't think that solution fits my expectations/needs. If the highlighter is unlocked and the user clicks on the highlighter button, I would expect that the previously selected node gets locked (if the node doesn't exist we can default to the root element). I think the extra menuitem in the infobar dropdown adds unnecessary complexity and can cause users to get stuck with a "broken" highlighter.
Comment 8•12 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #7) > Sorry, the terminology here is indeed confusing. > > I don't think that solution fits my expectations/needs. If the highlighter > is unlocked and the user clicks on the highlighter button, I would expect > that the previously selected node gets locked (if the node doesn't exist we > can default to the root element). True, or we could select the element that is currently outlined (so just call `higlighter.lock()`). > I think the extra menuitem in the infobar dropdown adds unnecessary > complexity and can cause users to get stuck with a "broken" highlighter. +1 But then there's no way to hide the highlighter (and I'm personally ok with that).
Updated•12 years ago
|
Blocks: DevToolsPaperCuts
Comment 10•12 years ago
|
||
Hi, I've created a patch (I think correctly) modifying the Inspector unlock button's behavior to what you have described above, toggling between unlocked and locked, and locking on the last element that had focus. It's a a pretty straight forward to line change but I'd appreciate any feed back never the less. Thanks, Phil
Reporter | ||
Updated•12 years ago
|
Attachment #714560 -
Attachment is patch: true
Attachment #714560 -
Flags: feedback?(jAwS)
Reporter | ||
Comment 11•12 years ago
|
||
Comment on attachment 714560 [details] [diff] [review] Patch enabling Inspector unlock button to toggle between locked and unlocked This looks good to me and matches my expectations. Thanks Phil! I've passed this on to Paul for final review.
Attachment #714560 -
Flags: review?(paul)
Attachment #714560 -
Flags: feedback?(jAwS)
Attachment #714560 -
Flags: feedback+
Comment 12•12 years ago
|
||
Comment on attachment 714560 [details] [diff] [review] Patch enabling Inspector unlock button to toggle between locked and unlocked I'd prefer if you'd implement a `toggleLockState` function. `unlockAndFocus` is intended to unlock, no matter what.
Attachment #714560 -
Flags: review?(paul)
Comment 13•12 years ago
|
||
Here is a new patch implementing a separate function to handle toggling between locked and unlocked. This also updates the UI components to call this function.
Reporter | ||
Comment 14•12 years ago
|
||
Comment on attachment 718029 [details] [diff] [review] Added function toggleLockState to Highlighter.jsm Should this call unlockAndFocus() instead of unlock() ? Also, please set the "review?" flag to Paul Rouget when you upload a patch.
Attachment #718029 -
Attachment is patch: true
Attachment #718029 -
Flags: review?(paul)
Comment 15•12 years ago
|
||
I've updated this patch to use the unlockAndFocus() method instead of just unlock().
Attachment #714560 -
Attachment is obsolete: true
Attachment #718029 -
Attachment is obsolete: true
Attachment #718029 -
Flags: review?(paul)
Attachment #718592 -
Flags: review?(paul)
Attachment #718592 -
Flags: review?(jAwS)
Comment 16•12 years ago
|
||
Comment on attachment 718592 [details] [diff] [review] toggleLockState.patch v2 You probably want to bind `this.toggleLockState`: > this.toggleLockState = this.toggleLockState.bind(this); See https://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/Highlighter.jsm#97 Besides that, it looks good. Can you write a test and make sure all the tests in devtools/inspector/test pass?
Attachment #718592 -
Flags: review?(paul)
Attachment #718592 -
Flags: review?(jAwS)
Comment 17•12 years ago
|
||
Phil, could you confirm that you're still working on this?
Flags: needinfo?(mrphilh)
Updated•11 years ago
|
Assignee: mrphilh → nobody
Reporter | ||
Updated•11 years ago
|
Status: ASSIGNED → NEW
Flags: needinfo?(mrphilh)
Assignee | ||
Comment 18•11 years ago
|
||
If no one else is working on this I would be happy to try finishing it up. The current behavior is really annoying me.
Assignee | ||
Comment 20•11 years ago
|
||
Built off existing patch by adding bind for 'this.toggleLockState' and modified existing highlighter test to use 'toggleLockState' and assert that it locks and unlocks as expected. I think I got everything straight but this is my first time touching the Firefox code base and I've never used Mercurial. Let me know if I messed anything up.
Attachment #767659 -
Flags: review?(paul)
Comment 21•11 years ago
|
||
Comment on attachment 767659 [details] [diff] [review] Change inspect button to toggle highlighter lock/unlock Girish, you're ok with this change?
Attachment #767659 -
Flags: feedback?(scrapmachines)
Comment 22•11 years ago
|
||
Comment on attachment 767659 [details] [diff] [review] Change inspect button to toggle highlighter lock/unlock Review of attachment 767659 [details] [diff] [review]: ----------------------------------------------------------------- The patch is correct to what it should do. f+ to that. But, the interaction is not correct. I cannot see the correct use case for the request. Rationale: When I click on the button to enter inspect mode, and hover over the page to hover different nodes, the highlighter is unlocked right now. Now if I have to lock the highlighter using the inspect button again, then I will have to move my mouse down to the toolbox, which will change the selected node in the selection object on the way. Thus when you click on the inspect button to lock the highlighter again, some random node will be selected. Is that alright ? Do we want that ? What is the use case in that ?
Attachment #767659 -
Flags: feedback?(scrapmachines)
Reporter | ||
Comment 23•11 years ago
|
||
(In reply to Girish Sharma [:Optimizer] from comment #22) > Comment on attachment 767659 [details] [diff] [review] > Change inspect button to toggle highlighter lock/unlock > > Review of attachment 767659 [details] [diff] [review]: > ----------------------------------------------------------------- > > The patch is correct to what it should do. f+ to that. > > But, the interaction is not correct. I cannot see the correct use case for > the request. > Rationale: > When I click on the button to enter inspect mode, and hover over the page to > hover different nodes, the highlighter is unlocked right now. Now if I have > to lock the highlighter using the inspect button again, then I will have to > move my mouse down to the toolbox, which will change the selected node in > the selection object on the way. Thus when you click on the inspect button > to lock the highlighter again, some random node will be selected. Is that > alright ? Do we want that ? What is the use case in that ? This is my use case: 1. Open the inspector and select an element that is deep in the page DOM heirarcy. 2. Mess around with the markup view. 3. Try to hit the left-slider arrow on the breadcrumbs, but accidentally hit the Inspect button. How can I undo entering Inspect mode? Presently, I would need to go find that element within the page and click on it again. With this patch, I can just click the button again to get back to my work.
Comment 24•11 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #23) > This is my use case: > 1. Open the inspector and select an element that is deep in the page DOM > heirarcy. > 2. Mess around with the markup view. > 3. Try to hit the left-slider arrow on the breadcrumbs, but accidentally hit > the Inspect button. > > How can I undo entering Inspect mode? Presently, I would need to go find > that element within the page and click on it again. With this patch, I can > just click the button again to get back to my work. Two things : 1) If your mouse did not go outside the toolbox after accidentally hitting the inspect button, then the selection is still the same, so clicking on the node in the markup view should lock it again. 2) If your mouse has already gone outside, then the selection has already changed. Even if you have the button as toggle-able inspect button, the node that will be selected when you click the button again will already be different.
Reporter | ||
Comment 25•11 years ago
|
||
(In reply to Girish Sharma [:Optimizer] from comment #24) > 2) If your mouse has already gone outside, then the selection has already > changed. Even if you have the button as toggle-able inspect button, the node > that will be selected when you click the button again will already be > different. If the user hasn't clicked on a node, we should revert to their previous selection.
Comment 26•11 years ago
|
||
(In reply to Jared Wein [:jaws] from comment #25) > (In reply to Girish Sharma [:Optimizer] from comment #24) > > 2) If your mouse has already gone outside, then the selection has already > > changed. Even if you have the button as toggle-able inspect button, the node > > that will be selected when you click the button again will already be > > different. > > If the user hasn't clicked on a node, we should revert to their previous > selection. Paul - Do we want that ? It will be a bit surprising, imo, but if this is implemented, then the togle-able button makes sense.
Flags: needinfo?(paul)
Comment 27•11 years ago
|
||
Comment 23 makes sense. As far as I can tell, we don't change or remove any feature. Just add a new one. So yeah.
Flags: needinfo?(paul)
Comment 28•11 years ago
|
||
Comment on attachment 767659 [details] [diff] [review] Change inspect button to toggle highlighter lock/unlock Review of attachment 767659 [details] [diff] [review]: ----------------------------------------------------------------- cpaul37, can you please update the patch to have the logic that we discussed in the comments above. Basically, store the last locked on node somewhere, and when the toggle-able button to the left of the breadcrumbs is clicked to lock the inspector, lock the last locked node instead of whatever was highlighted. One additional comment below: ::: browser/devtools/inspector/highlighter.js @@ +415,5 @@ > this.inspectButton = this.chromeDoc.createElement("toolbarbutton"); > this.inspectButton.className = "highlighter-nodeinfobar-button highlighter-nodeinfobar-inspectbutton" > let toolbarInspectButton = this.inspector.panelDoc.getElementById("inspector-inspect-toolbutton"); > this.inspectButton.setAttribute("tooltiptext", toolbarInspectButton.getAttribute("tooltiptext")); > + this.inspectButton.addEventListener("command", this.toggleLockState); This change is actually useless. There is no possible way for a normal human being to reach this button and click it when the highlighter is not locked. (The button is not even visible to begin with, forget being able to reach out to it). So basically, the code flow will always be to the left of the : in ? .. : ..;
Assignee | ||
Comment 29•11 years ago
|
||
That all makes sense. I'll make the changes when I get off work today.
Assignee | ||
Comment 30•11 years ago
|
||
Updated the toggle to revert back to the originally selected node, and added a new section to the test for checking that behavior.
Attachment #767659 -
Attachment is obsolete: true
Attachment #767659 -
Flags: review?(paul)
Attachment #768796 -
Flags: review?(paul)
Assignee | ||
Comment 31•11 years ago
|
||
Realized the "highligter-lock" param for setNode was redundant since I need to call this.lock() anyway to account for the node not having changed. Also added a test to cover this scenario.
Attachment #768796 -
Attachment is obsolete: true
Attachment #768796 -
Flags: review?(paul)
Attachment #768801 -
Flags: review?(paul)
Comment 32•11 years ago
|
||
Comment on attachment 768801 [details] [diff] [review] Removed redundancy from previous patch Comment 28 is incorrect. You actually need to use `toggleLockState` in the infobar as well. We can't *lock* from the infobar, but we can unlock. With your current patch, if the inspector is unlocked from the infobar, nothing is restored when you lock via the inspect button in the inspector.
Attachment #768801 -
Flags: review?(paul) → review-
Updated•11 years ago
|
Whiteboard: [good first bug][mentor=jaws][lang=js] → [good first bug][mentor=paul][lang=js]
Assignee | ||
Comment 33•11 years ago
|
||
Switched back to using toggleLockState in the infobar so the selected node gets remembered.
Attachment #768801 -
Attachment is obsolete: true
Attachment #771045 -
Flags: review?(paul)
Comment 34•11 years ago
|
||
Comment on attachment 771045 [details] [diff] [review] Switched back to using toggleLockState Thanks! https://tbpl.mozilla.org/?tree=Try&rev=6d6356836c5a
Attachment #771045 -
Flags: review?(paul) → review+
Updated•11 years ago
|
Whiteboard: [good first bug][mentor=paul][lang=js] → [land-in-fx-team]
Updated•11 years ago
|
Whiteboard: [land-in-fx-team] → [good-first-bugs][lang=js][mentor=paul][land-in-fx-team]
Comment 35•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/9069fff5c78f
Whiteboard: [good-first-bugs][lang=js][mentor=paul][land-in-fx-team] → [good-first-bugs][lang=js][mentor=paul][fixed-in-fx-team]
Comment 36•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9069fff5c78f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [good-first-bugs][lang=js][mentor=paul][fixed-in-fx-team] → [good-first-bugs][lang=js][mentor=paul]
Target Milestone: --- → Firefox 25
Updated•11 years ago
|
No longer blocks: DevToolsPaperCuts
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•