1262668 - Breadcrumbs seem draggable/as if they have a splitter because of their styling

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 48, 32bit, ID 20160403030243
STR:
1. Enable DARK theme in devtools. Open Inspector, dock it to the bottom side of window
2. Hide sidebar in Inspector, show Console
3. Look at the visual difference between Console and Inspector

AR:
 There's no difference. It's now hard to determine where the splitter is.
 Now it looks like the splitter is the line between breadcrumbs and markup.
  
ER:
 It should be easy to hit a splitter. Since we can't drag the whole toolbar like in GoogleChrome...

Note:
 I'm not alone! Julian Descottes also encountered this bug. Well, he didn't tell me that, I saw it in
 bug 1260053 comment 11, in the GIF he recorded: attachment 8736652 [details]. Earlier, before
 bug 1242709 made theme pale, this could be easily fixed by setting content background to breadcrumbs
 I still don't get it, why more "improvements" (like poor blurred icons), why not long-standing bugs?

Comment 1

[Tim Nguyen :ntim]

There's absolutely no relation to bug 1242709...

Helen, I do agree this is an issue, what do you think ? Maybe we can make the whole toolbar draggable for side host? Or maybe we can remove the top border from the breadcrumbs ?

Comment 2

[Patrick Brosset <:pbro>]

I've been bitten by this already too. But I would argue that it's actually better with the dark theme. 
With the light theme, both the breadcrumbs and the console have a top border that look similar, so it's easy to get confused and try dragging from the top border of the breadcrumbs.
However, with the dark theme, you can't see the black top border of the breadcrumbs, and the top border of the console really stands out, so it's kind of hard to miss it.
But I suspect this is very subjective, and as I said, I've also been bitten by this, so I agree this is confusing and making it harder to resize the console.

Having the whole toolbar draggable like in Chrome would definitely help, but I think what makes it work in Chrome mostly is the fact that the breadcrumbs and console toolbar have different background colors. Like, they really look different visually, and the breadcrumbs' background is the same as the inspector's. So, visually, it looks like they are in the same panel, so it's harder to make the same mistake.
Also in Chrome, unless I don't know how to do it, you can't hide the sidebar panel and so, having it always around provides even more of a visual clue that the breadcrumbs isn't a toolbar you can drag from.
This is why step 2 in the STR is important. However, if you dock the inspector to the right side of the window, then you end up with the same problem of being confused between the breadcrumbs and the sidebar's tabs (rules, computed, ...).

So, maybe we need to change the background of the breadcrumbs, maybe change the border color, I don't know. Or maybe get rid of the breadcrumbs altogether!

Tim forgot to NI? Helen in comment 1.

Comment 3

[Julian Descottes [:jdescottes]]

It would be nice here to differentiate splitters from simple borders. What about making the splitter borders slightly darker in light theme?

(FYI: filed Bug 1262758 to increase the clickable area of splitters. It is only 3px wide at the moment. So small you easily miss the cursor change when hovering a splitter, which makes it even harder to tell what is border from what is a splitter)

Comment 4

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

Some more context would be helpful:

Is the issue that the splitter is confusing because of its color? That visually it's not distinct enough?

Or is the issue that it's difficult to grab things because the splitter area is small and difficult to hover over to get the icon to drag in the first place?

Or both?

It seems like the first issue is occurring because the breadcrumbs seem like you can interact with them/drag them when you cannot. I think that can be solved by changing the background color of the breadcrumbs so they seem more "disabled".

The fact that the splitter areas are small and hard to grab seems like a separate issue that seems more serious to me. Should we have a discussion about making toolbars as an area draggable?

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?)) from comment #4)
> The fact that the splitter areas are small and hard to grab seems like a
> separate issue that seems more serious to me. Should we have a discussion
> about making toolbars as an area draggable?

Julian is currently working on making the splitter bigger in Bug 1258114 (toolbars being draggable is another bug, but I'm not convinced we'd need to do that once we make the draggable area on splitters larger)

Comment 6

[Julian Descottes [:jdescottes]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?)) from comment #4)
> Some more context would be helpful:
> 
> Is the issue that the splitter is confusing because of its color? That
> visually it's not distinct enough?
> 
> Or is the issue that it's difficult to grab things because the splitter area
> is small and difficult to hover over to get the icon to drag in the first
> place?
> 

Sorry for making this confusing. This bug is about making the splitter more "visible" to the user, either by changing colors, changing the breadcrumbs styling etc...

As Brian said, increasing the splitter area is handled in Bug 1258114. I mentioned it because I believe a bigger splitter area will slightly improve the situation here: the user will clearly see a cursor change when hovering the splitter. But that's a minor detail.

Comment 7

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

(In reply to Julian Descottes [:jdescottes] from comment #6)
> Sorry for making this confusing. This bug is about making the splitter more
> "visible" to the user, either by changing colors, changing the breadcrumbs
> styling etc...
So I think the issue is less with the splitter itself and more that the breadcrumbs /seem/ operable in some way—I think the solution is to give the breadcrumbs more of a disabled background. I'm not sure which bug at this point is the right one to rename.

> As Brian said, increasing the splitter area is handled in Bug 1258114. I
> mentioned it because I believe a bigger splitter area will slightly improve
> the situation here: the user will clearly see a cursor change when hovering
> the splitter. But that's a minor detail.
I've been working with James to restyle the debugger and I like the solution we've come up with there for splitters: https://github.com/jlongster/debugger.html

The splitter visually looks the same, but there's a larger space to the right and left (created with a white border) where one can grab the splitter.

Comment 8

[arni2033]

Attachment: screenshot 1 - If somebody's still going to work on this, please consider presence of scrollbars.png

I'm sorry: I was wrong about this bug. There are (were) other "pseudo toolbars" in devtools:
Netmonitor (already fixed), small sidebar in Debugger, Scratchpad (recently added).
Probably the reason I only noticed it now is because I only use Console in Inspector and Debugger

Now I can't even think of possible suggestions (probably it's WONTFIX), so I'll just move breadcrumbs
to the top using CSS, as I can accidentally hit a button on Taskbar while clicking on breadcrumbs.

Comment 9

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

Attachment: breadcrumb-border.png

Attaching what I think the border should look like (1px solid #fafafa, a hex that doesn't currently have a variable attached to it).

Comment 10

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

Attachment: breadcrumb-border.png

Updated to include dark theme.

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Joe, status for 48?

Comment 12

[Julian Descottes [:jdescottes]]

We currently don't have plans for fixing this issue in Firefox 48 or 49. Even though it is a regression, it is not severe enough to require an urgent fix & uplift. Added tracking for Firefox 50.

Comment 13

[Jim Mathies [:jimm]]

A few months have passed, what are your plans for this P2?

Comment 14

[Julian Descottes [:jdescottes]]

We will discuss this one during inspector triage tomorrow.

Comment 15

[Julian Descottes [:jdescottes]]

Will try to improve this for Firefox 51.

Comment 16

[Julian Descottes [:jdescottes]]

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #10)
> Created attachment 8740435 [details]
> breadcrumb-border.png
> 
> Updated to include dark theme.

Thanks for the suggestions Helen! After trying them out, I want to propose slight modifications :)

Light theme:
#fafafa is really close to the current background color of the breadcrumbs (#fcfcfc). To my eye, it looks more like a gap than a border. #f3f3f3 ?

(in the mockups, the breadcrumbs have the same background-color as the markup view, which can explain why #fafafa works better in the mockups than in the current inspector?)

Dark theme:
#444b5b is really close to the current splitter color (#454d5d), I can't really tell the difference. What about #3d4656.

Try push if you want to grab builds to test the colors: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf08e5aa9008

Comment 17

[Julian Descottes [:jdescottes]]

What do you think about the colors proposed in comment #16 ?

Comment 18

[Tim Nguyen :ntim]

I think the main problem is the background-color, not the border-color. I think it would be enough to change the background-color to var(--theme-body-background) and keep the border as it currently is.

Comment 19

[Julian Descottes [:jdescottes]]

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #18)
> I think the main problem is the background-color, not the border-color. I
> think it would be enough to change the background-color to
> var(--theme-body-background) and keep the border as it currently is.

That's a good point, but I'd say both are needed. I know this is highly subjective (and depends on your screen, calibration etc...), but with the light theme, just updating the background color does not make much of a visual difference for me. I'll submit a patch combining both.

Comment 20

[arni2033]

(In reply to Julian Descottes [:jdescottes] from comment #19)
> with the light theme, just updating the background color does not make much of a visual difference
It is so, because bug 1242709 broke the backgrounds, as I explained in comment 0.
At this point relation to bug 1242709 (rejected in comment 1) should be obvious for everybody

Comment 21

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

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #18)
> I think the main problem is the background-color, not the border-color. I
> think it would be enough to change the background-color to
> var(--theme-body-background) and keep the border as it currently is.

I agree with this, although I'm happy to look over your combination, Julian—you've been looking at this longer than I have! :)

Comment 22

[Julian Descottes [:jdescottes]]

Thanks for the feedback!

Try push using theme-body-background for the breadcrumbs toolbar:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a37a721f592

Try push combining lighter borders + using theme-body-background:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bd992728ff3

Let me know which one we should use.

Comment 23

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

I think the second try-push works well for the reason the bug was originally opened:

> Try push combining lighter borders + using theme-body-background:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bd992728ff3
>

Thanks for the work, Julian!

Comment 24

[Julian Descottes [:jdescottes]]

Attachment: Bug 1262668 - update inspector breadcrumbs background and border color;

When the inspector is in portrait mode, the breadcrumbs top border can easily be
mistaken for the splitter between the markupview and the side panel.

This patch reuses the body background for the breadcrumbs background and uses a
custom border color for the top border of the breadcrumbs. In the dark theme this
color remains the same as the splitter color, because the background change is
significant enough. In the light/firebug theme, the color is lighter than the
splitter color

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

Comment 25

[Tim Nguyen :ntim]

Comment on attachment 8795297 [details]
Bug 1262668 - update inspector breadcrumbs background and border color;

https://reviewboard.mozilla.org/r/81398/#review80104

::: devtools/client/themes/inspector.css:121
(Diff revision 1)
>  #inspector-breadcrumbs-toolbar {
>    padding: 0px;
>    border-bottom-width: 0px;
>    border-top-width: 1px;
> +  border-top-color: var(--theme-breadcrumbs-border-color);
> +  background-color: var(--theme-body-background);

Might be worth adding a comment referencing to this bug:
/* Bug 1262668 - Use a lighter background so the breadcrumbs toolbar doesn't get mistaken as a splitter */

::: devtools/client/themes/variables.css:29
(Diff revision 1)
>    --theme-selection-background: #4c9ed9;
>    --theme-selection-background-semitransparent: rgba(76, 158, 217, 0.15);
>    --theme-selection-color: #f5f7fa;
>    --theme-splitter-color: #dde1e4;
>    --theme-comment: #696969;
> +  --theme-breadcrumbs-border-color: #f3f3f3;

I'm not a fan of having tool specific vars inside variables.css. Maybe we can rename it?

Comment 26

[Julian Descottes [:jdescottes]]

Comment on attachment 8795297 [details]
Bug 1262668 - update inspector breadcrumbs background and border color;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81398/diff/1-2/

Comment 27

[Julian Descottes [:jdescottes]]

Thanks for the reviews Helen and Tim.

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #25)
> Comment on attachment 8795297 [details]
> Bug 1262668 - update inspector breadcrumbs background and border color;
> 
> https://reviewboard.mozilla.org/r/81398/#review80104
> 
> ::: devtools/client/themes/variables.css:29
> (Diff revision 1)
> >    --theme-selection-background: #4c9ed9;
> >    --theme-selection-background-semitransparent: rgba(76, 158, 217, 0.15);
> >    --theme-selection-color: #f5f7fa;
> >    --theme-splitter-color: #dde1e4;
> >    --theme-comment: #696969;
> > +  --theme-breadcrumbs-border-color: #f3f3f3;
> 
> I'm not a fan of having tool specific vars inside variables.css. Maybe we
> can rename it?

Not a big fan either. I used --theme-inactive-border-color before and I switched back to this in the version I uploaded here. 

But honestly this variable is so contextual to the breadcrumbs that I have a hard time finding a good name for it. This variable is set so that the breadcrumbs border is not getting confused with a splitter. Do we think we might reuse it in practice?

The other option is to declare this variable in inspector.css (similar to what is done in canvasdebugger.css for instance).
What do you think?

Comment 28

[Tim Nguyen :ntim]

(In reply to Julian Descottes [:jdescottes] from comment #27)
> Thanks for the reviews Helen and Tim.
> 
> (In reply to Tim Nguyen :ntim (use needinfo?) from comment #25)
> > Comment on attachment 8795297 [details]
> > Bug 1262668 - update inspector breadcrumbs background and border color;
> > 
> > https://reviewboard.mozilla.org/r/81398/#review80104
> > 
> > ::: devtools/client/themes/variables.css:29
> > (Diff revision 1)
> > >    --theme-selection-background: #4c9ed9;
> > >    --theme-selection-background-semitransparent: rgba(76, 158, 217, 0.15);
> > >    --theme-selection-color: #f5f7fa;
> > >    --theme-splitter-color: #dde1e4;
> > >    --theme-comment: #696969;
> > > +  --theme-breadcrumbs-border-color: #f3f3f3;
> > 
> > I'm not a fan of having tool specific vars inside variables.css. Maybe we
> > can rename it?
> 
> Not a big fan either. I used --theme-inactive-border-color before and I
> switched back to this in the version I uploaded here. 
> 
> But honestly this variable is so contextual to the breadcrumbs that I have a
> hard time finding a good name for it. This variable is set so that the
> breadcrumbs border is not getting confused with a splitter. Do we think we
> might reuse it in practice?
That's something Helen should be able to answer

> The other option is to declare this variable in inspector.css (similar to
> what is done in canvasdebugger.css for instance).
> What do you think?
I like calling it --breadcrumbs-border-color and putting it in inspector.css if it's really specific to breadcrumbs, otherwise we can name it --theme-secondary/inactive-border-color and put it in variables.css.

Comment 29

[Julian Descottes [:jdescottes]]

Comment on attachment 8795297 [details]
Bug 1262668 - update inspector breadcrumbs background and border color;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81398/diff/2-3/

Comment 30

[Julian Descottes [:jdescottes]]

Comment on attachment 8795297 [details]
Bug 1262668 - update inspector breadcrumbs background and border color;

I don't know why the review flag got reset here, carrying over.

Comment 31

[Julian Descottes [:jdescottes]]

try is green, landing

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/c593d6a12071
update inspector breadcrumbs background and border color;r=helenvholmes

Comment 33

[Julian Descottes [:jdescottes]]

Attachment: breadcrumbs_border_style.png

For reference, screenshot of light & dark themes now.

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c593d6a12071

Comment 35

[Gerry Chang [:gchang]]

Hi :jdescottes,
Since this bug is a regression and also affects 51, do you consider to uplift this for 51 if this patch is not too risky?

Comment 36

[Tim Nguyen :ntim]

Comment on attachment 8795297 [details]
Bug 1262668 - update inspector breadcrumbs background and border color;

Approval Request Comment
[Feature/regressing bug #]: bug 1256422
[User impact if declined]: Breadcrumb border looks like a splitter
[Describe test coverage new/current, TreeHerder]: on Nightly.
[Risks and why]: Low risk, CSS only 
[String/UUID change made/needed]: no

Comment 37

[Tim Nguyen :ntim]

Since this patch is low risk, and it's early in the cycle, we can consider uplifting to 50 as well.

Comment 38

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8795297 [details]
Bug 1262668 - update inspector breadcrumbs background and border color;

CSS only, low-risk, Aurora51+, Beta50+

Comment 39

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Arni2033, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 40

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/5d8fa3122348

Comment 41

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

https://hg.mozilla.org/releases/mozilla-beta/rev/d1699ef21941

Comment 42

[arni2033]

Sorry for long response.
I remember: I tested it a month ago on Nightly 2016-10-01 and found fix range - it was fixed

Comment 43

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed using Firefox 50 RC and latest Dev Edition 51.0a2 across platforms.