1639179 - DevTools Inspector tabs not re-loading same tab after toggling the 3pane

Status: ASSIGNED

(DevTools :: Inspector, enhancement, P3)

Description

[Cristian Fogel, QA [:cfogel]]

Attachment: Screen Recording 0002-05-19 at 15.01.03.mov

Affected versions

• 76.0.1, 77.0b7, 78.0a1 (2020-05-18)

Affected platforms

• Windows 10, macOS 10.15.4, Ubuntu 18.04;

Steps to reproduce

1. Launch Firefox, access any webpage;
2. Open DevTools, select any of the Computed/Changes/Compatibility tabs;
3. Click on the Toggle 3pane button

Expected result

• toggle of 3pane tab should reveal the same-active tab after re-enable;

Actual result

• toggling does not show the Compatibility tab again

Regression range

• Last good revision: (2018-04-05)
• First bad revision: (2018-04-06)
• Pushlog: URL

Additional notes

• attached recording with the issue.

Comment 1

[Cristian Fogel, QA [:cfogel]]

Not a regression, updated the section in the initial comment.
With implementation of the 3pane toggle (feature) the issue became visible.

Comment 2

[Razvan Caliman [:rcaliman]]

Marking this as an enhancement.

Comment 3

[Razvan Caliman [:rcaliman]]

This seems like a good first bug for someone new. Marking myself as mentor.

Learn how to get started here:
https://docs.firefox-dev.tools/

---

The handler for the the 3-pane mode toggle is here:
https://searchfox.org/mozilla-central/rev/7dafc35406b9c945189c617d427f5458933fd3fb/devtools/client/inspector/inspector.js#776-780

When loading the Inspector, the sidebar is set up with the default tab (last one viewed by the user):
https://searchfox.org/mozilla-central/rev/7dafc35406b9c945189c617d427f5458933fd3fb/devtools/client/inspector/inspector.js#1026-1038

Somehow in the addRuleView() call done when toggling 3-pane mode, the defaultTab is not restored. The bug is probably around here:
https://searchfox.org/mozilla-central/rev/7dafc35406b9c945189c617d427f5458933fd3fb/devtools/client/inspector/inspector.js#839-918

Comment 4

[Martín Eizayaga]

Could I be assigned to this? :)

Comment 5

[Razvan Caliman [:rcaliman]]

Sure thing! Let me know if you need more guidance beyond the comment above.

Comment 6

[Martín Eizayaga]

Thanks! I had one question, actually. Do you know of a way how to set up the Visual Studio debugger for ./mach run?

Comment 7

[Razvan Caliman [:rcaliman]]

I don't know. You can try asking in the DevTools channel on Riot.

When debugging DevTools (or Firefox UI) we tend to use another DevTools instance called the Browser Toolbox.

Comment 8

[Release mgmt bot [:sylvestre / :calixte / :marco for bugbug]]

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 9

[RAHUL SAWRA[:rsawra]]

Hi Razvan, is the issue still open?

Comment 10

[vishal]

Hello Razvan,
Can i take this issue?

Comment 11

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

Hi Vishal,
assigned to you.

Razvan is no longer with Mozilla so, we need to figure our the way how to fix this ourselves.
Instructions in comment #3 might be good start
:jdescottes might also know more details.

Thank for helping!

Honza

Comment 12

[vishal]

Ok sir let me try this.

Comment 13

[vishal]

Hello :jdescottes,
Can you tell me about addRuleView function of inspector.js?

Comment 14

[vishal]

Hello :jdescottes.
Can you help me?

Comment 15

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

@vishal: you can use the "Request information from" field at the bottom of this page to get attention.

Honza

Comment 16

[Julian Descottes [:jdescottes]]

The comment at https://searchfox.org/mozilla-central/rev/7dafc35406b9c945189c617d427f5458933fd3fb/devtools/client/inspector/inspector.js#843-844 explicitly says that the Rules panel should become the default one when switching from the 3 pane view to 2 pane view.

So we need to be careful here, we don't want to change that.

Can you tell me about addRuleView function of inspector.js?

Sure, but do you have specific questions about the method :) ?

You can find all call sites using searchfox: https://searchfox.org/mozilla-central/search?path=&q=addRuleView
This is only called when we initially setup the sidebar, and everytime we toggle between 3pane view and 2pane view.

The important thing to note is that the method accepts a defaultTab parameter. When it's not provided, it will use "ruleview" instead.

Let's look at the call site from setupSidebar:

    let defaultTab = Services.prefs.getCharPref(
      "devtools.inspector.activeSidebar"
    );

    if (this.is3PaneModeEnabled && defaultTab === "ruleview") {
      defaultTab = "layoutview";
    }

    // Append all side panels
    this.addRuleView({ defaultTab });

https://searchfox.org/mozilla-central/rev/be413c29deeb86be6cdac22445e0d0b035cb9e04/devtools/client/inspector/inspector.js#1028-1034

and at the call site from onSidebarToggle:

    this.is3PaneModeEnabled = !this.is3PaneModeEnabled;
    await this.setupToolbar();
    this.addRuleView({ skipQueue: true });

So what we want to do here, is to restore the selected tab when we switch from 2 pane to 3 pane. But when we go from 3 pane to 2 pane, we still want to select the ruleview. I would suggest to:

• extract the code to read defaultTab from setupSidebar to a dedicated method
• call this new method from onSidebarToggle
• in onSidebarToggle, detect if we are switching from 2pane to 3pane (based on the value of this.is3PaneModeEnabled)
• if we are switching from 2pane to 3pane pass the computed defaultTab to addRuleView

I think we could further improve this by removing the default value of defaultTab in addRuleView. It makes the behavior hard to predict. Explicitly requesting a value would be much easier to understand.

This will also need a new test (browser mochitest) to assert the behavior. We have a few tests for 3pane mode (https://searchfox.org/mozilla-central/search?path=&q=devtools%2Fclient%2Finspector%2Ftest%2Fbrowser_inspector_pane-toggle-0) but none of them are checking the selected tab behavior.

Vishal: Take a look at my hints and suggestions. For the record, I think this bug is more complex than the average devtools' good-first-bug. Let us know if that's ok.