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

RESOLVED FIXED in Firefox 58

Status

P3
enhancement
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: jdescottes, Assigned: jdescottes)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 58
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Updated

a year ago
Blocks: 1408339
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
(Assignee)

Updated

a year ago
Attachment #8922871 - Flags: review?(poirot.alex)
(Assignee)

Updated

a year ago
Attachment #8922870 - Flags: review?(poirot.alex)
(Assignee)

Updated

a year ago
Attachment #8922869 - Flags: review?(poirot.alex)
(Assignee)

Updated

a year ago
Attachment #8922869 - Flags: review?(poirot.alex)
(Assignee)

Updated

a year ago
Attachment #8922870 - Flags: review?(poirot.alex)
(Assignee)

Updated

a year ago
Attachment #8922871 - Flags: review?(poirot.alex)

Comment 5

a year ago
mozreview-review
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 6

a year ago
mozreview-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 7

a year ago
mozreview-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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 16

a year ago
mozreview-review-reply
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.

Comment 17

a year ago
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

Updated

9 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.