Closed Bug 1262668 Opened 4 years ago Closed 3 years ago

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

Categories

(DevTools :: Inspector, defect, P1)

defect

Tracking

(firefox47 wontfix, firefox48 wontfix, firefox49 wontfix, firefox50 verified, firefox51 verified, firefox52 verified)

VERIFIED FIXED
Firefox 52
Tracking Status
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- verified
firefox51 --- verified
firefox52 --- verified

People

(Reporter: arni2033, Assigned: jdescottes)

References

()

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

>>>   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?
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 ?
See Also: 1242709
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.
Flags: needinfo?(hholmes)
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)
See Also: → 1262758
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?
Flags: needinfo?(pbrosset)
Flags: needinfo?(jdescottes)
Flags: needinfo?(hholmes)
(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)
(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.
Flags: needinfo?(jdescottes)
(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.
Summary: It's difficult to determine new position of splitter below breadcrumbs that look like toolbar → Breadcrumbs seem draggable/as if they have a splitter because of their styling
Flags: needinfo?(pbrosset)
Priority: -- → P2
Whiteboard: [btpp-fix-later]
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.
Attached image breadcrumb-border.png (obsolete) —
Attaching what I think the border should look like (1px solid #fafafa, a hex that doesn't currently have a variable attached to it).
Attached image breadcrumb-border.png
Updated to include dark theme.
Attachment #8740161 - Attachment is obsolete: true
Joe, status for 48?
Flags: needinfo?(jwalker)
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.
Flags: needinfo?(jwalker)
A few months have passed, what are your plans for this P2?
Flags: needinfo?(jdescottes)
We will discuss this one during inspector triage tomorrow.
Flags: needinfo?(jdescottes)
Whiteboard: [btpp-fix-later] → [inspector-triage]
Will try to improve this for Firefox 51.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Priority: P2 → P1
Whiteboard: [inspector-triage]
(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
What do you think about the colors proposed in comment #16 ?
Flags: needinfo?(hholmes)
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.
(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.
(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
(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! :)
Flags: needinfo?(hholmes)
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.
Flags: needinfo?(hholmes)
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!
Flags: needinfo?(hholmes)
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?
Attachment #8795297 - Flags: review?(hholmes) → review+
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?
Flags: needinfo?(ntim.bugs)
(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.
Flags: needinfo?(ntim.bugs)
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.
Attachment #8795297 - Flags: review?(hholmes) → review+
try is green, landing
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/c593d6a12071
update inspector breadcrumbs background and border color;r=helenvholmes
For reference, screenshot of light & dark themes now.
https://hg.mozilla.org/mozilla-central/rev/c593d6a12071
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
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?
Flags: qe-verify?
Flags: needinfo?(jdescottes)
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
Flags: needinfo?(jdescottes)
Attachment #8795297 - Flags: approval-mozilla-beta?
Attachment #8795297 - Flags: approval-mozilla-aurora?
Flags: qe-verify? → qe-verify+
Since this patch is low risk, and it's early in the cycle, we can consider uplifting to 50 as well.
Comment on attachment 8795297 [details]
Bug 1262668 - update inspector breadcrumbs background and border color;

CSS only, low-risk, Aurora51+, Beta50+
Attachment #8795297 - Flags: approval-mozilla-beta?
Attachment #8795297 - Flags: approval-mozilla-beta+
Attachment #8795297 - Flags: approval-mozilla-aurora?
Attachment #8795297 - Flags: approval-mozilla-aurora+
Hi Arni2033, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(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
Status: RESOLVED → VERIFIED
Flags: needinfo?(arni2033)
Verified as fixed using Firefox 50 RC and latest Dev Edition 51.0a2 across platforms.
See Also: → 1327782
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.