1249147 - Add a resizable split pane component to devtools/client/shared/components

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

This commit adds HSplitBox which gives two panes aligned horizontally and a
splitter between them to resize their relative widths as needed.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Add a resizable split pane component to devtools/client/shared/components

Still WIP but seems to work well.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Add a resizable split pane component to devtools/client/shared/components

This commit adds HSplitBox which gives two panes aligned horizontally and a
splitter between them to resize their relative widths as needed.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5de075683a06

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8720881 [details] [diff] [review]
Add a resizable split pane component to devtools/client/shared/components

Lin, bgrins suggested I flag you for additional review. I hope you don't mind! Thanks!

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8720881 [details] [diff] [review]
Add a resizable split pane component to devtools/client/shared/components

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

::: devtools/client/shared/components/h-split-box.js
@@ +53,5 @@
> +    // 1. The relative width of the right pane is 1 - leftWidth. For example,
> +    // with leftWidth = .5, both panes are of equal width; with leftWidth = .25,
> +    // the left panel will take up 1/4 width and the right panel will take up
> +    // 3/4 width.
> +    leftWidth: PropTypes.number,

Note: our current splitters often use a px value for the right pane and then flex the remaining space for the left pane.  I think this will be a common need and is probably desirable UX even for the memory feature (might want to request ux feedback from Helen on that feature to get her opinion).

@@ +79,5 @@
> +
> +    const rect = this.refs.box.getBoundingClientRect();
> +    const { left, right } = rect;
> +    const width = right - left;
> +    const relative = event.clientX - left;

I haven't checked - will this work in RTL locales?

@@ +94,5 @@
> +      {
> +        className: "h-split-box",
> +        ref: "box",
> +        onMouseMove: this._onMouseMove,
> +        onMouseUp: this._onMouseUp,

Won't this miss mouse ups that happen outside of the container (or even the document).  i.e. if I start dragging then move the mouse onto the web content or toolbox tabs then do a mouse up it will still be 'stuck'

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8720881 [details] [diff] [review]
Add a resizable split pane component to devtools/client/shared/components

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

Very excited for this component.

::: devtools/client/shared/components/h-split-box.js
@@ +43,5 @@
> +  },
> +
> +  propTypes: {
> +    // The contents of the left pane.
> +    left: PropTypes.any.isRequired,

Do we want to require content in both panes? There are a few scenarios where we load content in the main (left) window, and then when inspecting something, we open the right pane -- might make other code easier if in this case left is the content, and right is null (resulting in this component not doing much at all), but then we can populate `right` when we do want to display something.

@@ +58,5 @@
> +
> +    // A callback fired when the user drags the splitter to resize the relative
> +    // pane widths. The function is passed the leftWidth value that would put
> +    // the splitter underneath the users mouse.
> +    onResize: PropTypes.func.isRequired,

This probably shouldn't be required, for most simple views, I'd just want the size to change

@@ +71,5 @@
> +    this.setState({ mouseDown: false });
> +    event.preventDefault();
> +  },
> +
> +  _onMouseMove(event) {

We should possibly consider debounce/throttling this function depending on how often it's called

@@ +79,5 @@
> +
> +    const rect = this.refs.box.getBoundingClientRect();
> +    const { left, right } = rect;
> +    const width = right - left;
> +    const relative = event.clientX - left;

While I think we should handle RTL support in a follow up if it's significant, I think it may be better to describe "left" and "right" panes in terms of "main" and "side" or something panels, rather than left/right atleast

::: devtools/client/shared/components/test/mochitest/test_HSplitBox_01.html
@@ +4,5 @@
> +Basic tests for the HSplitBox component.
> +-->
> +<head>
> +  <meta charset="utf-8">
> +  <title>Tree component test</title>

update <title>

@@ +62,5 @@
> +
> +    const { left, top, width } = container.getBoundingClientRect();
> +    const middle = left + width / 2;
> +    const oneQuarter = left + width / 4;
> +    const threeQuarters = left + 3 * width / 4;

nit: capitalize constants so its more obvious below when reading the tests

@@ +77,5 @@
> +    synthesizeMouseAtCenter(splitter, { button: 1, type: "mousedown" }, window);
> +
> +    function aboutEq(a, b) {
> +      dumpn(`Checking ${a} ~= ${b}`);
> +      return Math.abs(a - b) < .01;

nit: `const EPSILON = 0.01` at the top

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from comment #6)
> @@ +79,5 @@
> > +
> > +    const rect = this.refs.box.getBoundingClientRect();
> > +    const { left, right } = rect;
> > +    const width = right - left;
> > +    const relative = event.clientX - left;
> 
> While I think we should handle RTL support in a follow up if it's
> significant, I think it may be better to describe "left" and "right" panes
> in terms of "main" and "side" or something panels, rather than left/right
> atleast

I think there are cases where the 'main' is on the right (like perf tool) and cases where 'main' is on left (like inspector).  Still, it might be good to have a concept of main and side especially if we do the flex behavior as I described in Comment 5 in which the main region is flexible and the side has a fixed px width.  That could maybe be specified as an option like leftWidth: "flex" and rightWidth: 200 or something like that.

Comment 8

[Brian Grinstead [:bgrins]]

Also, if we did want to decouple the panes left/right names which depends on locale we could call them 'start' and 'end' like margin-inline-start / margin-inline-end instead of margin-left / margin-right.

That terminology could also be re-used in a vertical splitter component, and make sense if we wanted to make have an option to make this one automatically switch to a vertical layout at a certain breakpoint.  Side note: I'm not actually sure what the best way to handle that would be with these components - maybe the parent component should re-render on resize and change which type of splitter the children are in, or maybe it should be an option within the splitter component.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

If the width is not a prop, then it can't be maintained across component setup/teardown cycles. For example, if I drag to a certain width, switch views to something that doesn't use this component anymore so it is unmounted/destroyed, and then switch back to the original view again, I want my width to be preserved. If we don't have the width as a prop and keep it in state, it will not be preserved. This means that callers of the component *must* maintain that state somewhere, explicitly pass it in as a prop, and handle updating that state via onResize. Luckily, this is pretty straightforward, see the WIP patch in bug 1149385.

I went through this same thing with the tree component wrt focus and expansion. It turns out that if you want state to persist, you have to persist that state. Obvious, when said that way. Slightly annoying with a little boiler plate, but very straightforward, non-magical, and explicit.

> Also, if we did want to decouple the panes left/right names which depends on locale we could call them 'start' and 'end' like margin-inline-start / margin-inline-end instead of margin-left / margin-right.

I'm ok with "start" and "end" instead of "left" and "right". I'm not a fan of "main".

> We should possibly consider debounce/throttling this function depending on how often it's called

I haven't found a need for debouncing; everything is smooth. This can easily be handled case-by-case as needed in the callers onResize, as well.

> nit: capitalize constants so its more obvious below when reading the tests

These aren't constants, as they are derived from the rect.

> Note: our current splitters often use a px value for the right pane and then flex the remaining space for the left pane.

We could easily add a min{Start,Left}Width and min{End,Right}Width props that have sane defaults. Then you could get the desired effect of a fixed sized right pane with a flex left pane by doing

  HSplitBox({
    leftWidth: 1,
    minRightWidth: "100px",
    // ...
  })

To take a step back though: I don't want to block bug 1149385 on the Perfect Generic Split Box Component. If this isn't generic enough, I am happy to keep it within devtools/client/memory/components and let someone else do the follow ups to take it over the finish line and into devtools/client/shared/components. Please keep this in mind when considering what is or is not needed for this component to land so I can ship my other bug.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #9)
> To take a step back though: I don't want to block bug 1149385 on the Perfect
> Generic Split Box Component. If this isn't generic enough, I am happy to
> keep it within devtools/client/memory/components and let someone else do the
> follow ups to take it over the finish line and into
> devtools/client/shared/components. Please keep this in mind when considering
> what is or is not needed for this component to land so I can ship my other
> bug.

Of course - I don't think figuring out a vertical mode is needed to land this, for instance.  But I hope for the sake of our memory tool users it hits some UX OK-ness before shipping it in any directory.

1) Specifically the "sticky" problem described in Comment 5.  I tried to reproduce locally in test_HSplitBox_01.html but for some reason I was unable to do any manual resizing at all in that test case.  Seeing these errors in the console - unsure if they are related:
* Warning: render(): Rendering components directly into document.body is discouraged, since its children are often manipulated by third-party scripts and browser extensions. This may lead to subtle reconciliation issues. Try rendering into a container element created for your app. react-dev.js:20749:9
* Warning: setProps(...) and replaceProps(...) are deprecated. Instead, call render again at the top level. react-dev.js:20749:9

2) Also, can you make the splitter handle wider than 1px so it's more accessible to grab?  We do this with the other splitters with some CSS like so: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/splitters.css#23-26

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Add a resizable split pane component to devtools/client/shared/components

Rename "left" -> "start" and "right" -> "end".

Carrying jsantell's r+.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> I don't think figuring out a vertical mode is needed to land
> this, for instance. 

Agreed.

> But I hope for the sake of our memory tool users it
> hits some UX OK-ness before shipping it in any directory.

I definitely agree that there is a minimum level of user experience we need to have to ship something!

I think we easily meet that level right now for the memory tool's use in particular.

> 1) Specifically the "sticky" problem described in Comment 5.  I tried to
> reproduce locally in test_HSplitBox_01.html but for some reason I was unable
> to do any manual resizing at all in that test case. 

I did see similar issues when originally I had the onMouseUp bound to the splitter, but when I moved it to the container I never saw the issue again. I tried to repro on the memory tool and couldn't.

I could move the listener to the window, if that would make you happier.

> Seeing these errors in
> the console - unsure if they are related:

These are unrelated warnings about things you shouldn't do in a production app, but this is jsut a test. I don't think it matters. All the other tests also render directly to the body and use setProps as well.

> 2) Also, can you make the splitter handle wider than 1px so it's more
> accessible to grab?  We do this with the other splitters with some CSS like
> so:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/
> splitters.css#23-26

This is great!

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Add a resizable split pane component to devtools/client/shared/components

Fix the "sticky" issue by binding mouse up and mouse move events to the top window.

Make the splitter secretly have 3px width so it is easier to grab.

Comment 14

[Lin Clark [:linclark]]

Comment on attachment 8721092 [details] [diff] [review]
Add a resizable split pane component to devtools/client/shared/components

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

I'm happy to land this and iterate on it if we need to.

I did notice a couple things while manually testing, though:

- You can move the splitter all the way to the right side and then you can't move it anymore.

- When it's side pinned, the horizontal split makes things hard to see. IIRC, the split panes we have with xul switch the splitter from vertical to horizontal when the toolbox is side pinned.

Comment 15

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8721092 [details] [diff] [review]
Add a resizable split pane component to devtools/client/shared/components

This looks good to me. (Looks exactly the same visually, which I'm sure was the intention.)

Was there anything you specifically wanted me to look at, Lin?

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Lin Clark [:linclark] from comment #14)
> Comment on attachment 8721092 [details] [diff] [review]
> Add a resizable split pane component to devtools/client/shared/components
> 
> Review of attachment 8721092 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm happy to land this and iterate on it if we need to.
> 
> I did notice a couple things while manually testing, though:
> 
> - You can move the splitter all the way to the right side and then you can't
> move it anymore.

Will attach a new version that adds min{Start,End}Width props (with sane defaults) so that one can prevent this scenario happening.

> - When it's side pinned, the horizontal split makes things hard to see.
> IIRC, the split panes we have with xul switch the splitter from vertical to
> horizontal when the toolbox is side pinned.

A ResponsiveSplitBox component could certainly be built on top of this component and a VSplitBox component.

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Add a resizable split pane component to devtools/client/shared/components

This commit adds HSplitBox which gives two panes aligned horizontally and a
splitter between them to resize their relative widths as needed.

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=496a24fccd60

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Add a resizable split pane component to devtools/client/shared/components

Fix the test that broke when we moved the mouse move handler to the top window.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7f0f90bcd1f

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'm hitting conflicts applying this patch. Can we get a one rebased onto fx-team tip?

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/923f6b1493b9

Comment 22

[Lina Butler [:lina]]

https://hg.mozilla.org/mozilla-central/rev/923f6b1493b9