Closed Bug 1258114 Opened 8 years ago Closed 8 years ago

Inspector pane too hard to resize (developer tools usability)

Categories

(DevTools :: Framework, defect, P3)

43 Branch
x86_64
Linux
defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: fabiosantosart, Assigned: jdescottes)

References

Details

(Whiteboard: [btpp-backlog])

Attachments

(4 files, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151216175450

Steps to reproduce:

1. Open the developer tools as a pane (IE: not in a separate window)
2. Hover the line that splits the tools and the web page with the mouse, observing where it can be resized (the cursor changes to a vertical resize indicator)


Actual results:

To resize I have to put my mouse in a 4px tall active area. This makes it really hard to point mouse cursor there, especially with a trackpad, thus making it hard to resize the inspector. Compare with Firebug, which has a thick draggable affordance, or with the Midori developer toolbar, which has a very thick draggable area (despite the thinner visible line).


Expected results:

This area should be taller, so that it's feasible to point to with a trackpad on my first try.
Component: Untriaged → Developer Tools
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
:helenvholmes, any thoughts about this?
Component: Developer Tools → Developer Tools: Framework
Flags: needinfo?(hholmes)
Priority: -- → P3
Whiteboard: [btpp-backlog]
I'd be all right with increasing the draggable area to be larger (maybe 8px?) but I suspect Brian's a better person to ask to make sure we're not overlooking anything by increasing the draggable area.
Flags: needinfo?(hholmes) → needinfo?(bgrinstead)
On Chrome, the whole tab strip acts as a toolbox resizer handle.
(In reply to Tim Nguyen [:ntim] from comment #4)
> On Chrome, the whole tab strip acts as a toolbox resizer handle.

That's Bug 1056543
See Also: → 1056543
So yes, these numbers could be increased from 3 to something else: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/splitters.css#21-23.  Likewise for .devtools-side-splitter: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/splitters.css#33-35.

Two things to note:

1) This will also affect splitter within the tools (i.e. between the markup view and rule view in the inspector, or between split console and selected tool).
2) The space will be added on "top" of the splitter, not overlapping the toolbox as in attachment 8732525 [details].  That's because it's a xul:splitter that's in a different document and is above the toolbox.  Because of that I'd recommend an increase to something like 5px so that it's not too far away from the actual line drawn by the splitter (if it was overlapping top/bottom than an 8px grip area would be at most 4px away from the drawn line on either end).
Flags: needinfo?(bgrinstead)
(In reply to Tim Nguyen [:ntim] from comment #4)
> On Chrome, the whole tab strip acts as a toolbox resizer handle.

I'd like to add that while this is quite an excellent help resizing the chrome tools, the problem persists if the tools are to the right of the web page in a vertical split (the toolbar remains on top)
Taking the bug, since I was working on a duplicate!
Assignee: nobody → jdescottes
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch bug1258114.proto.patch (obsolete) — Splinter Review
(In reply to Brian Grinstead [:bgrins] from comment #6)
> 2) The space will be added on "top" of the splitter, not overlapping the
> toolbox as in attachment 8732525 [details].  That's because it's a
> xul:splitter that's in a different document and is above the toolbox. 
> Because of that I'd recommend an increase to something like 5px so that it's
> not too far away from the actual line drawn by the splitter (if it was
> overlapping top/bottom than an 8px grip area would be at most 4px away from
> the drawn line on either end).

With some tweaking we can actually achieve the same. Most of the time (always), the splitters are in the same document as the splitted elements.

Brian: do you have any example in mind where the attached patch would not work?
Flags: needinfo?(bgrinstead)
Comment on attachment 8739027 [details] [diff] [review]
bug1258114.proto.patch

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

Leaving a couple of notes

::: devtools/client/themes/splitters.css
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /* Splitters */

While you are here, can you add a comment to the top of the file saying that this is loaded by common.css (which is loaded in both the browser.xul frame and toolbox frames).

This explains why we need copy/pasted variables and is a warning for not making any changes that rely on the toolbox being present.

@@ +23,5 @@
> +  border-color: transparent;
> +  background-color: var(--devtools-splitter-bordercolor);
> +  background-clip: content-box;
> +
> +  z-index: 10;

What's this z-index needed?  Looks like it wasn't there before.

::: devtools/client/themes/variables.css
@@ +24,5 @@
>    --theme-selection-background: #4c9ed9;
>    --theme-selection-background-semitransparent: rgba(76, 158, 217, 0.15);
>    --theme-selection-color: #f5f7fa;
>    --theme-splitter-color: #dde1e4;
> +  --theme-splitter-bordercolor: #bbbbbb;

What's the distinction btw splitter-color and splitter-bordercolor (it looks like the names are mixed up and splitter-bordercolor is set on the background)
(In reply to Julian Descottes [:jdescottes] from comment #9)
> Created attachment 8739027 [details] [diff] [review]
> bug1258114.proto.patch
> 
> (In reply to Brian Grinstead [:bgrins] from comment #6)
> > 2) The space will be added on "top" of the splitter, not overlapping the
> > toolbox as in attachment 8732525 [details].  That's because it's a
> > xul:splitter that's in a different document and is above the toolbox. 
> > Because of that I'd recommend an increase to something like 5px so that it's
> > not too far away from the actual line drawn by the splitter (if it was
> > overlapping top/bottom than an 8px grip area would be at most 4px away from
> > the drawn line on either end).
> 
> With some tweaking we can actually achieve the same. Most of the time
> (always), the splitters are in the same document as the splitted elements.
> 
> Brian: do you have any example in mind where the attached patch would not
> work?

Seems to work fine, even when splitting between toolbox and rest of the browser.  I'd suggest to make the hit area a bit smaller since it carries over nearly onto the text of the toolbox tabs
Flags: needinfo?(bgrinstead)
Also, I don't think we need to introduce a new color here.  Increasing the hit area would be great, and then we could discuss color changes in another bug.
Thanks for having a look!

(In reply to Brian Grinstead [:bgrins] from comment #11)
> Comment on attachment 8739027 [details] [diff] [review]
> bug1258114.proto.patch
> 
> Review of attachment 8739027 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Leaving a couple of notes
> 
> ::: devtools/client/themes/splitters.css
> @@ +1,5 @@
> >  /* This Source Code Form is subject to the terms of the Mozilla Public
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  /* Splitters */
> 
> While you are here, can you add a comment to the top of the file saying that
> this is loaded by common.css (which is loaded in both the browser.xul frame
> and toolbox frames).
> 
> This explains why we need copy/pasted variables and is a warning for not
> making any changes that rely on the toolbox being present.
> 
> @@ +23,5 @@
> > +  border-color: transparent;
> > +  background-color: var(--devtools-splitter-bordercolor);
> > +  background-clip: content-box;
> > +
> > +  z-index: 10;
> 
> What's this z-index needed?  Looks like it wasn't there before.

The layout is as follows:
<element1>
<splitter>
<element2>

Having a z-index moves the invisible bottom-border on top of <element2>, which allows to drag the splitter whether your mouse is above or below the splitter. Now the value "10" is totally random, I was really just trying things out.

> 
> ::: devtools/client/themes/variables.css
> @@ +24,5 @@
> >    --theme-selection-background: #4c9ed9;
> >    --theme-selection-background-semitransparent: rgba(76, 158, 217, 0.15);
> >    --theme-selection-color: #f5f7fa;
> >    --theme-splitter-color: #dde1e4;
> > +  --theme-splitter-bordercolor: #bbbbbb;
> 
> What's the distinction btw splitter-color and splitter-bordercolor (it looks
> like the names are mixed up and splitter-bordercolor is set on the
> background)

Really sorry about that, as I mentioned on IRC, I mixed a couple of splitter-related bugs. This was an attempt to change the splitter color (inspired by Bug 1262668). All the color related changes should be removed.
While working on this, you can accidentally improve or worsen bug 1171449, bug 1197602.
(In reply to arni2033 from comment #15)
> While working on this, you can accidentally improve or worsen bug 1171449,
> bug 1197602.

Thanks for the pointers!

For Bug 1171449, I logged a duplicate earlier : Bug 1262748. I intend to fix both at the same time, because as you said increasing the resize handle area might make Bug 1171449 trigger more frequently.
I have a patch ready for this one as well.

Regarding 1197602, my changes don't seem to have any impact, positive or negative.
(In reply to Brian Grinstead [:bgrins] from comment #13)
> Also, I don't think we need to introduce a new color here.  Increasing the
> hit area would be great, and then we could discuss color changes in another
> bug.

I would prefer it if we left the splitter color as-is. IIRC borders and splitters were the same color before as well; either way, I would propose that we use left + right borders with a white or a transparent background to increase the hit area of the splitters to make them more selectable.
One more thing.
Please make sure that dragging vertical scrollbar in dark theme won't be affected much. If splitters
will have wide invisible area, that will significantly reduce draggable area of scrollbar thumb.

There're already plenty of scrollbar issues, as if the goal was to create the most unusable tool...
(don't read it if you already know how awful floating scrollbars are)
 1) It's hard to see if text is selected (bug 1100622, bug 1251753)
 2) It's not possible to scroll page a bit to the down/up, because there're no scrollbuttons.
    This is essential for mouse option "When I rotate mouse wheel" -> "Scroll by 1 page"
 3) Chain of nested scrollable elements can't be scrolled by dnd (bug 1197229)
 4) Scrollbar has no minimal width/height, so it's hard to hit scrollbar thumb 8px * 8px
 5) Scrollbar overlaps "close" buttons in Debugger -> Variables, so it's hard to hit them (same as 3)
 6) Area reserved for performing Shift+Click in scrollbar is unclear
Attached patch bug1258114.wip.patch (obsolete) — Splinter Review
(In reply to arni2033 from comment #18)
> One more thing.
> Please make sure that dragging vertical scrollbar in dark theme won't be
> affected much. If splitters
> will have wide invisible area, that will significantly reduce draggable area
> of scrollbar thumb.
> 
> There're already plenty of scrollbar issues, as if the goal was to create
> the most unusable tool...
> (don't read it if you already know how awful floating scrollbars are)
>  1) It's hard to see if text is selected (bug 1100622, bug 1251753)
>  2) It's not possible to scroll page a bit to the down/up, because there're
> no scrollbuttons.
>     This is essential for mouse option "When I rotate mouse wheel" ->
> "Scroll by 1 page"
>  3) Chain of nested scrollable elements can't be scrolled by dnd (bug
> 1197229)
>  4) Scrollbar has no minimal width/height, so it's hard to hit scrollbar
> thumb 8px * 8px
>  5) Scrollbar overlaps "close" buttons in Debugger -> Variables, so it's
> hard to hit them (same as 3)
>  6) Area reserved for performing Shift+Click in scrollbar is unclear

Thanks for listing all this! Right now vertical splitters only have a draggable area on the left of the splitter, horizontal splitter only have a draggable area on top of the splitter. My patch will increase the overall draggable area by making the other side draggable as well. Right now we have 2 + 1 draggable pixels, we will have 2 + 1 + 2.

It should not make any of the use cases you mentioned worst. We need to make sure it doesn't overlap with controls on the other side of the splitter of course. I can't think of any panel impacted because of this but let me know if you have any concern (you can apply the attached patch if you want to try it).
Attachment #8739027 - Attachment is obsolete: true
Devtools splitters now have a clickable area on both sides of the splitter.
The previous layout provided a 3px clickable area (2px empty content + 1px
visible border).

The new layout uses invisible borders on both sides and a visible (clipped)
content. This patch still uses 2px on each side, the overall clickable area
becomes 5px : 2px + 1px + 2px.

The base splitter width is now set in a variable defined in splitters.css

Review commit: https://reviewboard.mozilla.org/r/45489/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45489/
Attachment #8740016 - Flags: review?(bgrinstead)
Comment on attachment 8740016 [details]
MozReview Request: Bug 1258114 - use invisible borders to drag devtools splitters on both sides;r=bgrins

https://reviewboard.mozilla.org/r/45489/#review42089

I like the approach overall, here's a couple of notes:

1) so, there's a usage in the memory tool of a class called .h-split-box-splitter that's sort of like .devtools-side-splitter, but not quite.  I think we should switch the class there (and remove the h-split-box-splitter CSS) so it's consistent. But, when I switch that code locally, the background becomes 5px wide.  Possibly because it's a CSS flex child and not XUL flex, but hopefully the styling here could work for both (if possible).  This makes me think that we should be setting width to 1px (which should still be draggable for 5px b/c the border should be draggable).

2) it seems to me on the inspector, for example, that the draggable area doesn't start until the right side of the splitter, and extends 5px to the left (so well into the scrollbar area).  Can you reproduce this or is it centered horizontally for you?

::: devtools/client/themes/splitters.css:42
(Diff revision 1)
> +  /* it clickable on both sides. */
> +  z-index: 1;
> +}
> +
> +.devtools-horizontal-splitter {
> +  -moz-margin-start: 0;

Replace with margin-inline-start, if it's neeed
Attachment #8740016 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #21)
> Comment on attachment 8740016 [details]
> MozReview Request: Bug 1258114 - devtools splitters: increase draggable
> area;r=bgrins
> 
> https://reviewboard.mozilla.org/r/45489/#review42089
> 
> I like the approach overall, here's a couple of notes:
> 
> 1) so, there's a usage in the memory tool of a class called
> .h-split-box-splitter that's sort of like .devtools-side-splitter, but not
> quite.  I think we should switch the class there (and remove the
> h-split-box-splitter CSS) so it's consistent. But, when I switch that code
> locally, the background becomes 5px wide.  Possibly because it's a CSS flex
> child and not XUL flex, but hopefully the styling here could work for both
> (if possible).  This makes me think that we should be setting width to 1px
> (which should still be draggable for 5px b/c the border should be draggable).
> 

Thanks for spotting this one, I'll have a look.

> 2) it seems to me on the inspector, for example, that the draggable area
> doesn't start until the right side of the splitter, and extends 5px to the
> left (so well into the scrollbar area).  Can you reproduce this or is it
> centered horizontally for you?
> 

It is centered for me. The area remains small so it's kind of hard to tell. For sure on the left side it stops at the exact same pixel as on Nightly, we just have 2 more pixels on the right. Are you testing on OSX? (I would have been in favor of bumping the area to 3px on each side of the splitter, but for all the reasons mentioned in previous comments, I prefer to be conservative for now).

> ::: devtools/client/themes/splitters.css:42
> (Diff revision 1)
> > +  /* it clickable on both sides. */
> > +  z-index: 1;
> > +}
> > +
> > +.devtools-horizontal-splitter {
> > +  -moz-margin-start: 0;
> 
> Replace with margin-inline-start, if it's neeed

Thanks!

============

I have spotted a display issue with this new style in the debugger view. I didn't see it before but I'll try to fix this before resubmitting for review.
Comment on attachment 8740016 [details]
MozReview Request: Bug 1258114 - use invisible borders to drag devtools splitters on both sides;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45489/diff/1-2/
Attachment #8740016 - Flags: review?(bgrinstead)
(In reply to Julian Descottes [:jdescottes] from comment #22)
> > 2) it seems to me on the inspector, for example, that the draggable area
> > doesn't start until the right side of the splitter, and extends 5px to the
> > left (so well into the scrollbar area).  Can you reproduce this or is it
> > centered horizontally for you?
> > 
> 
> It is centered for me. The area remains small so it's kind of hard to tell.
> For sure on the left side it stops at the exact same pixel as on Nightly, we
> just have 2 more pixels on the right. Are you testing on OSX? (I would have
> been in favor of bumping the area to 3px on each side of the splitter, but
> for all the reasons mentioned in previous comments, I prefer to be
> conservative for now).

You might have to flip the "Show scroll bars" option in system preferences to "Always" and check on light theme.  This should be closer to Windows behavior (which I can check on tomorrow with the patch applied).
(In reply to Brian Grinstead [:bgrins] from comment #24)
> (In reply to Julian Descottes [:jdescottes] from comment #22)
> > > 2) it seems to me on the inspector, for example, that the draggable area
> > > doesn't start until the right side of the splitter, and extends 5px to the
> > > left (so well into the scrollbar area).  Can you reproduce this or is it
> > > centered horizontally for you?
> > > 
> > 
> > It is centered for me. The area remains small so it's kind of hard to tell.
> > For sure on the left side it stops at the exact same pixel as on Nightly, we
> > just have 2 more pixels on the right. Are you testing on OSX? (I would have
> > been in favor of bumping the area to 3px on each side of the splitter, but
> > for all the reasons mentioned in previous comments, I prefer to be
> > conservative for now).
> 
> You might have to flip the "Show scroll bars" option in system preferences
> to "Always" and check on light theme.  This should be closer to Windows
> behavior (which I can check on tomorrow with the patch applied).

Just tried this, and still no difference between my patch and Nightly. Attached is the leftmost position of the resize handle for the inspector splitter.

Do you see something different from this screenshot?

Otherwise, I'm wondering if we are not just confused by the cursor centering here. The hotspot of the default cursor (the arrow) is topmost & leftmost black pixel of the cursor image. So it is the 2nd pixel from the left on an 11 pixel-wide cursor, which can feel off-center.

Adding a ni? so that you can take a look and decide if we need to do screensharing here.
Attachment #8739770 - Attachment is obsolete: true
Flags: needinfo?(bgrinstead)
We talked about this and decided it might just be confusing due to cursor centering.  We might want to make the splitter width a bit off center to make it look more visually aligned, but as long as it works well on Windows it should be fine either way.  Julian, did you get a chance to check?  Should I be waiting for a new version of the patch to review?
Flags: needinfo?(bgrinstead) → needinfo?(jdescottes)
Comment on attachment 8740016 [details]
MozReview Request: Bug 1258114 - use invisible borders to drag devtools splitters on both sides;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45489/diff/2-3/
Attachment #8740016 - Attachment description: MozReview Request: Bug 1258114 - devtools splitters: increase draggable area;r=bgrins → MozReview Request: Bug 1258114 - use invisible borders to drag devtools splitters on both sides;r=bgrins
Sorry about the wait, had a few test failures to fix. I tested on Windows, and we have the same behavior as on OSX (light & dark theme).

This patch is using an off-centered version for the side-splitter (1px left / 4px right). Total that's 1px more than the previous patch. I can switch to 1px/3px if you prefer.
Flags: needinfo?(jdescottes)
Comment on attachment 8740016 [details]
MozReview Request: Bug 1258114 - use invisible borders to drag devtools splitters on both sides;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45489/diff/3-4/
Comment on attachment 8740016 [details]
MozReview Request: Bug 1258114 - use invisible borders to drag devtools splitters on both sides;r=bgrins

https://reviewboard.mozilla.org/r/45489/#review43121

works for me, thanks!

::: devtools/client/themes/components-h-split-box.css:27
(Diff revision 4)
>    flex-direction: row;
>    flex: 1;
>  }
>  
> -.h-split-box-splitter {
> -  -moz-border-end: 1px solid var(--theme-splitter-color);
> +.h-split-box .devtools-side-splitter {
> +  box-sizing: border-box;

Why does this need border-box?  Should all splitters have this if they are in the HTML namespace?  If so, should this be added to the shared styles?  OK to wait if unsure.

::: devtools/client/themes/splitters.css:6
(Diff revision 4)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /* Splitters */
> +/* This file is loaded by both browser.xul and toolbox.xul */

Nit: for multiline comments please stick with this format:

/* This file is loaded by both browser.xul and toolbox.xul
   Therefore, rules defined here can not rely on toolbox.xul variables. */
   
Also, please move this comment above the /* splitters */ comment

::: devtools/client/themes/splitters.css:10
(Diff revision 4)
>  /* Splitters */
> +/* This file is loaded by both browser.xul and toolbox.xul */
> +/* Therefore, rules defined here can not rely on toolbox.xul variables. */
> +
> +:root {
> +  /* Define the widths of the draggable areas on each side of a splitter. top */

Same nit as above

::: devtools/client/themes/splitters.css:42
(Diff revision 4)
> -  min-height: 3px;
> -  height: 3px;
> -  margin-top: -3px;
> +  border-color: transparent;
> +  background-color: var(--devtools-splitter-color);
> +  background-clip: content-box;
>    position: relative;
> +
> +  /* Positive z-index positions the splitter on top of its siblings and makes */

Same nit as above
Attachment #8740016 - Flags: review?(bgrinstead) → review+
Comment on attachment 8740016 [details]
MozReview Request: Bug 1258114 - use invisible borders to drag devtools splitters on both sides;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45489/diff/4-5/
Thanks for the review Brian! Applied your suggestions and pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d2388e881b7
Comment on attachment 8740016 [details]
MozReview Request: Bug 1258114 - use invisible borders to drag devtools splitters on both sides;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45489/diff/5-6/
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8758281&repo=fx-team
Flags: needinfo?(jdescottes)
Comment on attachment 8740016 [details]
MozReview Request: Bug 1258114 - use invisible borders to drag devtools splitters on both sides;r=bgrins

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45489/diff/6-7/
Missed one css import in a test running in the mochitest-oth test suite :/

New try https://treeherder.mozilla.org/#/jobs?repo=try&revision=4624be1a2325 !
Flags: needinfo?(jdescottes)
https://hg.mozilla.org/mozilla-central/rev/98b97a7844cc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.