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

RESOLVED FIXED in Firefox 58

Status

()

Firefox
Developer Tools
P3
enhancement
RESOLVED FIXED
a month ago
23 days ago

People

(Reporter: jdescottes, Assigned: jdescottes)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Attachment #8922871 - Flags: review?(poirot.alex)
Attachment #8922870 - Flags: review?(poirot.alex)
Attachment #8922869 - Flags: review?(poirot.alex)
Clearing review flags until I have results from try.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7125f0f49e7c37ddcb3da181327c1fc40f2a9790
Attachment #8922869 - Flags: review?(poirot.alex)
Attachment #8922870 - Flags: review?(poirot.alex)
Attachment #8922871 - Flags: review?(poirot.alex)

Comment 5

27 days 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

27 days 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

27 days 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

24 days 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

24 days 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

23 days 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
https://hg.mozilla.org/mozilla-central/rev/fe394cb8b022
https://hg.mozilla.org/mozilla-central/rev/3417172742e8
https://hg.mozilla.org/mozilla-central/rev/f046fbaaaab6
Status: ASSIGNED → RESOLVED
Last Resolved: 23 days ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.