1625121 - Selecting a tab in a tab-bar should make it visible

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P3)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

STR:

1. Load any page e.g. google.com
2. Select the Network panel (but, it's also reproducible with e.g. Inspector panel)
3. Make sure there are some requests and select one to open the Side bar
4. Adjust the Side bar width so, not all side panels are visible
5. Click the drop down arrow to see list of all side panels
6. Selects the last panel

AR:
The panel is selected but its tab isn't visible

ER:
The panel should be selected and its tab should visible to the user.

Honza

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: image.png

Attaching a screenshot showing the problem

The Timings panel is selected by you can see that its tab isn't visible.

Honza

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Some instructions for anyone interested in fixing this.

1. The list of side panel/tab is implemented on top of TabBar component
   https://searchfox.org/mozilla-central/rev/4d9cd186767978a99dafe77eb536a9525980e118/devtools/client/shared/components/tabs/TabBar.js#33

2. This component renders list of Tab components
   https://searchfox.org/mozilla-central/rev/4d9cd186767978a99dafe77eb536a9525980e118/devtools/client/shared/components/tabs/TabBar.js#352

3. The Tabs components renders the final tab-list and the drop down menu
   https://searchfox.org/mozilla-central/rev/4d9cd186767978a99dafe77eb536a9525980e118/devtools/client/shared/components/tabs/Tabs.js#35

The drop down menu is displayed only if the list of tabs overflows the available space
https://searchfox.org/mozilla-central/rev/4d9cd186767978a99dafe77eb536a9525980e118/devtools/client/shared/components/tabs/Tabs.js#344

4. Here is the complete logic rendering the list of tabs
   https://searchfox.org/mozilla-central/rev/4d9cd186767978a99dafe77eb536a9525980e118/devtools/client/shared/components/tabs/Tabs.js#280-362

We need to make the list of tabs srollable and auto-scroll to ensure visibility of the selected tab.
Perhaps play with tabs-menu element content positioning using CSS?

Honza

Comment 3

[Ria Agnes Jose]

Hi, I would like to work on this bug. Could you please assign it to me?

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Done, it's yours now.

Honza

Comment 5

[Firefox Bug Husbandry Bot]

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is P3 (Backlog,) indicating it has been triaged, the bug's Severity is being updated to S3 (normal.)

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 7

[Gagah Pangeran Rosfatiputra [:GPR]]

Hello Honza! I would like to work on this bug.

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Assigned to you, thank you for helping.

Comment 9

[Gagah Pangeran Rosfatiputra [:GPR]]

Attachment: main tab.gif

Before I work on this, there's a questions I want to ask.

The main tabs in devtools have this behaviour, it just replace the last tab with current viewed overflow tab. (see attachment gif)

Should we use this approach to make it consistent or go with your proposal solution with scrollable tab?

Thank you!

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Gagah Pangeran Rosfatiputra from comment #9)

Before I work on this, there's a questions I want to ask.

The main tabs in devtools have this behaviour, it just replace the last tab with current viewed overflow tab. (see attachment gif)

Should we use this approach to make it consistent or go with your proposal solution with scrollable tab?

Good point. I've been discussing this with others and we should try to mimic what the Toolbox panel-tabs are doing (the main tabs in devtools). Later (in a follow up bug) we could also try to reuse the same React component for both.

Honza

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 12

[Kevin Pellet (Ilphrin)]

Hi,

Can I be assigned to this bug please ?

I've looked at the code you linked and figured out where to put some CSS to allow scrolling, I still need to look for the best way to auto scroll at the desired tab

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Assigned to you, thank you for helping.
Honza

Comment 14

[Kevin Pellet (Ilphrin)]

Attachment: WIP: Bug 1625121 - Make tabs-menu scrollable and auto scroll to selected tab

Comment 15

[Kevin Pellet (Ilphrin)]

Hello Honza

Here is a first try at fixing this, I added a right margin of 15px because of the dropdown button
Please let me know if there is anything to add or modify!

Comment 16

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thank you!

I just posted a comment in the Phabricator.

Honza

Comment 17

[Kevin Pellet (Ilphrin)]

Hello!
I can't figure out why tabDomElement is null in the case of the inspector, there is a tab with the correct id, but it can't be reached, even within the Browser Toolbox I can't get it from document.getElementById, even though it works for the network panel, do you know why it is not accessible ?

Kevin

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

The patch is using global document variable, which is pointing to different documents.
(depending on how the TabBar.js module is loaded)

• NetMonitor - "chrome://devtools/content/netmonitor/index.html"
• Inspector - "about:devtools-toolbox"

Using global variables isn't good practice and we should access the current document e.g. from an event object

Try the following.
Does it work for you?

diff --git a/devtools/client/shared/components/tabs/TabBar.js b/devtools/client/shared/components/tabs/TabBar.js
--- a/devtools/client/shared/components/tabs/TabBar.js
+++ b/devtools/client/shared/components/tabs/TabBar.js
@@ -242,27 +242,27 @@ class Tabbar extends Component {
         // the panel if needed.
         if (tabs.length > 0 && this.props.onSelect) {
           this.props.onSelect(this.getTabId(activeTab));
         }
       }
     );
   }

-  select(tabId) {
+  select(doc, tabId) {
     const index = this.getTabIndex(tabId);
     if (index < 0) {
       return;
     }

     const newState = Object.assign({}, this.state, {
       activeTab: index,
     });

-    const tabDomElement = document.getElementById(`${tabId}-tab`);
+    const tabDomElement = doc.getElementById(`${tabId}-tab`);
     tabDomElement.scrollIntoView();

     this.setState(newState, () => {
       if (this.props.onSelect) {
         this.props.onSelect(tabId);
       }
     });
   }
@@ -308,17 +308,17 @@ class Tabbar extends Component {

     // Generate list of menu items from the list of tabs.
     this.state.tabs.forEach(tab => {
       menu.append(
         new MenuItem({
           label: tab.title,
           type: "checkbox",
           checked: this.getCurrentTabId() === tab.id,
-          click: () => this.select(tab.id),
+          click: () => this.select(target.ownerDocument, tab.id),
         })
       );
     });

     // Show a drop down menu with frames.
     menu.popupAtTarget(target, this.props.menuDocument);

     return menu;

Comment 19

[Kevin Pellet (Ilphrin)]

Indeed! I never had to deal with multiple documents so I didn't know how to handle this, your modifications works perfectly I'll update the patch, thanks you!

Comment 20

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Perfect

We'll also need an automated test for this feature to avoid any regressions in the future.

There are some existing tests you can use as an inspiration:

• Here is the directory with Network panel tests
  https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/

• Every test needs to be registered in browser.ini file (in the right order)
  https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser.ini

• This test opens/closes the side bar: browser_net_pane-toggle.js

• This test checks that Params are sorted: browser_net_params_sorted.js

• This test check POST data: browser_net_post-data-01.js

And there are others...

You might also read docs here: https://firefox-source-docs.mozilla.org/devtools/tests/mochitest-devtools.html

The test we introduce should check that selecting a tab in a tab-bar makes it visible.

Honza

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 22

[Kevin Pellet (Ilphrin)]

Hi Honza,

Sorry for the veeeeery long delay ! I didn't have any more time to focus on contributing to open source, but now I can try to finish this patch

Currently, I am stuck writing the test, I understand how to write one, but I am not sure how I can retrieve the details tabs popup. Maybe you can help me with that ?
What I would like to achieve is to loop through all elements in the popup menu, click on each element and check if the offset of the selected element is correct relative to the scrollOffset if the <nav> element, but as I do not understand how to get the popup from the DOM...

Here is my current code:

  const { tab, monitor } = await initNetMonitor(SIMPLE_URL, {
    requestCount: 1,
  });
  info("Starting test... ");

  const { document, store, windowRequire } = monitor.panelWin;
  const Actions = windowRequire("devtools/client/netmonitor/src/actions/index");

  const networkEvent = waitForNetworkEvents(monitor, 1);
  tab.linkedBrowser.reload();
  await networkEvent;

  store.dispatch(Actions.toggleNetworkDetails());

  clickOnSidebarTab(document, "headers");

  const tabsMenu = document.querySelector(".tabs-menu");

  is(tabsMenu.scrollLeft, 0, "The details panel should be scrolled left");

  await teardown(monitor);

Again, sorry for being afk for so long, and thank you for your help
Kevin

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

1. The context menu (containing list of all tabs) is generated here:
   https://searchfox.org/mozilla-central/rev/15f12b0c6c56b449304f6cb1f84ac4df84dc936a/devtools/client/shared/components/tabs/TabBar.js#302
   It's a simple loop iterating over all tabs in the tab-bar and generating list of menu items

2. Here is the place were the onAllTabsMenuClick above is passed into Sidebar component
   https://searchfox.org/mozilla-central/rev/15f12b0c6c56b449304f6cb1f84ac4df84dc936a/devtools/client/shared/components/tabs/TabBar.js#346
   Sidebar component is built on top of the Tabs and TabBar components

3. It looks like you need to set ID for the Menu object (i.e. DOM element) created in #1 (within theonAllTabsMenuClick function) , so it can be later queried by the test.

4. And the test can just use querySelector API to get the Menu element and iterate over its child nodes.

Does that make sense?

Again, sorry for being afk for so long, and thank you for your help

No worries, thank you for helping to finish the patch!

Honza

Comment 24

[Kevin Pellet (Ilphrin)]

Attachment: WIP: Bug 1625121 - Add tests

Depends on D122489

Comment 25

[Kevin Pellet (Ilphrin)]

Thanks you for this explanation!
I gave a try at adding tests, though I am not sure if this is the right way to do it so please let me know if there is anything I should change in order to improve the readability of the code! :)

Comment 26

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Sorry for the delay, my review comments are in Phab
https://phabricator.services.mozilla.com/D143656

Please always need info me, so I have the request in my queue.

Comment 27

[Kevin Pellet (Ilphrin)]

Hi!

I tried reproducing this error you've seen in the Inspector panel but it is working for me and I see no errors in the Browser console :/

Sorry I'll keep that in mind!

Comment 28

[Jan Honza Odvarko [:Honza] (always need-info? me)]

When testing that patch manually, it nicely works and fixes the issue in both the Network and Inspector panel.

When running the test one time it also works, but when running the test twice in a row it fails the second time.

Here is what I am doing (Win10, Firefox Nightly):

1. Apply both patches (https://phabricator.services.mozilla.com/D122489, https://phabricator.services.mozilla.com/D143656#inline-797099)
   Btw. the "Add tests" patch should be rebased on top of the latest m-c

2. Build: $ mach build faster

3. Run test once: $ mach test devtools/client/netmonitor/test/browser_net_tabbar_focus.js => PASS

4. Run test two times in a row: $ mach test devtools/client/netmonitor/test/browser_net_tabbar_focus.js --repeat 2 => FAILS

Here is the failure stack trace:

Stack trace:
chrome://mochikit/content/browser-test.js:test_ok:1400
chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_tabbar_focus.js:selectTabFromTabsMenuButton:66
chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_tabbar_focus.js:null:82
chrome://mochikit/content/browser-test.js:handleTask:988
chrome://mochikit/content/browser-test.js:_runTaskBasedTest:1060
chrome://mochikit/content/browser-test.js:Tester_execTest:1195
chrome://mochikit/content/browser-test.js:nextTest/<:971
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/<:1038
  FAIL The cookies tab is visible -

Can you see that error on your machine?

Comment 29

[Kevin Pellet (Ilphrin)]

Ohhhh right I have the error too. I tried adding some await wait(10000); in the test but the test doesn't seem to care about it, and when running with repeat param, it looks like the panel is very tiny on the second run, like if the synthesizeMouse calls werent' run

is there something I am missing here ? Like is there something I do that block the test from actually awaiting ?

Comment 30

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Kevin Pellet (Ilphrin) from comment #29)

is there something I am missing here ? Like is there something I do that block the test from actually awaiting ?

Julian, any tips about why the test fails when running the second time?

Comment 31

[Julian Descottes [:jdescottes]]

Splitter size is stored in a preference called devtools.netmonitor.panes-network-details-width, so the second time you run the test the details panel is already shrinked when the test starts.

Reset the pref via Services.prefs.clearUserPref("devtools.netmonitor.panes-network-details-width"); for instance in https://searchfox.org/mozilla-central/rev/4a15041348e08fb0d6f5726015c32861e663fbfe/devtools/client/netmonitor/test/head.js#205-214

Comment 32

[Kevin Pellet (Ilphrin)]

Attachment: Bug 1625121 - Fix repeated test by resetting the panel width setting r?Honza

Depends on D143656

Comment 33

[Kevin Pellet (Ilphrin)]

Thank you Julian it worked like a charm!

I am not sure though why moz-phab made another diff instead of using the old one :/ Is that an issue?

Comment 34

[Julian Descottes [:jdescottes]]

(Honza is away until August 21st, don't hesitate to ping me instead in the meantime :) )

(In reply to Kevin Pellet (Ilphrin) from comment #33)

Thank you Julian it worked like a charm!

Great!

I am not sure though why moz-phab made another diff instead of using the old one :/ Is that an issue?

moz-phab will always create one diff per commit. From the looks of it, you created several commits on top of each other, so moz-phab is creating one diff for each. This is typically how folks usually work on services like GitHub before squashing everything at merge time. But here we do things slightly differently. See
https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html#to-update-a-submitted-patch

Basically you can use git/hg commit --amend to update an existing commit with new changes. Now for your current stack, you already have 3 different commits stacked, so we cannot use --amend. We need to find a way to create a single commit containing all your changes. You can either try to rewrite the history of your local repository if you are comfortable with that (hg histedit or git rebase -i are usually good tools to do that). Or you can manually update/checkout the latest central, redo your changes and push again to phabricator. If you rewrite the history, moz-phab should normally update the first diff which was created, and we can then abandon the 2 others. If you created a completely new commit, moz-phab will create a new diff and we'll abandon the 3 existing ones.

Both solutions are fine. Don't hesitate to join https://chat.mozilla.org/#/room/#devtools:mozilla.org and ping me there (nickname is also "jdescottes") if it gives you any trouble.

Comment 35

[Kevin Pellet (Ilphrin)]

Attachment: WIP: Bug 1625121 - Make tabs-menu scrollable and auto scroll to selected tab

Comment 36

[Kevin Pellet (Ilphrin)]

Thanks Julian! I re-submitted a patch with hg histedit, I double-checked it and it seems to contain everything :D

Comment 37

[Julian Descottes [:jdescottes]]

Forgot to clear the needinfo, thanks for uploading the new patch :)

Feel free to abandon the other ones (you can select the option on phabricator, above the comment box)

Comment 38

[BugBot [:suhaib / :marco/ :calixte]]

The following patch is waiting for review from an inactive reviewer:

ID	Title	Author	Reviewer Status
D154296	Bug 1625121 - Fix repeated test by resetting the panel width setting r?Honza	ilphrin	Honza: Inactive

:ilphrin, could you please find another reviewer?

For more information, please visit auto_nag documentation.

Comment 39

[Julian Descottes [:jdescottes]]

:ilphrin, feel free to keep Honza as reviewer, and you can also add me directly if you want. It would be great if you could abandon the outdated revisions: D154296, D143656 and D122489 . And move the new one to review?

Comment 40

[Suhaib Mujahid [:suhaib]]

Sorry for the noise, there was a problem with a Bugzilla API that caused the bot to think more people were inactive than normal (bug 1789646).

Comment 41

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 42

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 43

[jynk.zilla]

Hi Honza! I would like to work on this bug.

Comment 44

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Assigned to you

Honza

Comment 45

[jynk.zilla]

Attachment: Bug 1625121 - [devtools] scrolling the tabs to the seleted tab

Comment 46

[jynk.zilla]

Hello Honza! I have sent my code to the Phabricator.

Comment 47

[Julian Descottes [:jdescottes]]

(In reply to jynk.zilla from comment #46)

Hello Honza! I have sent my code to the Phabricator.

Your patch at https://phabricator.services.mozilla.com/D173438 has been uploaded as a draft. If you want to start collecting feedback on it, can you submit it for review? This way it will be easier for us to receive notifications about questions & comments you will add on phabricator.

Comment 48

[jynk.zilla]

(In reply to Julian Descottes [:jdescottes] from comment #47)

(In reply to jynk.zilla from comment #46)

Hello Honza! I have sent my code to the Phabricator.

Your patch at https://phabricator.services.mozilla.com/D173438 has been uploaded as a draft. If you want to start collecting feedback on it, can you submit it for review? This way it will be easier for us to receive notifications about questions & comments you will add on phabricator.

Thanks
I submitted it for review

Comment 49

[jynk.zilla]

Attachment: WIP: Bug 1625121 - Refactoring Tabbar, Tabs components

Depends on D173438

Comment 50

[jynk.zilla]

I changed the patch in Phabricator

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/576c09adcdaa
[devtools] scrolling the tabs to the seleted tab r=jdescottes,devtools-reviewers

Comment 52

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/576c09adcdaa