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 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]

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.

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.

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.

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.

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

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

https://hg.mozilla.org/mozilla-central/rev/8b549261f40d https://hg.mozilla.org/mozilla-central/rev/1ce779d6d880