The default bug view has changed. See this FAQ.

The inspector-inspect-toolbutton should toggle between turning on/off the picking mode

RESOLVED FIXED in Firefox 25

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jaws, Assigned: Cameron Paul)

Tracking

({polish})

Trunk
Firefox 25
polish
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good-first-bugs][lang=js][mentor=paul])

Attachments

(2 attachments, 5 obsolete attachments)

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.

Comment 1

4 years ago
Hi, 

I'd like to take crack at this one. I'm new to this project, any advice/guidance would be appreciated.

Thanks,
Phil
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

4 years ago
> clicking it again would exit the inspect mode

What do you mean by that? Hiding the highlighter or locking it?

Comment 4

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

4 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.
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

4 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

4 years ago
Blocks: 831711

Comment 9

4 years ago
Created attachment 714560 [details] [diff] [review]
Patch enabling Inspector unlock button to toggle between locked and unlocked

Comment 10

4 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
Attachment #714560 - Attachment is patch: true
Attachment #714560 - Flags: feedback?(jAwS)
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

4 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

4 years ago
Created attachment 718029 [details] [diff] [review]
Added function toggleLockState to Highlighter.jsm

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

4 years ago
Created attachment 718592 [details] [diff] [review]
toggleLockState.patch v2

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)

Updated

4 years ago
Attachment #718592 - Flags: review?(paul)
Attachment #718592 - Flags: review?(jAwS)

Comment 16

4 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)
Phil, could you confirm that you're still working on this?
Flags: needinfo?(mrphilh)

Updated

4 years ago
Assignee: mrphilh → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(mrphilh)
(Assignee)

Comment 18

4 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.
Thanks!
Assignee: nobody → cpaul37
Status: NEW → ASSIGNED
(Assignee)

Comment 20

4 years ago
Created attachment 767659 [details] [diff] [review]
Change inspect button to toggle highlighter lock/unlock

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

4 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 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)
(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.
(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.
(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.
(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

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

4 years ago
That all makes sense. I'll make the changes when I get off work today.
(Assignee)

Comment 30

4 years ago
Created attachment 768796 [details] [diff] [review]
Updated toggle to revert to previously selected node

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

4 years ago
Created attachment 768801 [details] [diff] [review]
Removed redundancy from previous patch

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

4 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

4 years ago
Whiteboard: [good first bug][mentor=jaws][lang=js] → [good first bug][mentor=paul][lang=js]
(Assignee)

Comment 33

4 years ago
Created attachment 771045 [details] [diff] [review]
Switched back to using toggleLockState

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

4 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

4 years ago
Whiteboard: [good first bug][mentor=paul][lang=js] → [land-in-fx-team]

Updated

4 years ago
Whiteboard: [land-in-fx-team] → [good-first-bugs][lang=js][mentor=paul][land-in-fx-team]

Comment 35

4 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]
https://hg.mozilla.org/mozilla-central/rev/9069fff5c78f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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
No longer blocks: 831711
You need to log in before you can comment on or make changes to this bug.