Adjust the widths of the split box on toggling of the split rule view

RESOLVED FIXED in Firefox 61

Status

enhancement
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: gl, Assigned: gl)

Tracking

(Blocks 1 bug)

unspecified
Firefox 61
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

Last year
When we collapse the split rule view, we should collapse the sidebar split box to be the width of the right most sidebar. When toggling on the split rule view, we should make each panel in the inspector 1/3 of the total available width.

Comment 2

Last year
mozreview-review
Comment on attachment 8945995 [details]
Bug 1433611 - Adjust the widths of the split box on toggling of the split rule view.

https://reviewboard.mozilla.org/r/216062/#review221856


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/inspector.js:661
(Diff revision 1)
>          this.sidebarToggle.setState({ collapsed: true });
>        }
> +      // Set the width of the split box to the width of the inspector sidebar.
> +      this.splitBox.setState({
> +        width: this.splitBox.state.width - this.sidebarSplitBox.state.width,
> +      })

Error: Missing semicolon. [eslint: semi]
Comment hidden (mozreview-request)
Assignee

Updated

Last year
Blocks: 1433716
Assignee

Updated

Last year
No longer blocks: 1369945
Assignee

Updated

Last year
Attachment #8945995 - Flags: review?(bgrinstead)
Just adding an additional concern we noticed:
Currently in Nightly, when switching to vertical mode, the two bottom columns are different widths with the rules being way wider.
It should default to 50-50 widths if vertical mode is anywhere narrower than that point at which it switches to 3-column.
Comment hidden (mozreview-request)
Assignee

Comment 6

Last year
For context, when the inspector is in landscape mode, it will first try to toggle on the 3 pane mode by creating a split rule view with the same sidebar width, but if the original sidebar width was already big and doubling the width of the sidebar's width would effectively be bigger than half of the toolbox's width, we will instead toggle on the 3 pane mode all with equal widths.

When the inspector is in portrait mode, it will just toggle on its T shape pane such that the 2 bottom panes are equal widths.

We also increased the breakpoint width of the toolbox's side view to 1000px so that it will keep its T shape until the toolbox's width is bigger than 1000px.
Assignee

Updated

Last year
Blocks: 1447562
Comment on attachment 8945995 [details]
Bug 1433611 - Adjust the widths of the split box on toggling of the split rule view.

https://reviewboard.mozilla.org/r/216062/#review235466

This looks mostly good to me if this was agreed with Victoria (which I think is the case).
Apart from the few comments below, I have one general comment: can you rephrase comments that mention "split rule" to mention "3 pane" instead, since that is the agreed naming for this feature. We might as well call it the same in the docs, in the onboarding experience, and in the code. It just makes everything easier to understand.

Oh, and finally, this adds quite a bit of feature logic, which can be understood by reading the code and comments, but it would be even more obvious if there was an integration test for it. This way we would know exactly what the expectation for this feature is (on top of having the added value of avoiding regressions automatically).

::: commit-message-96cf0:1
(Diff revision 3)
> +Bug 1433611 - Adjust the widths of the split box on toggling of the split rule view. r=pbro

Can you please add more lines below the commit message that essentially contain the text you added in your last bugzilla comment. So that in the future, we can easily find all of the information about why we did this by just looking at the commit.

::: devtools/client/inspector/inspector.js:51
(Diff revision 3)
>  
>  // Sidebar dimensions
>  const INITIAL_SIDEBAR_SIZE = 350;
>  
> -// If the toolbox width is smaller than given amount of pixels,
> +// If the toolbox width is smaller than the given amount of pixels,
>  // the sidebar automatically switches from 'landscape' to 'portrait' mode.

I think this comment should explain what landscape and portrait modes even mean for the sidebar.
I think a little ascii-art would be a good idea here in the comment.

::: devtools/client/inspector/inspector.js:53
(Diff revision 3)
>  const INITIAL_SIDEBAR_SIZE = 350;
>  
> -// If the toolbox width is smaller than given amount of pixels,
> +// If the toolbox width is smaller than the given amount of pixels,
>  // the sidebar automatically switches from 'landscape' to 'portrait' mode.
>  const PORTRAIT_MODE_WIDTH = 700;
> +// If the side toolbox width is smaller than the given amount of pixels,

Similarly here, what does "side toolbox" mean? I mean, I think I know, but better make these comments as self-explanatory as possible, so there isn't any doubt for future people reading the code.

::: devtools/client/inspector/inspector.js:55
(Diff revision 3)
> -// If the toolbox width is smaller than given amount of pixels,
> +// If the toolbox width is smaller than the given amount of pixels,
>  // the sidebar automatically switches from 'landscape' to 'portrait' mode.
>  const PORTRAIT_MODE_WIDTH = 700;
> +// If the side toolbox width is smaller than the given amount of pixels,
> +// the sidebar automatically switches from 'landscape' to 'portrait' mode.
> +const SIDE_PORTAIT_MODE_WIDTH = 1000;

Should the `WIDTH` part be instead named `MIN_WIDTH` or maybe `THRESHOLD` so it's more obvious.
Same comment for the `PORTRAIT_MODE_WIDTH`.

::: devtools/client/inspector/inspector.js:632
(Diff revision 3)
> +      const toolboxWidth =
> +        this.panelDoc.getElementById("inspector-splitter-box").clientWidth;
> +      // Whether or not doubling the sidebar's width for the main split box will be
> +      // bigger the half the size of the toolbox's width.
> +      const canDoubleSidebarWidth = (sidebarWidth * 2) < (toolboxWidth / 2);
> +      const isLandscapeMode = this.useLandscapeMode();

Move this variable definition to be just above the `if (isLandscapeMode)` below.
Or in fact, no need for the variable if it's just used in the if condition anyway. Just call the function there.
Attachment #8945995 - Flags: review?(pbrosset)
Comment hidden (mozreview-request)

Comment 9

Last year
mozreview-review
Comment on attachment 8945995 [details]
Bug 1433611 - Adjust the widths of the split box on toggling of the split rule view.

https://reviewboard.mozilla.org/r/216062/#review239576


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/test/browser_inspector_pane-toggle-04.js:37
(Diff revision 4)
> +  is(sidebarSplitBoxWidth, toolboxWidth / 2, "Got correct sidebar split box width");
> +
> +  info("Click on the toggle button to toggle OFF the 3 pane inspector");
> +  EventUtils.synthesizeMouseAtCenter(button, {}, inspector.panelDoc.defaultView);
> +  await inspector.once("ruleview-added");
> +  debugger;

Error: Unexpected 'debugger' statement. [eslint: no-debugger]
Comment on attachment 8945995 [details]
Bug 1433611 - Adjust the widths of the split box on toggling of the split rule view.

https://reviewboard.mozilla.org/r/216062/#review239574

Hey Gabriel. I have a few more comments, but they are mostly about comments/naming and tests. So R+ anyway, given that you address them before landing.

::: devtools/client/inspector/inspector.js:599
(Diff revision 4)
>      await this.setupToolbar();
>      await this.addRuleView();
>    },
>  
>    /**
> -   * Adds the rule view to the main or split sidebar depending on whether or not it is
> +   * Adds the rule view to the middle/bottom-left panel or inspector sidebar depending on

I was confused at first by the "middle/bottom-left" part. I was trying to find where in the toolbox was middle bottom left, but then realized the / meant OR here. So it's either middle (in horizonal mode) or bottom-left (in vertical mode).
Sorry, migth be just me being dense, but some more words to explain this would help.

::: devtools/client/inspector/inspector.js:614
(Diff revision 4)
>    async addRuleView(defaultTab = "ruleview") {
> -    let ruleViewSidebar = this.sidebarSplitBox.startPanelContainer;
> +    const ruleViewSidebar = this.sidebarSplitBox.startPanelContainer;
> +    const toolboxWidth =
> +      this.panelDoc.getElementById("inspector-splitter-box").clientWidth;
>  
>      if (this.isSplitRuleViewEnabled) {

Can we rename this to `this.is3PaneModeEnabled` please?

::: devtools/client/inspector/inspector.js:623
(Diff revision 4)
> +      // Whether or not doubling the inspector sidebar's (right/bottom panel) width will
> +      // be bigger than half the size of the toolbox's width.
> +      const canDoubleSidebarWidth = (sidebarWidth * 2) < (toolboxWidth / 2);

This is only used in landscape mode, so should be moved to be inside of that if body instead.

::: devtools/client/inspector/test/browser_inspector_pane-toggle-01.js:10
(Diff revision 4)
> +Services.prefs.setBoolPref("devtools.inspector.split-sidebar-toggle", true);
> +
> +registerCleanupFunction(function() {
> +  Services.prefs.clearUserPref("devtools.inspector.split-sidebar-toggle");
> +});

We now have a better way to do this:

```
await pushPref("devtools.inspector.split-sidebar-toggle", true);
```

This way, it's just one line, and it gets cleared automatically when the test ends.

Same comment for the 3 other tests below.

::: devtools/client/inspector/test/browser_inspector_pane-toggle-02.js:26
(Diff revision 4)
> +  EventUtils.synthesizeMouseAtCenter(button, {}, inspector.panelDoc.defaultView);
> +  await inspector.once("ruleview-added");

It's usually considered better to start listening before you trigger the action that will result in the event being fired:

```
let onRuleViewAdded = inspector.once("ruleview-added");
EventUtils.synthesizeMouseAtCenter(button, {}, inspector.panelDoc.defaultView);
await onRuleViewAdded;
```

In practice it often works the way you did it because the action triggers a bunch of async stuff anyway, so we have the time to start listening after. But it's more correct the way I said, and if things change in the future, this should avoid races.

(same comment later in this test and in other tests)

::: devtools/client/inspector/test/browser_inspector_pane-toggle-03.js:17
(Diff revision 4)
> +
> +registerCleanupFunction(function() {
> +  Services.prefs.clearUserPref("devtools.inspector.split-sidebar-toggle");
> +});
> +
> +const SIDEBAR_WIDTH = 200;

Are we sure that 200 can always be doubled and still be less than half of the toolbox's size?
What if we run this test on a VM that has its resolution set such that the toolbox is only 600px wide?
I don't know if this is a thing we will ever encounter, but I think a wise thing to do here would be to measure the toolbox first, and if it's too small, then just skip the test (like `ok(true, "fine, can't run, width too small")`)

::: devtools/client/shared/components/splitter/SplitBox.js:73
(Diff revision 4)
>       * The state stores the current orientation (vertical or horizontal)
>       * and the current size (width/height). All these values can change
>       * during the component's life time.

This should be updated now that the state stores more things. It should explain what each of the stored properties are.
Attachment #8945995 - Flags: review?(pbrosset) → review+

Comment 11

Last year
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b549261f40d
Adjust the widths of the split box on toggling on the 3 pane inspector. r=pbro

Comment 12

Last year
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ce779d6d880
Follow up: rename isSplitRuleViewEnabled to is3PaneModeEnabled in rules.js. r=me

Comment 13

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/8b549261f40d
https://hg.mozilla.org/mozilla-central/rev/1ce779d6d880
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61

Updated

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