1353933 - Let's add a note to about:debugging about web-ext

Status: RESOLVED FIXED

(WebExtensions :: Developer Tools, enhancement, P5)

Description

[Andy McKay]

How about a line tooltip at the top of about:debugging pointing the developer towards web-ext. Its pretty cool but seems not a lot of people know about it. We can link to the MDN docs, something like: "Try out web-ext for a command line tool that provides auto-reloading and other features".

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Getting_started_with_web-ext

I'm terrible at text though.

Comment 1

[Andy McKay]

Attachment: Screenshot 2017-04-13 16.18.21.png

Comment 2

[Andy McKay]

Attachment: web-ext.patch

Comment 3

[Andy McKay]

Attachment: Screenshot-2017-4-17 Debugging with Firefox Developer Tools.png

This still looks awkward to me, so how about adding the message to the "Nothing yet" message. That way if you've loaded an add-on using web-ext, it won't show up. If you haven't, it will.

Comment 4

[Andy McKay]

Attachment: bug 1353933 add a note about web-ext

Review commit: https://reviewboard.mozilla.org/r/132562/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132562/

Comment 5

[Andy McKay]

Attachment: Screenshot 2017-04-21 14.26.46.png

Mark and I figured something like this would work.

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8860539 [details]
bug 1353933 add a note about web-ext

https://reviewboard.mozilla.org/r/132562/#review135730

Thanks! 

Just a few comments on the changeset, and I would also like to discuss the positioning and display of the message.

When there are no temporary extensions loaded, this reads like "Nothing yet. Use web-ext ...". I think it would be nice to highlight this message and separate it visually from the "Nothing yet" text. Maybe we could use a background similar to the one we have for the multicontent process warning message in the workers category?
(.service-worker-multi-process in aboutdebugging.css)

In the long run I think this message should be close to the "Load temporary Add-on" button, because it's an alternative way for loading temporary extensions.
But that's not easy to do with the current layout. Now that we display temporary extensions and extensions in two separate lists maybe we can do a a few UI changes:
- move the "load temporary addon" in the "temporary extensions" section (and if we do that, we could position this new promotion text close to it)
- remove the "reload" button from the regular extensions
(not to be done in this bug of course, but might be a good follow up)

::: devtools/client/aboutdebugging/aboutdebugging.css:200
(Diff revision 1)
>  
>  .addons-options {
>    flex: 1;
>  }
>  
> -.addons-debugging-label {
> +.addons-debugging-label, .addons-web-ext-span {

move the second selector on a dedicated line (better for diffs)

.addons-debugging-label,
.addons-web-ext-span {

::: devtools/client/aboutdebugging/components/addons/panel.js:150
(Diff revision 1)
>          client,
>          debugDisabled,
>          targetClass,
>          sort: true
>        })),
> +    dom.div({ className: "addons-options" },

why "addons-options" ?

Shouldn't this be inside the "temporary-addons" container ?

::: devtools/client/aboutdebugging/components/addons/panel.js:152
(Diff revision 1)
>          targetClass,
>          sort: true
>        })),
> +    dom.div({ className: "addons-options" },
> +      dom.span({
> +        className: "addons-web-ext-span",

can we avoid "span" in the classname ? "addons-web-ext-tip" ?

::: devtools/client/locales/en-US/aboutdebugging.properties:70
(Diff revision 1)
>  # This string is displayed as a header above the list of temporarily loaded add-ons.
>  temporaryExtensions = Temporary Extensions
>  
> +# LOCALIZATION NOTE (temporaryExtensions):
> +# This string is displayed as a message below list of temporarily loaded add-ons.
> +webExtTip = Use web-ext to load temporary extensions from the command line.

Regarding the wording, I think we should be careful about the extensions/addon confusion. web-ext only allows to load WebExtension I assume? The addons page for about:debugging uses extensions to cover both addons and WebExtension, and we should be careful about the wording until 57 is released. Maybe use "WebExtension" instead of "extension"

Also the current text makes me think web-ext is something I should know about already. "Use web-ext to" -> "Use the web-ext tool to" or "You can install the web-ext tool to" (that's just my feeling however, wording is not really my cup of tea either :) )

::: devtools/client/locales/en-US/aboutdebugging.properties:72
(Diff revision 1)
>  
> +# LOCALIZATION NOTE (temporaryExtensions):
> +# This string is displayed as a message below list of temporarily loaded add-ons.
> +webExtTip = Use web-ext to load temporary extensions from the command line.
> +
> +

nit: remove extra blank line

Comment 7

[Mark Striemer [:mstriemer]]

When Andy and I were discussing the layout one thing I suggested was putting both buttons into a block like the extensions. I'm not sure this is exactly what it should look like since it might be a little too similar to the extensions but it fits the updated visual feel a little better.

I mocked this up quickly [1]. The options should probably be "From file..." and "From command line", the latter just opening the MDN article that this patch links to.

I can file an issue to get some feedback from Emanuela on how this should look if we're okay with the setup in this patch for now.

[1] https://www.dropbox.com/s/h49nt796gfj3i1i/Screenshot%202017-04-19%2016.21.37.png?dl=0

Comment 8

[Julian Descottes [:jdescottes]]

(In reply to Mark Striemer [:mstriemer] from comment #7)
> When Andy and I were discussing the layout one thing I suggested was putting
> both buttons into a block like the extensions. I'm not sure this is exactly
> what it should look like since it might be a little too similar to the
> extensions but it fits the updated visual feel a little better.
> 
> I mocked this up quickly [1]. The options should probably be "From file..."
> and "From command line", the latter just opening the MDN article that this
> patch links to.
> 
> I can file an issue to get some feedback from Emanuela on how this should
> look if we're okay with the setup in this patch for now.
> 
> [1]
> https://www.dropbox.com/s/h49nt796gfj3i1i/Screenshot%202017-04-19%2016.21.37.
> png?dl=0

As you mentioned, I don't think the actions should look like an extension block. Definitely worth filing a separate bug and get UI/UX feedback on this topic.

For the purpose of this bug, let's keep the add button as it is. We simply should make sure the web-ext tip stands out, as I mentioned in my review.

Comment 9

[Andy McKay]

Comment on attachment 8860539 [details]
bug 1353933 add a note about web-ext

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132562/diff/1-2/

Comment 10

[Andy McKay]

Thanks for the feedback, I think I've done the fixes based on the feedback. Based on the comments I've left the text in the same place following some more UX input on this and spawning a new bug.

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8860539 [details]
bug 1353933 add a note about web-ext

https://reviewboard.mozilla.org/r/132562/#review138054

Thanks! Few nits + missing some styling to make the tip stand out more. 

Feel free to land with the comments addressed!

::: devtools/client/aboutdebugging/components/addons/panel.js:150
(Diff revision 2)
>          client,
>          debugDisabled,
>          targetClass,
>          sort: true
> -      })),
> +      }),
> +      dom.span({

It would be nice if the tip could stand out from the rest (particularly from the "Nothing yet" text).

Can you wrap the span + a in a container and give it the same style as .service-worker-multi-process (http://searchfox.org/mozilla-central/source/devtools/client/aboutdebugging/aboutdebugging.css#154-159,165-168)

::: devtools/client/locales/en-US/aboutdebugging.properties:68
(Diff revision 2)
>  
>  # LOCALIZATION NOTE (temporaryExtensions):
>  # This string is displayed as a header above the list of temporarily loaded add-ons.
>  temporaryExtensions = Temporary Extensions
>  
> +# LOCALIZATION NOTE (temporaryExtensions):

# LOCALIZATION NOTE (temporaryExtensions): -> # LOCALIZATION NOTE (webExtTip):

::: devtools/client/locales/en-US/aboutdebugging.properties:69
(Diff revision 2)
>  # LOCALIZATION NOTE (temporaryExtensions):
>  # This string is displayed as a header above the list of temporarily loaded add-ons.
>  temporaryExtensions = Temporary Extensions
>  
> +# LOCALIZATION NOTE (temporaryExtensions):
> +# This string is displayed as a message below list of temporarily loaded add-ons.

s/below list/below the list

Comment 12

[Andy McKay]

Comment on attachment 8860539 [details]
bug 1353933 add a note about web-ext

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132562/diff/2-3/

Comment 13

[Andy McKay]

Attachment: Screenshot 2017-05-05 17.01.29.png

Comment 14

[Andy McKay]

Just pushed up a new change to just wrap the line in a div and give it a help icon (see attached image). I think that's nicely separated now. That felt a bit more than adding a class since you gave me an r?, so wanted to check if I'm cool to ship with that change.

Comment 15

[Julian Descottes [:jdescottes]]

LGTM, thanks for doing this!

Comment 16

[Andy McKay]

Comment on attachment 8860539 [details]
bug 1353933 add a note about web-ext

https://reviewboard.mozilla.org/r/132562/#review139784

::: devtools/client/aboutdebugging/components/addons/panel.js:150
(Diff revision 2)
>          client,
>          debugDisabled,
>          targetClass,
>          sort: true
> -      })),
> +      }),
> +      dom.span({

I found that span rather yellow for my tastes, I think its cool for warnings, but to be there permanently was a bit much.

Instead I went and grabbed the "help" icon out of devtools. It lines up nicely with the checkbox from enable add-on debugging and is standsout quite a bit I think, without being too garish.

Also lines up nicely for some other tips if we wanted to do those e.g.: links to API docs on MDN etc.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3261993daa1
add a note about web-ext r=jdescottes

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d3261993daa1