1465644 - Add draggable splitter bars between the panes in side-docked mode

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Victoria Wang [:victoria]]

When DevTools is docked to side, it's often hard to tell where to click to resize the panes, especially when there are 3 panes due to split console. Adding 4px tall splitter bars as shown in this mockup should make this easier to use.

https://mozilla.invisionapp.com/share/WRJU9XMY9U7#/screens

Light mode color: Gray 30, Gray 40 on hover/focus
Dark mode color: Gray 70, Gray 60 on hover/focus

Comment 1

[Victoria Wang [:victoria]]

I worked on this a little more to improve the visuals and I realized we can actually just do 2px-tall bars for a subtler effect that still gets the message across. (Updated the above mockup link.) 

For this revision, we should change the dark mode colors to Gray 60, Gray 50 on hover/focus.

Comment 2

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Victoria Wang [:victoria] from comment #1)
> I worked on this a little more to improve the visuals and I realized we can
> actually just do 2px-tall bars for a subtler effect that still gets the
> message across. (Updated the above mockup link.) 
> 
> For this revision, we should change the dark mode colors to Gray 60, Gray 50
> on hover/focus.

Hi Victoria,

As discussed with you in the all hands, I've created the package of 2px and 3px version for this.
The following link is that. (This is rough experimental implemetation, so I confirmed only the 3 pane of inspector and split console.)

3 px : https://queue.taskcluster.net/v1/task/LFGdEa9BRg-S3IgGinLHOQ/runs/0/artifacts/public/build/target.dmg
2 px : https://queue.taskcluster.net/v1/task/GQn8Wk-PTk-kI3cBoEIZlQ/runs/0/artifacts/public/build/target.dmg

Could you please confirm this behavior of these packages?

Comment 3

[Victoria Wang [:victoria]]

Awesome - thanks for these builds!

I like 2px a bit better because it looks more sleek and still gives the needed sense of separation, but it is a very tiny click/hover target that's easy to miss. Two things I'd love to try out:

1. The splitter could visually appear to be 2px tall, but with a click/hover activation target of 6px or more (by using a transparent border or something?).

2. More of the 'whitespace' in the toolbars could be draggable to give really large targets - for example in split Console, the space between "Filter Output" and "Persist Logs." For the splitter between the two Inspector halves, the unclickable parts of the breadcrumbs bar could be draggable. This is similar to Chrome's draggable tab bars (the space after the last tab is draggable).

^ Either way it would be really nice to also fix the split Console resize behavior so that it only affects the second two rows and not all three rows.

Needinfoing Nicolas in case he has thoughts on split console resizing or these builds in general :)

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

I agree with everything in Comment 3 :) 
Having a larger target for the split console would be really neat. However, we should be careful when implementing it that it doesn't "override" other hit-boxes (i'm thinking of the inspector breadcrumb for example, when the split console is enabled with the inspector).

Comment 5

[Victoria Wang [:victoria]]

Expanding the hit targets to all whitespace sounds like it may require some tricky development/experimentation/testing, so it seems to me that landing this patch as it is with the 2px splitters would be a great as it's already a big improvement. 

(Or if it will be simple enough, including the addition of the small amount of extra hit target via simple transparent 2-3px border on each side of the splitter would be great as well.)

Comment 6

[Mantaroh Yoshinaga[:mantaroh]]

Thanks, victoria, nicolas.

Sorry for my late reply.

As a first step, I tried that I change the all of splitter to 2px. You can confirm this behavior with following object.[1]

[1] https://queue.taskcluster.net/v1/task/BqIU1C0fRKGxuLYBeM0KSg/runs/0/artifacts/public/build/target.dmg

(In reply to Victoria Wang [:victoria] from comment #3)
> 2. More of the 'whitespace' in the toolbars could be draggable to give
> really large targets - for example in split Console, the space between
> "Filter Output" and "Persist Logs." For the splitter between the two
> Inspector halves, the unclickable parts of the breadcrumbs bar could be
> draggable. This is similar to Chrome's draggable tab bars (the space after
> the last tab is draggable).
> 

OK. I'm going to try this experimental implementation. Maybe, we need to add the logic into toolbar.js in order to resize the iframe window.

Comment 7

[Mantaroh Yoshinaga[:mantaroh]]

I'm back to this bug.

I tried implementing the draggable white space. Then I think that we need the following changes:
  * Wrap the iframe of "devtools-toolbox-bottom-iframe" by using "hbox" element in order to resize.
  * Add messaging between browser.xul and toolbox.xul, sending resize height value.

I'd like to separate this bug into the changing the splitter widht and implementing the draggable element.

Comment 8

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: draggable-bar.png

Hi victoria,

It's diffcult to find the draggable area, at the moment, current draggable splitter has 6px draggable area. This attachment is coloring this area. The horizontal splitter has 2px top border and 2px bottom border and own border width is 2px, so total draggable area is 6px.
The vertical splitter has 1px left border and 4px right border and own border width is 2px, so total draggable area is 5px. (The reason of different of vertical size, prevent to overlap the scroll bar.)

According to comment 3, Do we need to expand this area more?

Comment 9

[Victoria Wang [:victoria]]

Hi Mantaroh! Sorry for the delay in responding. 

I tried out the build and it's looking great!

- I agree with making a separate bug for implementing fancier draggable areas. That feature seems lower priority than getting this bug landed.
- Only the internal horizontal lines need the visual change of 2px plus hover styling - the main DevTools border and all vertical lines are clear enough as is (since there aren't any extra vertical lines that visually compete with them).
- RE: invisible drag border - I feel like we could add an extra 1px to the sides of every splitter and it would still be pretty safe. This may make sense for both the horizontal and vertical borders - I'd love to have a build to try that out.

Comment 10

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Victoria Wang [:victoria] from comment #9)
> Hi Mantaroh! Sorry for the delay in responding. 
> 
> I tried out the build and it's looking great!
> 
> - I agree with making a separate bug for implementing fancier draggable
> areas. That feature seems lower priority than getting this bug landed.
> - Only the internal horizontal lines need the visual change of 2px plus
> hover styling - the main DevTools border and all vertical lines are clear
> enough as is (since there aren't any extra vertical lines that visually
> compete with them).
> - RE: invisible drag border - I feel like we could add an extra 1px to the
> sides of every splitter and it would still be pretty safe. This may make
> sense for both the horizontal and vertical borders - I'd love to have a
> build to try that out.

Thanks!

I updated the test target of this change:
https://queue.taskcluster.net/v1/task/CRC7AfETTXOhI4SmCrpy6g/runs/0/artifacts/public/build/target.dmg

Comment 11

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1465644 - Part 1. Change characode of SplitConsole.css from dos to unix.

Review commit: https://reviewboard.mozilla.org/r/259526/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259526/

Comment 12

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1465644 - Part 2. Introduce the emphasized splitter style.

This patch will introduce the emphasized splitter style to the horizontal
splitter of devtools. (Only the SplitterBox and the splitter of the split
console)

Review commit: https://reviewboard.mozilla.org/r/259528/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/259528/

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8995048 [details]
Bug 1465644 - Part 1. Change characode of SplitConsole.css from dos to unix.

https://reviewboard.mozilla.org/r/259526/#review266572

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8995049 [details]
Bug 1465644 - Part 2. Introduce the emphasized splitter style.

https://reviewboard.mozilla.org/r/259528/#review266574

it's working nicely, thanks Mantaroh.
I kinda want the hover style to be applied to all splitters because it feels nice, but I defer this to Victoria (and we can do it in a follow up)

::: devtools/client/debugger/new/dist/debugger.css:524
(Diff revision 1)
>       it clickable on both sides. */
>    z-index: 1;
>  }
>  
>  .split-box.vert > .splitter {
> -  min-width: calc(var(--devtools-splitter-inline-start-width) +
> +  min-width: var(--devtools-vertical-splitter-min-width);

the original code is in https://github.com/devtools-html/devtools-core/blob/ccdd73596e99e28fa4ed3b61700e28e9986d3893/packages/devtools-splitter/src/SplitBox.css#L50-L52 
that's fine overriding it here, but we should make sure to backport those changes to github so this doesn't get overidden (this file is generated by a build process)

::: devtools/client/debugger/new/dist/debugger.css:536
(Diff revision 1)
>  
>    cursor: ew-resize;
>  }
>  
>  .split-box.horz > .splitter {
> -  min-height: calc(var(--devtools-splitter-top-width) +
> +  min-height: var(--devtools-horizontal-splitter-min-height);

the original code is in https://github.com/devtools-html/devtools-core/blob/ccdd73596e99e28fa4ed3b61700e28e9986d3893/packages/devtools-splitter/src/SplitBox.css#L63-L65 
that's fine overriding it here, but we should make sure to backport those changes to github so this doesn't get overidden (this file is generated by a build process)

::: devtools/client/themes/splitters.css:76
(Diff revision 1)
>    margin-bottom: calc(-1 * var(--devtools-splitter-bottom-width));
>  
>    cursor: ns-resize;
>  }
>  
> +#toolbox-deck + splitter.devtools-horizontal-splitter {

i guess this could be dangerous if some element is introduced between the 2 at some point ?
Could we use ~ instead of + maybe to reduce the chances of being bitten by this ?

Comment 15

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8995049 [details]
Bug 1465644 - Part 2. Introduce the emphasized splitter style.

https://reviewboard.mozilla.org/r/259528/#review266574

Thank you for review.

I tried to updat the target object by using this styl to all splitters:
https://queue.taskcluster.net/v1/task/KKK7XG8dTY2Rt8-8a0pS3g/runs/0/artifacts/public/build/target.dmg

(try-push: https://hg.mozilla.org/try/rev/2a4bdee38d1de576127bfb943f2a0129a4e14d8a)

> the original code is in https://github.com/devtools-html/devtools-core/blob/ccdd73596e99e28fa4ed3b61700e28e9986d3893/packages/devtools-splitter/src/SplitBox.css#L50-L52 
> that's fine overriding it here, but we should make sure to backport those changes to github so this doesn't get overidden (this file is generated by a build process)

OK! I'll send PR to devtools-core.

Comment 16

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #15)
> Comment on attachment 8995049 [details]
> Bug 1465644 - Part 2. Introduce the emphasized splitter style.
> 
> https://reviewboard.mozilla.org/r/259528/#review266574
> 
> Thank you for review.
> 
> I tried to updat the target object by using this styl to all splitters:
> https://queue.taskcluster.net/v1/task/KKK7XG8dTY2Rt8-8a0pS3g/runs/0/
> artifacts/public/build/target.dmg
> 
> (try-push:
> https://hg.mozilla.org/try/rev/2a4bdee38d1de576127bfb943f2a0129a4e14d8a)
> 
> > the original code is in https://github.com/devtools-html/devtools-core/blob/ccdd73596e99e28fa4ed3b61700e28e9986d3893/packages/devtools-splitter/src/SplitBox.css#L50-L52 
> > that's fine overriding it here, but we should make sure to backport those changes to github so this doesn't get overidden (this file is generated by a build process)
> 
> OK! I'll send PR to devtools-core.

Victoria,

I try to apply this color style to all splitters(excepting the devtool window splitter). How you do you feel about it?

Comment 17

[Victoria Wang [:victoria]]

Finally took a look at this - it's looking really good! 

A few tiny things re: vertical mode with light theme:
Debugger - looks like it may be missing the thick splitter between its two panels
Network - when 'sidebar' with response header info is not visible, the splitter between the table view and the split console looks 1px too wide

Re: Dark theme, looks like you're waiting to work on this after the light theme version is done?

Comment 18

[Mantaroh Yoshinaga[:mantaroh]]

Victoria,

Sorry for my late reply, and dark theme is my mistake. I specified wrong css variable name. ("grye-60").

Debugger uses SplitBox of shared component. As mentioned Nicolas, this code is maintained on Github. So I'll fix later via github.

(In reply to Victoria Wang [:victoria] from comment #17)
> Network - when 'sidebar' with response header info is not visible, the
> splitter between the table view and the split console looks 1px too wide

You mean that the label height of "No headers for this request" is little narrow, right?
We specified this split box size to 50px, so this label is not fit in this height.[1] 

[1] https://searchfox.org/mozilla-central/rev/0f4d50ff5211e8dd85e119ef683d04b63062ed90/devtools/client/netmonitor/src/components/MonitorPanel.js#150

Comment 19

[Victoria Wang [:victoria]]

Attachment: extraborder.png

> You mean that the label height of "No headers for this request" is little narrow, right?
Hi Mantaroh! I'm actually talking about the line in this image. In Netmonitor only, there's an extra border on top of the regular splitter that has the hover effect but doesn't do anything.

Comment 20

[Mantaroh Yoshinaga[:mantaroh]]

Thanks! Victoria.

(In reply to Victoria Wang [:victoria] from comment #19)
> Created attachment 8999035 [details]
> extraborder.png
> 
> > You mean that the label height of "No headers for this request" is little narrow, right?
> Hi Mantaroh! I'm actually talking about the line in this image. In
> Netmonitor only, there's an extra border on top of the regular splitter that
> has the hover effect but doesn't do anything.

I got it. This is the splitter of netmonitor. I think that we need hide the split if network detail has opened.

Comment 21

[Mantaroh Yoshinaga[:mantaroh]]

Sorry, s/has opened/has NOT opened/. 


I tried setting the split box size to zero if the network detail has not opened.
Like the following:
https://hg.mozilla.org/try/rev/3a2080b3a3009ab83c90de47bd13a12c0c8f5040

It seems to me that this patch works well.

Comment 22

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1465644 - Part 1. Change characode of SplitConsole.css from dos to unix.

Oops, I couldn't update the patch of the review board. So I attached the patch of part 1 and part 2.
I'll land these patches to the m-i after addressing the problem of comment 21.

Comment 23

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1465644 - Part 2. Introduce the emphasized splitter style.

Comment 24

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1465644 - Part 3. Hide Splitter if the network panel detail has not opened. r?honza

Network monitor has the splitter box. This splitter is displayed between the
request list panel and network details panel. However, this splitter is
displayed even if the network details panel has not displayed.

This patch will set the spliter size to zero if the network details panel
is collapsed.

Comment 25

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

Comment on attachment 8999796 [details]
Bug 1465644 - Part 3. Hide Splitter if the network panel detail has not opened. r?honza

Jan Honza Odvarko [:Honza] has approved the revision.

Comment 26

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1465644 - Part 1. Change characode of SplitConsole.css from dos to unix. r?nchevobbe

Comment 27

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1465644 - Part 2. Introduce the emphasized splitter style. r?nchevobbe

Comment 28

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #27)
> Created attachment 9003042 [details]
> Bug 1465644 - Part 2. Introduce the emphasized splitter style. r?nchevobbe

I modified the test of this patch to prevent related test failure.

Comment 29

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9003041 [details]
Bug 1465644 - Part 1. Change characode of SplitConsole.css from dos to unix. r?nchevobbe

Nicolas Chevobbe [:nchevobbe] has approved the revision.

Comment 30

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9003042 [details]
Bug 1465644 - Part 2. Introduce the emphasized splitter style. r?nchevobbe

Nicolas Chevobbe [:nchevobbe] has approved the revision.

Comment 31

[Mantaroh Yoshinaga[:mantaroh]]

Thank you so much!

I'll land these patch and send PR of devtools-core.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fcef4f55c41b66a1ab97462240cb32239fc340d

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1296c1105fb
Part 1. Change characode of SplitConsole.css from dos to unix. r=nchevobbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ff5e06dccdc
Part 2. Introduce the emphasized splitter style. r=nchevobbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd86a6388333
Part 3. Hide Splitter if the network panel detail has not opened. r=honza

Comment 33

[Mantaroh Yoshinaga[:mantaroh]]

PR:
https://github.com/devtools-html/devtools-core/pull/1082

Comment 34

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e1296c1105fb
https://hg.mozilla.org/mozilla-central/rev/6ff5e06dccdc
https://hg.mozilla.org/mozilla-central/rev/fd86a6388333