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

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

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

[Phil]

Hi, 

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

Thanks,
Phil

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

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!

Comment 3

[Paul Rouget [:paul]]

> clicking it again would exit the inspect mode

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

Comment 4

[Phil]

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

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

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

[Paul Rouget [:paul]]

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

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

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

[Paul Rouget [:paul]]

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

Comment 9

[Phil]

Attachment: Patch enabling Inspector unlock button to toggle between locked and unlocked

Comment 10

[Phil]

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

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 12

[Paul Rouget [:paul]]

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.

Comment 13

[Phil]

Attachment: 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 14

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 15

[Phil]

Attachment: toggleLockState.patch v2

I've updated this patch to use the unlockAndFocus() method instead of just unlock().

Comment 16

[Paul Rouget [:paul]]

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?

Comment 17

[Josh Matthews [:jdm]]

Phil, could you confirm that you're still working on this?

Comment 18

[Cameron Paul]

If no one else is working on this I would be happy to try finishing it up. The current behavior is really annoying me.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Thanks!

Comment 20

[Cameron Paul]

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

Comment 21

[Paul Rouget [:paul]]

Comment on attachment 767659 [details] [diff] [review]
Change inspect button to toggle highlighter lock/unlock

Girish, you're ok with this change?

Comment 22

[Girish Sharma [:Optimizer]]

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 ?

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

(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

[Girish Sharma [:Optimizer]]

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

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

(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

[Girish Sharma [:Optimizer]]

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

Comment 27

[Paul Rouget [:paul]]

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.

Comment 28

[Girish Sharma [:Optimizer]]

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 ? .. : ..;

Comment 29

[Cameron Paul]

That all makes sense. I'll make the changes when I get off work today.

Comment 30

[Cameron Paul]

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

Comment 31

[Cameron Paul]

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

Comment 32

[Paul Rouget [:paul]]

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.

Comment 33

[Cameron Paul]

Attachment: Switched back to using toggleLockState

Switched back to using toggleLockState in the infobar so the selected node gets remembered.

Comment 34

[Paul Rouget [:paul]]

Comment on attachment 771045 [details] [diff] [review]
Switched back to using toggleLockState

Thanks!

https://tbpl.mozilla.org/?tree=Try&rev=6d6356836c5a

Comment 35

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/9069fff5c78f

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/9069fff5c78f