Closed Bug 1465644 Opened Last year Closed Last year

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

Categories

(DevTools :: General, enhancement, P3)

enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: victoria, Assigned: mantaroh)

Details

Attachments

(5 files, 4 obsolete files)

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
Summary: Add draggable splitter bars between the tools in vertical mode → Add draggable splitter bars between the panes in side-docked mode
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.
Product: Firefox → DevTools
(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?
Flags: needinfo?(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 :)
Flags: needinfo?(victoria) → needinfo?(nchevobbe)
Severity: normal → enhancement
Priority: -- → P3
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).
Flags: needinfo?(nchevobbe)
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.)
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.
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.
Attached image 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?
Flags: needinfo?(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.
Flags: needinfo?(victoria)
(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
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
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
Attachment #8995048 - Flags: review?(nchevobbe) → review+
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 ?
Attachment #8995049 - Flags: review?(nchevobbe) → review+
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.
(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?
Flags: needinfo?(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?
Flags: needinfo?(victoria)
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
Flags: needinfo?(victoria)
Attached image 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.
Flags: needinfo?(victoria)
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.
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.
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.
Attachment #8999794 - Flags: review+
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 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.
Attachment #8999796 - Flags: review+
Attachment #8995048 - Attachment is obsolete: true
Attachment #8995049 - Attachment is obsolete: true
Attachment #8999794 - Attachment is obsolete: true
Attachment #8999795 - Attachment is obsolete: true
(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 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.
Attachment #9003041 - Flags: review+
Comment on attachment 9003042 [details]
Bug 1465644 - Part 2. Introduce the emphasized splitter style. r?nchevobbe

Nicolas Chevobbe [:nchevobbe] has approved the revision.
Attachment #9003042 - Flags: review+
Thank you so much!

I'll land these patch and send PR of devtools-core.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fcef4f55c41b66a1ab97462240cb32239fc340d
Pushed by mantaroh@gmail.com:
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
You need to log in before you can comment on or make changes to this bug.