1210123 - Add devices refresh button, always show tabs button

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I think we need a refresh button for the devices as well (like with tabs).  I've seen a few cases with sidebars where a device only appeared by closing and reopening WebIDE.

Related to this topic, we may want to always show the "tabs" header and refresh button.  Currently, it only appears if there is at least one tab, but this means you can't refresh to notice the change 0 -> 1 tab.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Here are some usage patterns to fix here:

1. You have a device with Firefox for Android, but it's not running currently
2. Connect device to WebIDE, nothing appears in device list
3. Start Firefox for Android

With sidebars, you'll never see anything appear in the device list, because we need to re-scan to find it first.  We only get notifications of devices connecting, not programs on those devices, so don't have a way to know it's running.

Similarly with WiFi

1. WiFi debug is disabled or your device is off
2. Open WebIDE, nothing appears.
3. Enable WiFi debug on device and / or turn on device

Another case where we need to re-scan to find it.

It would nice to get this in 43 too, though there will probably be conflicts since we only removed sidebars in 44.

Jen, could you take a look at this?

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

In our team meeting, Alex mentioned we could also consider polling every so often to avoid requiring the refresh buttons.

Comment 3

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1210123.patch

Tested and it appears to work as expected. Let me know if you want anything reworded or if I should add anything else.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8681435 [details] [diff] [review]
Bug1210123.patch

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

Overall it functions well!  Just a few small tweaks to make.  Thanks for working on this!

::: devtools/client/webide/content/project-listing.xhtml
@@ -24,5 @@
>          <label class="panel-header">&projectPanel_myProjects;</label>
>          <div id="project-panel-projects"></div>
>          <label class="panel-header" id="panel-header-runtimeapps" hidden="true">&projectPanel_runtimeApps;</label>
>          <div id="project-panel-runtimeapps"/>
> -        <label class="panel-header" id="panel-header-tabs" hidden="true">&projectPanel_tabs;

I think we want to keep it hidden on startup, like with runtime apps.

::: devtools/client/webide/modules/project-list.js
@@ -150,5 @@
>      while (tabsNode.hasChildNodes()) {
>        tabsNode.firstChild.remove();
>      }
>  
> -    if (!AppManager.connected) {

I think we should keep this block: if we are not connected, we definitely do not have tabs.

@@ -159,3 @@
>      let tabs = AppManager.tabStore.tabs;
>  
> -    if (tabs.length > 0) {

This block would then change to always removing the attribute (since at this point we know we must be connected if we are still in this function).

The main thing we'll have accomplished is that the `tabs.length > 0` check will be gone, allowing you to use the button during the transition from 0 -> 1 tabs since it will always be present after connection.

::: devtools/client/webide/themes/panel-listing.css
@@ +91,5 @@
>    cursor: default;
>  }
>  
> +#refresh-tabs,
> +#refresh-devices {

Would it make sense to grab a refresh icon from Firefox or somewhere for these buttons, instead of using button text?

For the "refresh devices" button, there is not enough width to show it on the same line as the "usb devices" header text, so it looks a bit odd for the moment.

Comment 5

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Firefox_WebIDE__Main_Process.png

Hey Helen, do you have a good suggestion for a refresh icon that would fit in the area of where that button and text is?

Comment 6

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: reload-icons.zip

Seems like we use these icons elsewhere. They don't have a fill set, so they'd need either an inline fill or a CSS filter for them to look right (I think they'd look best/most consistent if you matched the gray of the System and Smart collections icons).

Jenn, think these will work?

Comment 7

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Updated with reload icon

Comment 8

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1210123.patch

Updated the patch with your suggested comments. Let me know if that works.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=119fb59ad9b3

Comment 9

[Jen Fong-Adwent [:ednapiranha]]

Comment on attachment 8685173 [details] [diff] [review]
Bug1210123.patch

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

Fixing tests, sending new patch

Comment 10

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1210123.patch

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1da54fa9c1c

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8685612 [details] [diff] [review]
Bug1210123.patch

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

What happened to the project-list.js changes so that we still show the tabs section even for 0 tabs?

::: devtools/client/webide/content/project-listing.xhtml
@@ +25,5 @@
>          <div id="project-panel-projects"></div>
>          <label class="panel-header" id="panel-header-runtimeapps" hidden="true">&projectPanel_runtimeApps;</label>
>          <div id="project-panel-runtimeapps"/>
>          <label class="panel-header" id="panel-header-tabs" hidden="true">&projectPanel_tabs;
> +          <button class="project-panel-item-refreshtabs refresh-icon" id="refresh-tabs" arial-label="&projectMenu_refreshTabs_label;"></button>

Maybe this should be a `title` attribute so it appears as a tooltip on hover as well?  This seems to be what the browser page reload button does, for example.

Or if you hate that for some reason, it should at least be `arial` -> `aria`.

::: devtools/client/webide/content/runtime-listing.xhtml
@@ +18,5 @@
>    <body>
>      <div id="runtime-panel">
>        <div id="runtime-panel-box">
> +        <label class="panel-header">&runtimePanel_usb;
> +          <button class="runtime-panel-item-refreshdevices refresh-icon" id="refresh-devices" aria-label="&runtimePanel_refreshDevices_label;"></button>

Maybe this should be a `title` attribute so it appears as a tooltip on hover as well?  This seems to be what the browser page reload button does, for example.

::: devtools/client/webide/themes/panel-listing.css
@@ +91,5 @@
>    cursor: default;
>  }
>  
> +.refresh-icon {
> +  background-image: url("chrome://devtools/skin/images/reload.svg");

The icon seems lighter gray than other WebIDE icons, and it seems different than the Firefox page reload too.  Were you trying to match it to something like Helen said in comment 6?

I don't feel strongly what we match it to myself, as long as it can be explained reasonably.

Comment 12

[Jen Fong-Adwent [:ednapiranha]]

Attachment: Bug1210123.patch

Updated with fixes - I chatted with Helen and the intention was to lighten the gray to roughly match the gray level of the other icons in WebIDE rather than the browser refresh (which is the default opacity 1.0). I think there are larger discussions she is having with Brian about consolidating rules for these styles in the near future (e.g. having an official shade rule for icons in certain states, etc). We both seem to be happy with this fix for the time being but I am happy to change it if you think it is better in the default style.

Also, updated the project tab list after clarification.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c8d74190d3a

Comment 13

[Tim Nguyen :ntim]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #11)
> >    cursor: default;
> >  }
> >  
> > +.refresh-icon {
> > +  background-image: url("chrome://devtools/skin/images/reload.svg");
> 
> The icon seems lighter gray than other WebIDE icons, and it seems different
> than the Firefox page reload too.  Were you trying to match it to something
> like Helen said in comment 6?
> 
> I don't feel strongly what we match it to myself, as long as it can be
> explained reasonably.
The one that is used inside Firefox's URL bar is reload-small.svg inside the zip. It's the icon sized at 14px.

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8686909 [details] [diff] [review]
Bug1210123.patch

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

Great, this version looks good to me!

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

[Tracking Requested - why for this release]: This should improve the experience of using some runtimes like Valence by exposing the tabs refresh button.  The switch to sidebars has made them harder to use until this fix.

Comment 16

[Pulsebot]

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

JRyans, should we uplift this to Aurora44 and Beta43 as those FF versions are marked as affected? Also, is this worth relnoting? Please let me know.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Joe, redirecting my request in comment 18 to you as JRyans is on PTO. Thanks!

Comment 20

[Joe Walker [:jwalker]]

Rather than find a new owner, I'm going to suggest we wait until Monday because JRyans is back then.

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Ritu Kothari (:ritu) from comment #18)
> JRyans, should we uplift this to Aurora44 and Beta43 as those FF versions
> are marked as affected? Also, is this worth relnoting? Please let me know.

Well, I was going to say it would be good to have in at least 44, but I just realized the patch also adds a new string, so that makes it harder.

I believe the main audience blocked by the issue would be Valence users, which is a small enough set that I believe we can just wait for the trains here.

I don't think it's worth relnoting either.  I'll remove the tracking flag.