Closed Bug 1410360 Opened 7 years ago Closed 7 years ago

[dt-onboarding] UX: should focus previous tab when closing devtools onboarding page

Categories

(DevTools :: General, enhancement, P3)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1410360 - open about:devtools tab next to the current tab;
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1410360 - reselect original tab after closing about:devtools;
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1410360 - add mochitest for about:devtools tab selection behavior;
59 bytes, text/x-review-board-request
ochameau
: review+
Details 
STRs:
- disabled devtools & restart (devtools.enabled = false)
- open two tabs:
  - first tab on site1
  - second tab on site2
- on first tab, press a devtools shortcut
- either enable devtools or close the tab

ER: User should go back to first tab with site1.
AR: second tab gets focused.

With only two tabs, this is not really a big deal. But with a lot of tabs opened, forcing the user to find back the tab where they triggered the devtools flow is not really user friendly.
Blocks: 1408339
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Attachment #8922871 - Flags: review?(poirot.alex)
Attachment #8922870 - Flags: review?(poirot.alex)
Attachment #8922869 - Flags: review?(poirot.alex)
Attachment #8922869 - Flags: review?(poirot.alex)
Attachment #8922870 - Flags: review?(poirot.alex)
Attachment #8922871 - Flags: review?(poirot.alex)
Comment on attachment 8922869 [details]
Bug 1410360 - open about:devtools tab next to the current tab;

https://reviewboard.mozilla.org/r/194012/#review199210

Oh, good idea!
Attachment #8922869 - Flags: review?(poirot.alex) → review+
Comment on attachment 8922870 [details]
Bug 1410360 - reselect original tab after closing about:devtools;

https://reviewboard.mozilla.org/r/194014/#review199206

Looks good codewise, also sounds fine to me.

But last time we discussed about that (long time ago in go faster meeting)
people tend to find action restore not important.
Did you discussed that with Victoria or Harald?

::: devtools/shim/aboutdevtools/aboutdevtools.js:80
(Diff revision 1)
>    // Update the current page based on the current value of DEVTOOLS_ENABLED_PREF.
>    updatePage();
>  }, { once: true });
>  
> +window.addEventListener("beforeunload", function () {
> +  // Focus the originator tab.

Is this a word "originator"?
Or did you meant original?

::: devtools/shim/aboutdevtools/aboutdevtools.js:85
(Diff revision 1)
> +  // Focus the originator tab.
> +  let { gBrowser } = Services.wm.getMostRecentWindow("navigator:browser");
> +  if (gBrowser.selectedTab.linkedBrowser.contentWindow !== window) {
> +    // Only try to focus the correct tab if the current tab is the about:devtools page.
> +    return;
> +  }

Today, about:devtools runs in parent process, but ideally it would run in content.

Doing 
if (document.visibilityState != "visible") {
  return;
}

(visibilityState is "hidden" when the tab is in background)

Would make it more compatible with this setup.

::: devtools/shim/aboutdevtools/aboutdevtools.js:92
(Diff revision 1)
> +  for (let tab of gBrowser.tabs) {
> +    let outerWindowID = tab.linkedBrowser.outerWindowID;
> +    if (outerWindowID === tabid) {
> +      gBrowser.selectedTab = tab;
> +    }
> +  }

You should use gBrowser.getBrowserForOuterWindowID here.

::: devtools/shim/devtools-startup.js:548
(Diff revision 1)
> +    }
> +
> +    let selectedTab = gBrowser.selectedTab;
> +    if (selectedTab) {
> +      params.push("tabid=" + selectedTab.linkedBrowser.outerWindowID);
> +    }

nit: you could use gBrowser.selectedBrowser
Attachment #8922870 - Flags: review?(poirot.alex) → review+
Comment on attachment 8922871 [details]
Bug 1410360 - add mochitest for about:devtools tab selection behavior;

https://reviewboard.mozilla.org/r/194016/#review199208

::: devtools/shim/aboutdevtools/test/browser_aboutdevtools_focus_owner_tab.js:29
(Diff revision 1)
> +  synthesizeToggleToolboxKey();
> +
> +  info("Wait for the about:devtools tab to be selected");
> +  await waitUntil(() => isAboutDevtoolsTab(gBrowser.selectedTab));
> +  ok(isAboutDevtoolsTab(gBrowser.selectedTab),
> +    "about:devtools was opened as expected.");

nit: this is redundant with the waitUntil, but not a big deal.

::: devtools/shim/aboutdevtools/test/browser_aboutdevtools_focus_owner_tab.js:41
(Diff revision 1)
> +  gBrowser.moveTabTo(tab1, gBrowser.tabs.length - 1);
> +  await removeTab(aboutDevtoolsTab);
> +
> +  await waitUntil(() => tab1 == gBrowser.selectedTab);
> +  ok(tab1 == gBrowser.selectedTab,
> +    "The correct tab was selected after closing about:devtools.");

nit: same, redundant assert.

::: devtools/shim/aboutdevtools/test/browser_aboutdevtools_focus_owner_tab.js:67
(Diff revision 1)
> +  synthesizeToggleToolboxKey();
> +
> +  info("Wait for the about:devtools tab to be selected");
> +  await waitUntil(() => isAboutDevtoolsTab(gBrowser.selectedTab));
> +  ok(isAboutDevtoolsTab(gBrowser.selectedTab),
> +    "about:devtools was opened as expected.");

nit: same
Attachment #8922871 - Flags: review?(poirot.alex) → review+
Re: action restore, I still think behavior that automatically opens DevTools for the user after installation seems too unexpected, but just re-selecting the tab the user was on, as described here, seems like very normal/expected behavior.
Comment on attachment 8922870 [details]
Bug 1410360 - reselect original tab after closing about:devtools;

https://reviewboard.mozilla.org/r/194014/#review199206

> Is this a word "originator"?
> Or did you meant original?

It is a word :) But I adapted the comment to avoid using it.

> Today, about:devtools runs in parent process, but ideally it would run in content.
> 
> Doing 
> if (document.visibilityState != "visible") {
>   return;
> }
> 
> (visibilityState is "hidden" when the tab is in background)
> 
> Would make it more compatible with this setup.

Good point, fixed. Interestingly I had to update the test. The visibility state is not updated immediately when setting the gBrowser.selectedTab.

> You should use gBrowser.getBrowserForOuterWindowID here.

Done (also used getTabForBrowser to get the actual tab, since I want to select it).

> nit: you could use gBrowser.selectedBrowser

Fixed.
Comment on attachment 8922871 [details]
Bug 1410360 - add mochitest for about:devtools tab selection behavior;

https://reviewboard.mozilla.org/r/194016/#review199208

> nit: this is redundant with the waitUntil, but not a big deal.

I switched them to info(). I added them to have some logs during the test. It's true that using an assert here is confusing.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe394cb8b022
open about:devtools tab next to the current tab;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/3417172742e8
reselect original tab after closing about:devtools;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/f046fbaaaab6
add mochitest for about:devtools tab selection behavior;r=ochameau
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: