3-pane inspector does not restore the previous column sizes

RESOLVED FIXED in Firefox 64

Status

defect
P2
normal
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: pbro, Assigned: grosbouddha)

Tracking

({regression})

unspecified
Firefox 65
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox63 unaffected, firefox64 verified, firefox65 verified)

Details

(Whiteboard: [dt-q])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 months ago
I could swear we had this working in the past, so I'm a bit surprised. But better file it anyway:

Steps:
- open the inspector
- make sure you are in "3-pane mode" (if you see only 2 columns: DOM and CSS then click on the expand icon in the toolbar to see 3 columns: DOM, CSS and Layout)
- resize the 3 columns to whatever you like
- close the inspector
- re-open it again

==> The previous sizes aren't restored, which can be very frustrating.
(Reporter)

Updated

6 months ago
No longer blocks: 1470379
Whiteboard: [dt-q]
(Reporter)

Updated

6 months ago
Keywords: regression
Priority: -- → P2
(Assignee)

Comment 1

5 months ago
Can I take a look at this bug?

btw:
 - it can be reproduced with the 2 columns layout too,
 - it seems to be a recent regression: I can repro on a dev FF, it works fine on FF 63.0 (current prod).
(Assignee)

Updated

5 months ago
Flags: needinfo?(pbrosset)
(Reporter)

Comment 2

5 months ago
Of course, I'll assign it to you. I'm running a mozregression at the moment to find out which bug broke this, maybe this will help us find a solution.
Assignee: nobody → grosbouddha
Status: NEW → ASSIGNED
Flags: needinfo?(pbrosset)
(Reporter)

Comment 3

5 months ago
mozregression completed and pointed to this pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=409be898a58d93400ea8c6cad5d4385b64dc628a&tochange=3f01040106719f099339ebe65ff98690a65ee9f0

Out of the 2 bugs, the cause for this regression is probably bug 1494162. And in particular the patch part 29 in that bug broke this: https://hg.mozilla.org/integration/mozilla-inbound/rev/3f0104010671

So that confirms that this regressed in 64. This means as soon as we find a fix, we should not only land it in m-c as always, but also request an uplift so it also lands on the beta branch (in 64).

Vincent: don't worry about this for now, we have time to work on this, and I'll request the uplift whenever the time is right.
(Assignee)

Comment 4

5 months ago
Thanks for the assignment, taking a look now.

For reference, not all tabs seem to actually record their preferred size (verified on ff 63.0):
 - Splitbox storing/restoring width: inspector and style tabs
 - Splitbox not restoring: a11y and debugger tabs
 - Splitbox not draggable: canvas, memory
 - No Splitbox: console, network, webaudio, dom, shader, scratchpad
(Assignee)

Comment 5

5 months ago
I found the issue.
The sidebar's lifecycle was not properly tied to the inspector's lifecycle.
When the sidebar is destroyed or hidden, it stores the preferred widths for the various panels.
However, the inspector destroy was not calling the sidebar destroy properly.
Properties were never stored. At read time, the pref reading code was throwing and defaulting to the default width values.

The fix should be a one liner and uplift-friendly.
I still need to write a non-regression test.

After reading some of the lifecycle management JS code, I think there is some room for improvement here.
Not sure what's the best forum/mode to make an impactful proposal though.
(Reporter)

Comment 6

5 months ago
(In reply to Vincent from comment #5)
> After reading some of the lifecycle management JS code, I think there is
> some room for improvement here.
> Not sure what's the best forum/mode to make an impactful proposal though.
Great work so far! 
I suggest joining the public devtools-html slack and the #inspector channel there. The relevant people to include in this conversation would be @gl, @rcaliman I think.
(Assignee)

Comment 7

5 months ago
Attachment #9024038 - Flags: review?(pbrosset)
(Assignee)

Comment 8

5 months ago
Attachment #9024038 - Attachment is obsolete: true
Attachment #9024038 - Flags: review?(pbrosset)
Vincent, it looks like the review flag was cleared on the second version of the patch...  Is that one intended for review now?
Flags: needinfo?(grosbouddha)
(Assignee)

Comment 10

5 months ago
Adding back the patch with a review request. Thanks!
Attachment #9024041 - Attachment is obsolete: true
Flags: needinfo?(grosbouddha)
Attachment #9024051 - Flags: review?(pbrosset)
(Assignee)

Comment 11

5 months ago
Ready for review, thanks jryans!
(Reporter)

Comment 12

5 months ago
Comment on attachment 9024051 [details] [diff] [review]
fix_inspector_layout_on_open.patch

Review of attachment 9024051 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks a lot Vincent for submitting a fix for this so quickly!
Let me pass this on to Gabriel since he was the one who worked on this originally.
Attachment #9024051 - Flags: review?(pbrosset) → review?(gl)
Comment on attachment 9024051 [details] [diff] [review]
fix_inspector_layout_on_open.patch

Review of attachment 9024051 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/inspector/test/browser_inspector_sidebarstate.js
@@ +41,5 @@
>  ];
>  
> +// Verify that the inspector properly read/write from user preferences to
> +// layout the inspector split boxes.
> +add_task(async function() {

We should move this to a new test file. Perhaps name it browser_inspector_pane_state_restore.js.

@@ +43,5 @@
> +// Verify that the inspector properly read/write from user preferences to
> +// layout the inspector split boxes.
> +add_task(async function() {
> +  // Simulate that the user already has stored splitboxes preferred widths.
> +  const EXPECTED_INITIAL_WIDTH = 101;

We should move these constants outside the add_task(). See https://searchfox.org/mozilla-central/source/devtools/client/inspector/test/browser_inspector_pane-toggle-03.js.

@@ +46,5 @@
> +  // Simulate that the user already has stored splitboxes preferred widths.
> +  const EXPECTED_INITIAL_WIDTH = 101;
> +  const EXPECTED_INITIAL_HEIGHT = 102;
> +  const EXPECTED_INITIAL_SIDEBAR_WIDTH = 103;
> +  Services.prefs.setIntPref(

We should use "await pushPref" to set these values.

@@ +53,5 @@
> +    "devtools.toolsidebar-height.inspector", EXPECTED_INITIAL_HEIGHT);
> +  Services.prefs.setIntPref(
> +    "devtools.toolsidebar-width.inspector.splitsidebar", EXPECTED_INITIAL_SIDEBAR_WIDTH);
> +
> +  // Open inspector.

We can remove this comment since it's implied by the function name.

@@ +54,5 @@
> +  Services.prefs.setIntPref(
> +    "devtools.toolsidebar-width.inspector.splitsidebar", EXPECTED_INITIAL_SIDEBAR_WIDTH);
> +
> +  // Open inspector.
> +  const res = await openInspectorForURL("about:blank");

Simplify to const { inspector } = await openInspectorForURL("about:blank");
Attachment #9024051 - Flags: review?(gl)
(Assignee)

Comment 14

5 months ago
Thanks Gabriel for the review!
However, I'm on a vacation break this week and don't have access to my dev machine right now.
I will continue on this next week, most likely Tuesday/Wednesday.
(Assignee)

Comment 15

5 months ago
I applied your comments and ran eslint/mochitests.
I did a new round of local testing too since the original patch was quite old.

Please take another look to the updated patch.
Attachment #9024051 - Attachment is obsolete: true
Flags: needinfo?(gl)

Comment 16

5 months ago
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/decb54eeaa7d
Restore the previous inspector layout when reopening. r=gl
Thank you for the patch, Vincent. I made minor comment changes to your unit test and landed it.
Flags: needinfo?(gl)
Attachment #9028754 - Flags: review+

Comment 18

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/decb54eeaa7d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Maybe request uplift?
Flags: needinfo?(gl)
Note that Fx64 is being released tomorrow, so the approval request will need to be for mozilla-release for possible dot release inclusion.
Flags: in-testsuite+
Comment on attachment 9028754 [details] [diff] [review]
fix_inspector_layout_on_open.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1503175

User impact if declined: 3-pane inspector does not restore the previous column sizes

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: Yes

If yes, steps to reproduce: 1. Open the inspector in 3 pane mode.
2. Change the width of the any of the sidebars.
3. Close the devtools
4. Reopen the inspector
5. Check that the widths of the newly opened inspector is the same as before it was closed

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Covered by automated tests, and the code itself is a simply destroy method call to ensure the widths of the inspector is stored in the prefs when the devtools are closed.

String changes made/needed:
Flags: needinfo?(gl)
Attachment #9028754 - Flags: approval-mozilla-release?

Comment on attachment 9028754 [details] [diff] [review]
fix_inspector_layout_on_open.patch

looks safe enough to ride along in 64.0.2.

Attachment #9028754 - Flags: approval-mozilla-release? → approval-mozilla-release+

Reproduced this issue using Fx 64.0b buildID 20181206201918 on Windows 10 x64.

Confirming the fix across platforms using:

  • 64.0.2, buildID 20190108160530
  • 65.0b9, buildID 20190107180200
You need to log in before you can comment on or make changes to this bug.