1262773 - Measuring tool not turned off when toolbox button is hidden

Status: ASSIGNED

(DevTools :: Inspector, defect, P2)

Description

[Sebastian Zartner [:sebo]]

When the toolbox options checkbox that toggles the measuring tool gets unchecked while the tool is active, the tool stays activated. This means the measuring overlay is still there even when the tool is deactivated.

The tool should instead get deactivated once the checkbox gets unchecked.

Sebastian

PS: The same is true for other tools like the rulers toggle or the tool for highlighting the painted area. Let me know if I should file separate bugs for these.

Comment 1

[Patrick Brosset <:pbro>]

Matteo, could you please investigate a bit here in order to prioritize this bug?

Comment 2

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Sebastian Zartner [:sebo] from comment #0)

> When the toolbox options checkbox that toggles the measuring tool gets
> unchecked while the tool is active, the tool stays activated. This means the
> measuring overlay is still there even when the tool is deactivated.
> 
> The tool should instead get deactivated once the checkbox gets unchecked.

Sebastian, could you provide some STR? I wasn't able to reproduce it simply clicking on the icons / executing commands from GCLI, and a mix of them (enable from button, disable from GCLI, and so on).
This, on the latest Nightly, 48.0a1 (2016-04-11), and on OS X 10.11.4.

> PS: The same is true for other tools like the rulers toggle or the tool for
> highlighting the painted area.

That's very possible, they all share the same mechanism.

> Let me know if I should file separate bugs for these.

In case we can use this bug for all of them, maybe we'll just rephrase the subject.

Comment 3

[Sebastian Zartner [:sebo]]

Sorry for not being clearer before. Here are the steps:
1. Open the DevTools
2. Switch to the Options panel
3. Check the option 'Measure a portion of the page'
4. Click the button of the Measure tool to enable it
5. Measure something on the page
6. Uncheck the option 'Measure a portion of the page' within the Options panel

=> The selected area is still visible and you can still create new measurements while the toolbox button is not available anymore.

Hope this makes it clearer now.

>> PS: The same is true for other tools like the rulers toggle or the tool for
>> highlighting the painted area.
>
> That's very possible, they all share the same mechanism.

That actually applies to all tools listed under 'Available Toolbox Buttons'.
My assumption was (and I guess I'm not the only one) that there is no way to disable the tool when the toolbox button is hidden. I didn't think of the GCLI to toggle it.

Though the main point of this issue is, when somebody removes the button from the toolbox, (s)he likely doesn't want to use the tool anymore and therefore it should be turned off.

Sebastian

Comment 4

[Patrick Brosset <:pbro>]

Marking as P2. Matteo, feel free to change it.
Also could you please check if this could be a good mentored first bug? I have a feeling it could.

Comment 5

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Sebastian Zartner [:sebo] from comment #3)
> Sorry for not being clearer before. Here are the steps:
> 1. Open the DevTools
> 2. Switch to the Options panel
> 3. Check the option 'Measure a portion of the page'
> 4. Click the button of the Measure tool to enable it
> 5. Measure something on the page
> 6. Uncheck the option 'Measure a portion of the page' within the Options
> panel
> 
> => The selected area is still visible and you can still create new
> measurements while the toolbox button is not available anymore.
> 
> Hope this makes it clearer now.

Okay, now I understand! I would say it's intended. Basically they're shortcut buttons for the GCLI command. The presence in the toolbox doesn't affect the functionality of the command itself; in fact if you execute the command from the GCLI and then enable the relative button, the button should get the current state of the tool – without "resetting" that state.

Said that, I also think it's bad UI, because if the user only uses the tool from the toolbox without knowing the GCLI, removing that from the toolbox makes virtually impossible for him to exit from the tool.

We should probably either reset the state of the tool every time the relative button is added / removed; or just when is removed.

But I don't think we have currently such functionality in GCLI, to specify in a command something to do when the related button is removed. Joe, do we?

> That actually applies to all tools listed under 'Available Toolbox Buttons'.
> My assumption was (and I guess I'm not the only one) that there is no way to
> disable the tool when the toolbox button is hidden. I didn't think of the
> GCLI to toggle it.

> Though the main point of this issue is, when somebody removes the button
> from the toolbox, (s)he likely doesn't want to use the tool anymore and
> therefore it should be turned off.

Yes, that's a valid point.

Comment 6

[Joe Walker [:jwalker]]

So I agree that it would be nice to turn a tool off when we remove the button, but I think this is a P3, right?

Comment 7

[Sebastian Zartner [:sebo]]

Attachment: Bug 1262773 - Turned off highlighters and paint flashing when toolbar button is removed. r=jdescottes

To improve the UX when removing toolbar buttons, the highlighters and paint flashing are turned off.
This avoids confusion for how to turn them off without having the buttons available.

For now, all highlighters and paint flashing are turned off at once when one of the related toolbar buttons is removed.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last months and this bug has priority 'P2'.
:jdescottes, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 9

[Julian Descottes [:jdescottes]]

I believe sebo is still active?

Sebastian do you want to keep the bug?

Comment 10

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.

Comment 11

[Sebastian Zartner [:sebo]]

Just to confirm, yes, I am still active, though I am currently working on custom formatters. Once that's done, I'm free to pick this up again. Though if anyone wants to jump in in the meantime, feel free to finish the patch.

Sebastian

Comment 12

[Sebastian Zartner [:sebo]]

Attachment: Bug 1262773 - [devtools] Generalized logic for destroying highlighters associated with toolbar buttons on page navigation. r=jdescottes

Instead of checking for specific highlighters being registered and destroying all of them if one is registered, the highlighters are now exposed together with the button on creation.
This allows to loop over the associated highlighters and explicitly destroy those.

Comment 13

[Sebastian Zartner [:sebo]]

Attachment: Bug 1262773 - [devtools] Destroyed all associated highlighters when a button is removed from the toolbar. r=jdescottes

Depends on D187061