Open Bug 1260225 Opened 8 years ago Updated 2 years ago

Move inspector-related command buttons (rulers, measurement) to the Inspector panel and enable them by default

Categories

(DevTools :: Inspector, defect, P2)

46 Branch
defect

Tracking

(Not tracked)

People

(Reporter: bgrins, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [btpp-fix-later][devtools-ux][good taipei bug])

Attachments

(2 files, 2 obsolete files)

Right now we have a lot of inspector related functionality hidden by default at the toolbox icon level.  This means lots of people don't find the features since it's hidden in the options panel.

Proposal: We should decouple these features from the gcli / command button implementation and move them into the inspector toolbar (in the space where the breadcrumbs used to be.

Then the buttons could be moved into the 'inspector' section of the options panel, but we can have a different set of them on by default.
Yes!
Priority: -- → P2
Whiteboard: [devtools-ux] → [btpp-fix-later][devtools-ux]
So I've gone through the list of the command buttons that we currently have. This is what I think we should do with them:

- Pick element: nothing, but the option to disable should be removed from settings. (Bug 1226913)
- iFrame: should move into a modal. (Bug 1238982)
- Split console: modal (Bug 1238982)
- RDM: modal (Bug 1238982)
- Highlight painted area: Should move into the inspector somewhere.
- Scratchpad: modal/browser console (Bug 1238982)
- Grab color: Should move into the inspector somewhere.
- Full page screenshot: modal (Bug 1238982)
- Measure portion of page: Should move into the inspector somewhere.
- Toggle rulers: Should move into the inspector somewhere.

For all of the, "Should move into the inspector somewhere" (there are four of them) I'm not sure _where_ they'll be just yet; I'll play with some designs so see if we can find a place that makes sense.
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?)) from comment #3)
> So I've gone through the list of the command buttons that we currently have.
> This is what I think we should do with them:
> 
> - Highlight painted area: Should move into the inspector somewhere.

Agreed, although I think it should still be off by default

> - Full page screenshot: modal (Bug 1238982)

This one feels 'inspector-y' to me

> For all of the, "Should move into the inspector somewhere" (there are four
> of them) I'm not sure _where_ they'll be just yet; I'll play with some
> designs so see if we can find a place that makes sense.

Thanks
(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?)) from comment #3)
> > - Highlight painted area: Should move into the inspector somewhere.
> 
> Agreed, although I think it should still be off by default
Definitely!

> > - Full page screenshot: modal (Bug 1238982)
> 
> This one feels 'inspector-y' to me
I'm fine with this.

An updated list:

- Pick element: nothing, but the option to disable should be removed from settings. (Bug 1226913)
- iFrame: should move into a modal. (Bug 1238982)
- Split console: modal (Bug 1238982)
- RDM: modal (Bug 1238982)
- Highlight painted area: Should move into the inspector somewhere (default=off).
- Scratchpad: modal/browser console (Bug 1238982)
- Grab color: Should move into the inspector somewhere.
- Full page screenshot: Should move into the inspector somewhere.
- Measure portion of page: Should move into the inspector somewhere.
- Toggle rulers: Should move into the inspector somewhere.
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?)) from comment #5)
> - Highlight painted area: Should move into the inspector somewhere
> (default=off).

As mentioned on IRC, this command belongs more inside the performance tool (which displays all paint markers) rather than the inspector.
Flagging as god taipei bug. I think bgrins would be a good mentor/reviewer for this, if anyone is interested. I can also help.
Whiteboard: [btpp-fix-later][devtools-ux] → [btpp-fix-later][devtools-ux][good taipei bug]
Patrick, how do you think we should do this technically?  The main difficulty with moving them is that the command buttons are coupled with gcli and toolbox startup (including building the buttons, setting 'active' state for long running commands, and managing button visibility based on options).  I think in most cases they shouldn't be relying on this stuff, but I don't know if you are envisioning that this decoupling would happen here or elsewhere.
Flags: needinfo?(pbrosset)
(In reply to Brian Grinstead [:bgrins] from comment #8)
> Patrick, how do you think we should do this technically?  The main
> difficulty with moving them is that the command buttons are coupled with
> gcli and toolbox startup (including building the buttons, setting 'active'
> state for long running commands, and managing button visibility based on
> options).  I think in most cases they shouldn't be relying on this stuff,
> but I don't know if you are envisioning that this decoupling would happen
> here or elsewhere.

- Highlight painted area
I tend to agree with ntim that this is more perf-related than inspector-related. In particular, the areas that are painted aren't necessarily DOM nodes, and often aren't.
I think this should be a function of the perf actors, and the button should be over in that panel instead.

- Grab color
As discussed with Helen last week, we may in fact get rid of this one, because we already have an eye dropper in the color picker tooltip. This global eye dropper doesn't seem too useful for devtools. When you're changing a color, it's most of the time in the CSS rule-view, and this is precisely where we have the color picker tooltip. So I'd vote for keeping this around as a toggle-able gcli toolbox command thing, or remove it.

- Measure portion of page
- Toggle rulers
These 2 ones are definitely designer-oriented, and so should live in the inspector's toolbar. These are actually highlighters, so we don't really need GCLI commands to use them. I think we should get rid of the command files for those, and re-implement the get actor/show/hide logic in the inspector instead.

- Full page screenshot
This one is kind of a mess, we have this button, we also have the inspector context menu to screenshot a node, we also have a screenshot button in RDM. I think it's time we take the code out of the GCLI command it is now, and make it a shared module that we can reuse more easily in several places in the UI.
The last version of this was the one done in the new RDM project, so we should look there first, see what we can reuse. But I vote for not using the GCLI command anymore in the toolbar (the command itself could stay, for people used to GCLI).
Flags: needinfo?(pbrosset)
(In reply to Patrick Brosset <:pbro> from comment #9)
> - Measure portion of page
> - Toggle rulers
> These 2 ones are definitely designer-oriented, and so should live in the
> inspector's toolbar. These are actually highlighters, so we don't really
> need GCLI commands to use them. I think we should get rid of the command
> files for those, and re-implement the get actor/show/hide logic in the
> inspector instead.
For these two I think they might merit more thought—I'm worried the Inspector toolbar will become like our devtools toolbar with buttons all over the place. Ultimately, we might not be solving a problem.

> - Full page screenshot
> This one is kind of a mess, we have this button, we also have the inspector
> context menu to screenshot a node, we also have a screenshot button in RDM.
> I think it's time we take the code out of the GCLI command it is now, and
> make it a shared module that we can reuse more easily in several places in
> the UI.
> The last version of this was the one done in the new RDM project, so we
> should look there first, see what we can reuse. But I vote for not using the
> GCLI command anymore in the toolbar (the command itself could stay, for
> people used to GCLI).
Screenshotting a page might not even belong in the devtools UI but even at the browser level; not that I know where it should live, exactly, but that's been brought up a few times—lots of users, not just devtoolers, want to be able to take screenshots of the page. Maybe it could start in the right-hand context menu...
> Screenshotting a page might not even belong in the devtools UI but even at the browser level

Absolutely! I just showed a non-dev friend how to take full-page screenshots with the dev tools because she just needs this. I would propose (in a new ticket) to surface the screenshot tool as:
* File > Take Screenshot (e.g. below 'Save page as')
* As an additional Tool 'Screenshot page' for the toolbar like there is 'Share this Page', 'Hello', 'Email link' etc. Hidden by default
* As Helen said as a Context menu 'Screenshot Element' (below 'Inspect Element' maybe)

OTOH, this could also be just an addon (of which there are already quite a few, I like none of them).
(In reply to Jens from comment #11)
> > Screenshotting a page might not even belong in the devtools UI but even at the browser level
> 
> Absolutely! I just showed a non-dev friend how to take full-page screenshots
> with the dev tools because she just needs this. I would propose (in a new
> ticket) to surface the screenshot tool as:
> * File > Take Screenshot (e.g. below 'Save page as')
> OTOH, this could also be just an addon (of which there are already quite a
> few, I like none of them).

I like the idea of "Screenshot page" appearing below "Save page as", both in the File menu and the right-click context menu (not the devtools context menu, the page context menu). The UX team has mentioned screenshotting as something

I think this could definitely blossom into a bunch of cool functionality (like what we currently have in the gcli), but I think anything more complicated than that one single option probably belongs in some sort of addon. I'm not sure if that's something where we'd want to reach out to an existing addon about extending functionality?
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #14)
> I like the idea of "Screenshot page" appearing below "Save page as", both in
> the File menu and the right-click context menu (not the devtools context
> menu, the page context menu). The UX team has mentioned screenshotting as
> something
Hm, somehow the rest of this got cut off. The UX team has mentioned screenshotting as something that might be useful for an audience larger than the developer tools audience, so I'd love to see it in the righthand context menu.

> I think this could definitely blossom into a bunch of cool functionality
> (like what we currently have in the gcli), but I think anything more
> complicated than that one single option probably belongs in some sort of
> addon. I'm not sure if that's something where we'd want to reach out to an
> existing addon about extending functionality?
There's a Test Pilot experience coming out called "Page Shot" where we might surface a lot of our awesome-er gcli functionality. I'm finding out more about this this afternoon.

All right, back to the "Measure a Portion of the Page" and "Show Rulers": it seems like we don't track much telemetry data for their usage, so I actually think the first steps are to gather some qualitative and quantitative data. I'm trying to gather qualitative information here: https://twitter.com/helenvholmes/status/735492719561330692 I think an engineering next-step would be to set up Telemetry probes for how many people turn them on and how often they're used, although even that won't give us the full picture. (We've discussed discoverability issues with these tools before.)

Without any solid information yet, I think we might move "Show Rulers" as an option into the Settings, and "Measure a Portion of the Page" might evolve into something like Bug 1263768 if it's not proving useful in its current implementation.

Patrick, can you give some guidance about what this means for the people in this bug?
Flags: needinfo?(pbrosset)
Hi Patrick, I would like to try this bug, is this still under discussion or ready to work on?
Flags: needinfo?(pbrosset)
Hi Joseph, thanks for wanting to work on this.
I think the 2 buttons that make the most sense are the ruler and measurer tools. Helen did mention in comment 10 that this needed a bit more thoughts on how/where to display them, but they still are the most actionable at this stage. So I would tend to suggest you to start working on them, at least at the technical level, there's some work to be done to actually move them outside of where they are right now.
For instance, they are gcli command buttons for now, while I don't think they should be anymore. They are just "highlighters" [1] that can be created using |toolbox.highlighterUtils.getHighlighterByType|.
So I'd work on refactoring that bit first, assuming that they would go in the inspector toolbar, and then sync back with Helen to decide how to progress on the UX side of things.

[1] https://wiki.mozilla.org/DevTools/Highlighter
Flags: needinfo?(pbrosset)
Assignee: nobody → jyeh
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #15)
> All right, back to the "Measure a Portion of the Page" and "Show Rulers": it
> seems like we don't track much telemetry data for their usage, so I actually
> think the first steps are to gather some qualitative and quantitative data.
> I'm trying to gather qualitative information here:
> https://twitter.com/helenvholmes/status/735492719561330692 I think an
> engineering next-step would be to set up Telemetry probes for how many
> people turn them on and how often they're used, although even that won't
> give us the full picture. (We've discussed discoverability issues with these
> tools before.)
That's right, telemetry data here would tell us how many people found the setting and then used the rulers. But we don't know if people would use the tool more if it was more obvious to enable.
Having said that, it's doesn't hurt to add a telemetry probe for the rulers and measurement tool.
@Joseph: I've filed bug 1275886 for this if you're interested in picking up that one too.

> Without any solid information yet, I think we might move "Show Rulers" as an
> option into the Settings,
So, instead of having a box to enable/disable the toolbar button in the options panel, you would add a box to show/hide the rulers, right? But still in the options panel.
@Helen: Would this be a good opportunity to add a settings icon (cog icon) to the inspector toolbar? Then we could have the show/hide rulers in this menu, as well as the other inspector settings that exist today in the options panel.

In any case, what I said in comment 13 still applies, we need to move out of the gcli command button mechanism and make something more generic that we can enable/disable easily from anywhere.
So Joseph can still work on that.

> and "Measure a Portion of the Page" might evolve
> into something like Bug 1263768 if it's not proving useful in its current
> implementation.

Ok, so it sounds like there are a few things to do:

- work on bug 1275886 to start gathering data,
- work on making the rulers and measurement tools easier to use without being tied to gcli or toolbar buttons,
- maybe adding a settings menu to the inspector to start moving things there. Or, depending on what Helen things, move the rulers toggle as a checkbox in the options panel instead.
Flags: needinfo?(pbrosset) → needinfo?(hholmes)
(In reply to Patrick Brosset <:pbro> from comment #16)
> > Without any solid information yet, I think we might move "Show Rulers" as an
> > option into the Settings,
> So, instead of having a box to enable/disable the toolbar button in the
> options panel, you would add a box to show/hide the rulers, right? But still
> in the options panel.
> @Helen: Would this be a good opportunity to add a settings icon (cog icon)
> to the inspector toolbar? Then we could have the show/hide rulers in this
> menu, as well as the other inspector settings that exist today in the
> options panel.
I think that we might use (instead of a settings icon) the ellipsis that's become sort of popular: http://cl.ly/061L2M3b210q http://cl.ly/3E2q3E2T3s0t

For the Inspector (since there won't be a lot in it other than these buttons for the moment), we could implement something like Bug 1238982 for it (which pops open a toolbar) or we could deeplink into the Settings page into a section specifically for the Inspector. (This is what James and I had discussed for the Debugger, assuming that the natural progression of our Settings page will be to split it up into sections.)

Since enhancements/work to Settings seems not to be on anyone's plates at the moment, the first might be the best approach for now as it feels more self-contained, but I'm not actually sure since I'm not familiar with any of that code.

> - work on bug 1275886 to start gathering data,
> - work on making the rulers and measurement tools easier to use without
> being tied to gcli or toolbar buttons,
Hm, I do have a question here—is part of this bug to change the functionality of these buttons? I didn't get that from the bug title alone. I assumed these buttons would do what they currently do, just not sit along the toolbar?

> - maybe adding a settings menu to the inspector to start moving things
> there. Or, depending on what Helen things, move the rulers toggle as a
> checkbox in the options panel instead.
I've laid out my ideas for this, but I'm going to let Patrick decide what he thinks is the best way to move forward—I'm fine with both implementations, though.
Flags: needinfo?(hholmes) → needinfo?(pbrosset)
(In reply to Patrick Brosset <:pbro> from comment #16)
> (In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #15)
> > Without any solid information yet, I think we might move "Show Rulers" as an
> > option into the Settings,
> So, instead of having a box to enable/disable the toolbar button in the
> options panel, you would add a box to show/hide the rulers, right? But still
> in the options panel.
> @Helen: Would this be a good opportunity to add a settings icon (cog icon)
> to the inspector toolbar? Then we could have the show/hide rulers in this
> menu, as well as the other inspector settings that exist today in the
> options panel.

My 2 cents: I wouldn't tie up this work, which is already concrete, with global settings changes.  We could move the rulers button and if we want to still pref it off by default, move the checkbox over into the Inspector section in the settings and keep the default setting to off.  That's an improvement already by getting things out of the top level button area and into the inspector.
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #17)
> Hm, I do have a question here—is part of this bug to change the
> functionality of these buttons? I didn't get that from the bug title alone.
> I assumed these buttons would do what they currently do, just not sit along
> the toolbar?
No functionality change. I meant making the rulers and measurement code easier to execute from a context that is not a toolbar button (toolbar buttons right now require a gcli command file, with specific attributes).
For what it's worth, we could even keep the gcli command if they're useful to some people but greatly simplify the code in them to just call the new rulers/measurements modules.

(In reply to Brian Grinstead [:bgrins] from comment #18)
> My 2 cents: I wouldn't tie up this work, which is already concrete, with
> global settings changes.  We could move the rulers button and if we want to
> still pref it off by default, move the checkbox over into the Inspector
> section in the settings and keep the default setting to off.  That's an
> improvement already by getting things out of the top level button area and
> into the inspector.
Sounds good to me. I can see that being a good step for this bug.
Flags: needinfo?(pbrosset)
Hi Brian,

I just made a patch to see if I am working on the right way.

This patch will add a rulers button into the inspector toolbar (between inspector-searchbox and inspector-pane-toggle). Clicking the button will show/hide the RulersHighlighter.

I know there are still a lot of work to do, just like to know if my patch did it right.

BTW, I would also like to know about the "destroy" event from the highlighter. I tried to listen to the event but it never succeed. Can you give me some hint about this?

Thanks!
Attachment #8758149 - Flags: feedback?(bgrinstead)
Comment on attachment 8758149 [details] [diff] [review]
0001-Bug-1260225-Move-inspector-related-command-buttons-e.patch

Review of attachment 8758149 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Joseph, thanks for the patch! This is looking like a really good start - I checked it out locally and it's working.  I'm going to forward the request to Gabe, since he's a better reviewer going forward for this work.

::: devtools/client/themes/variables.css
@@ +61,4 @@
>    --theme-graphs-full-blue: #00f;
>  
>    /* Images */
> +  --theme-rulers-image: url(chrome://devtools/skin/images/command-rulers.svg);

This should be moved into the :root declaration below so it can be shared between the light and dark themes
Attachment #8758149 - Flags: feedback?(bgrinstead) → feedback?(gl)
Hi Gabriel,

It's been a while from the previous patch, so I update it with my current progress.

This patch add the rulers and measuring tool button to the inspector toolbar. I am not sure whether it is correct to call `finalize` when we need to remove the highlighter.

Also, I still cannot find the way to listen to the `destroy` event. I hope you can give me some feedback from this patch :)

Thanks!
Attachment #8758149 - Attachment is obsolete: true
Attachment #8758149 - Flags: feedback?(gl)
Attachment #8760124 - Flags: feedback?(gl)
(In reply to Joseph Yeh [:joseph] from comment #22)
> This patch add the rulers and measuring tool button to the inspector
> toolbar. 

What about the eyedropper? This bug started with 'eyedropper, rulers, screenshot, measurement'. Screenshot is yet to be decided afaiu (although moving it to inspector would be better than leaving it where it is now due to not coming up with a decision), but seems like the eyedropper got lost unintentionally in the discussion. (disclaimer: I am not part of the Fx devtools team)
(In reply to Jens from comment #23)
> What about the eyedropper? This bug started with 'eyedropper, rulers,
> screenshot, measurement'. Screenshot is yet to be decided afaiu (although
> moving it to inspector would be better than leaving it where it is now due
> to not coming up with a decision), but seems like the eyedropper got lost
> unintentionally in the discussion.
Let's leave the eyedropper to bug 1262439. I'm working on it now and will move the eyedropper icon to the inspector toolbar in that bug. So the bug here can concentrate on the rulers and measurement tool only.
Comment on attachment 8760124 [details] [diff] [review]
0001-Bug-1260225-Move-inspector-related-command-buttons-e.patch

Review of attachment 8760124 [details] [diff] [review]:
-----------------------------------------------------------------

I am not quite sure what the UI should actually look like. So, I would probably do a ui-review or ask Helen to confirm where she wanted the buttons to be placed. Also, the patch comment should only indicate rulers and measurement buttons for the meantime.

::: devtools/client/inspector/inspector-panel.js
@@ +95,5 @@
>    this.onNewSelection = this.onNewSelection.bind(this);
>    this.onBeforeNewSelection = this.onBeforeNewSelection.bind(this);
>    this.onDetached = this.onDetached.bind(this);
> +  this.onRulersToggleButtonClicked = this.onRulersToggleButtonClicked.bind(this);
> +  this.onMeasureToggleButtonClicked = this.onMeasureToggleButtonClicked.bind(this);

Swap the order of these 2 so it will be in alphabetical order

@@ +376,5 @@
> +    this._rulersToggleButton.addEventListener("mousedown",
> +      this.onRulersToggleButtonClicked);
> +  },
> +
> +  onRulersToggleButtonClicked: function () {

Add a comment for the function

@@ +406,5 @@
> +    this._measureToggleButton.addEventListener("mousedown",
> +      this.onMeasureToggleButtonClicked);
> +  },
> +
> +  onMeasureToggleButtonClicked: function () {

Add a comment for the function.

::: devtools/client/locales/en-US/inspector.properties
@@ +69,5 @@
>  # could not be loaded (for example, because of a connectivity problem).
>  docsTooltip.loadDocsError=Could not load docs page.
>  
> +# LOCALIZATION NOTE (inspector.rulers) A string displayed as the
> +# tooltip of button in devtools toolbox which toggles the rulers.

It should be in the inspector toolbox instead of devtools toolbox.
Attachment #8760124 - Flags: feedback?(gl) → feedback+
Comment on attachment 8764841 [details] [diff] [review]
0001-Bug-1260225-Move-inspector-related-command-buttons-e.patch

Hi Gabriel,

In this patch, I tried to listen to the `destroy` event from [1], but somehow I cannot achieve it.

In the file `/devtools/client/inspector/inspector-panel.js` from line 418 to 425 and line 461 to 468, I would like to do something same as [2] did, but the callback function never got triggered.

In my observation, it looks like the highlighter target in [1] is differ from the highlighter we get from `getHighlighterByType`.

Is there any way we can bridge these two ends so the event can be triggered normally, or is there other formal way to achieve this?


Thanks!


[1] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/highlighters/rulers.js#275

[2] https://dxr.mozilla.org/mozilla-central/source/devtools/shared/gcli/commands/rulers.js#96-104
Flags: needinfo?(gl)
Attached image Inspector Subtools Menu.png (deleted) —
I haven't been ui-r'd on the patch yet, but here are my thoughts on what the UI for this should look like.

I'm imagining that we'll have a submenu in the Inspector toolbar area where users can toggle the icons on and off. The submenu interacts the same as the Rules area submenu: http://cl.ly/2N2B0d280j0g, http://cl.ly/2d223f2t213b
make title reflect the actual work.
Summary: Move inspector-related command buttons (eyedropper, rulers, screenshot, measurement) to the Inspector panel and enable them by default → Move inspector-related command buttons (rulers, measurement) to the Inspector panel and enable them by default
Comment on attachment 8764841 [details] [diff] [review]
0001-Bug-1260225-Move-inspector-related-command-buttons-e.patch

Review of attachment 8764841 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry Joseph, this needinfo has been neglected on my part since I am not too familiar in this area.

::: devtools/client/inspector/inspector-panel.js
@@ +457,5 @@
> +    this.toolbox.highlighterUtils.getHighlighterByType("MeasuringToolHighlighter")
> +      .then(measuringToolHighligher => {
> +        this._measuringToolHighlighter = measuringToolHighligher;
> +
> +        events.once(this._measuringToolHighlighter, "destroy", () => {

I don't think we need to listen to the "destroy" event here. (I could be wrong)

We should also be removing the highlighters in the destroy method when the inspector panel is destroyed if the highlighters exist.
Flags: needinfo?(gl)
As I am currently not working on this bug, I'll leave this bug unassigned so others interested may follow up.
Assignee: jyeh → nobody
No longer blocks: top-inspector-bugs
Patrick, do we still want move rulers, measurement to the Inspector panel? I saw eyedroper is removed from the Inspector panel in nightly.
Flags: needinfo?(pbrosset)
(In reply to Fred Lin [:gasolin] from comment #32)
> Patrick, do we still want move rulers, measurement to the Inspector panel?
Yes I think we still want this. I've read the bug again, and I think we agreed on moving these 2 icons to the inspector toolbar, and move the corresponding toggle checkboxes from the "Toolbox buttons" section to the "Inspector" section in the settings panel.
> I saw eyedroper is removed from the Inspector panel in nightly.
The eyedropper icon is now in the inspector toolbar (since bug 1262439).
Flags: needinfo?(pbrosset)
Product: Firefox → DevTools
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: