936421 - Update the breadcrumbs as per Shorlander's new designs.

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Girish Sharma [:Optimizer]]

Basically there are two things to be done :

1) Match the breadcrumbs style (make it flatter, full height etc) as per http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail]
2) Match the text colors for various elements to that of the infobar. (purple for tagname, green for id etc.)

Pallets:
http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail]
http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail]

Comment 1

[Joe Walker [:jwalker]]

But don't start on this until we're sure of what we're doing. See bug 935956 for details.

Comment 2

[Girish Sharma [:Optimizer]]

How does highlighter v4 affect the breadcrumbs ?
Are we gonna lose breadcrumbs anytime soon ?

Comment 3

[Joe Walker [:jwalker]]

(In reply to Girish Sharma [:Optimizer] from comment #2)
> How does highlighter v4 affect the breadcrumbs ?
> Are we gonna lose breadcrumbs anytime soon ?

That's not something we've talks about, but I think it makes sense to see what UX does recommend. I've heard talk of allowing more than one thing to be highlighted for example. A few days shouldn't hurt.

Comment 4

[Girish Sharma [:Optimizer]]

(In reply to Joe Walker [:jwalker] from comment #3)
> (In reply to Girish Sharma [:Optimizer] from comment #2)
> > How does highlighter v4 affect the breadcrumbs ?
> > Are we gonna lose breadcrumbs anytime soon ?
> 
> That's not something we've talks about, but I think it makes sense to see
> what UX does recommend. I've heard talk of allowing more than one thing to
> be highlighted for example. A few days shouldn't hurt.

Yes sure :)
I was just concerned how infobar v4 affects breadcrumbs v2 :)
And that is an interesting discussion. What to do when multiple nodes are selected .

Comment 5

[Joe Walker [:jwalker]]

(In reply to Girish Sharma [:Optimizer] from comment #4)
> Yes sure :)
> I was just concerned how infobar v4 affects breadcrumbs v2 :)
> And that is an interesting discussion. What to do when multiple nodes are
> selected.

Dragonfly has a selection model like this. It may not affect the breadcrumbs, but I think it makes sense not to jump right into it.

Comment 6

[Paul Rouget [:paul]]

This should be based on top of bug 929127.

Also - if highlighter v4 affects this, this will be via bug 916443. And I don't believe the design will be affected by the location of the breadcrumbs.

Comment 7

[Joe Walker [:jwalker]]

It sounds from talking to Darrin that he's leaning towards a hover model for the highlighter, so not going down the dragonfly road.
So I think we shouldn't block this bug on that.

Comment 8

[Brian Grinstead [:bgrins]]

Darrin, can you provide any images needed (both 1x and 2x) for the breadcrumbs UI?  I believe that we can make it work with just the separator image between non active breadcrumbs, and that on the active one we can use borders in CSS to generate the triangle.

Comment 9

[Darrin Henein [:darrin]]

Attachment: Divider (Dark) @2x

Here is the dark theme @2x divider. Let me know if/when you need additional variations. The image is 48px x 24px, so it should take the full height of the bar and the chevron will be vertically centered.

For reference, the breadcrumb bar in the mockups is 48px tall. We can play with this as well though, in the name of reducing vertical space.

Comment 10

[Darrin Henein [:darrin]]

To clarify, when I said 48px that was @2x, so it should be actually 24px tall. My bad.

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: breadcrumb-compiled.png

Darrin, can you have a look at this screenshot from the patch that I've been working on?  It shows the crumbs in multiple states and locations (I was winging it with some of the additional states, since the reference mockup only shows the last node being selected).

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #11)
> Created attachment 8363883 [details]
> breadcrumb-compiled.png
> 
> Darrin, can you have a look at this screenshot from the patch that I've been
> working on?  It shows the crumbs in multiple states and locations (I was
> winging it with some of the additional states, since the reference mockup
> only shows the last node being selected).

You can also download an OSX build from https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-33db3aff6c3c/try-macosx64-debug/ to play with it.

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: theme-breadcrumbs.patch

Patrick,
Here is the code I'm currently working with.  The UI may change a bit based on feedback, but it should be pretty close to final here.

You will see a bunch of images have been removed from the project in the patch - this is because instead of border-images, I am using a single image for the 'chevron' element in between the crumbs, and a custom triangle looking element using a transform to cut the 'notches' out of the active breadcrumb.  I've also added a 2x version of the scrollbutton arrow that will take affect if you are on a retina display or set layout.css.devPixelsPerPx = 2 in your about:config.

In order to get the 'notched' effect, I'm using multiple CSS backgrounds with -moz-element on the active element - this copies the styles from an existing (positioned offscreen) element on the page.  If this is too weird, we could always get images sliced up for doing this (though we would need probably 8 images total to account for ltr/rtl and light/dark theme).  The nice part about using an element is that we can change all of these colors and orientations with CSS, plus it automatically works in 2x.

I've tried a bunch of different ways to achieve this effect (including using extra elements in between the crumbs in the DOM, and using pseudo elements without the benefit of position:absolute).  Overall, this solution (using multiple background images on the active node, and a single chevron image on the non active nodes) seems to cause the least amount of interference with the rest of the code.

Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=e6a2f4a5ee16.

Comment 14

[Brian Grinstead [:bgrins]]

Attachment: breadcrumb-ltr-rtl.png

Comparison of left to right and right to left styles for the new breadcrumbs style

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8364520 [details] [diff] [review]
theme-breadcrumbs.patch

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

I don't see anything wrong with the code, especially that it removes a lot of it, which is nice.
And I'm not uncomfortable with the -moz-element approach, it's definitely better than using images and other css-only solutions would have been complex too.
As long as we comment nicely why there's a separator element off-screen, then it's good. It gives us a lot of flexibility.

One thing I noticed, it's a little bit picky though, is that the an element is visibly smaller when it's selected than when it's not.
I'll attach a screenshot that shows an element's selected and unselected states overlaid on top of each other so you can see that they're not exactly the same size.
This was not the case before and I think it makes it look a bit weird at times.

I'm happy to R+ with this fixed.

::: browser/devtools/inspector/breadcrumbs.js
@@ +79,5 @@
> +                      "<box id='breadcrumb-separator-before'></box>" +
> +                      "<box id='breadcrumb-separator-after'></box>" +
> +                      "<box id='breadcrumb-separator-normal'></box>";
> +    this.container.parentNode.appendChild(this.separators);
> +

What if we want to have 2 breadcrumbs widgets in the same frame? Then we'd need only one instance of this separator element.
But that's up to you cause that usecase doesn't exist today.

Also, I know there's plenty of comments about this in the CSS file, but I think it would be good to add one here too, just to explain why these 3 types of separators are together in one element.

::: browser/devtools/shared/widgets/BreadcrumbsWidget.jsm
@@ +53,5 @@
> +                    "<box id='breadcrumb-separator-before'></box>" +
> +                    "<box id='breadcrumb-separator-after'></box>" +
> +                    "<box id='breadcrumb-separator-normal'></box>";
> +  this._parent.appendChild(this._separators);
> +

Same comments as in breadcrumbs.js
Also, do you know if we have a bug to merge breadcrumbs.js and BreadcrumbsWidget.jsm? It seems like they're doing the same thing and we should probably try to only keep one.

::: browser/themes/shared/devtools/widgets.inc.css
@@ +106,5 @@
> +
> +#breadcrumb-separator-normal {
> +  background: url([e-mail]) no-repeat center right;
> +  background-size: 12px 24px;
> +}

Perhaps here we want to explain why an image is used since we don't use any for the -before and -after separators.

Comment 16

[Patrick Brosset <:pbro>]

Attachment: breadcrumbs.png

Here's what I was talking about. It's a little bit hard to see, but you should see on the element in the middle that the chevron is further away to the right than the blue-ish arrow.

Also, I found bug 822388 which aims at using only one of the 2 breadcrumb widget implementations. Does your change apply to both?

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] from comment #16)
> Created attachment 8364969 [details]
> breadcrumbs.png
> 
> Here's what I was talking about. It's a little bit hard to see, but you
> should see on the element in the middle that the chevron is further away to
> the right than the blue-ish arrow.
> 

Thanks, I see and will look into the centering / size issue.

> Also, I found bug 822388 which aims at using only one of the 2 breadcrumb
> widget implementations. Does your change apply to both?

This patch is just applying the same changes to both, and not attempting to merge the inspector breadcrumbs into BreadcrumbsWidget (the one used by debugger).

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: breadcrumb-updated.png

Based on feedback from Comment 16 and Attachment 8364969 [details], I've kept the divider arrows more in line with the separator arrows.  Here is a screenshot of both LTR and RTL directions.  Darrin, can you have a look at this and let me know what you think?

Comment 19

[Darrin Henein [:darrin]]

Comment on attachment 8365978 [details]
breadcrumb-updated.png

This looks better, though I was noticing in teh mockup (http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail]) the highlight state extends to the top and bottom of that toolbar with no vertical padding. Will this have to wait for the other "smaller toolbar" stuff (borderless buttons, etc)?

Comment 20

[Brian Grinstead [:bgrins]]

Attachment: full-toolbar-height-with-buttons.png

(In reply to Darrin Henein [:darrin] from comment #19)
> Comment on attachment 8365978 [details]
> breadcrumb-updated.png
> 
> This looks better, though I was noticing in teh mockup
> (http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/
> [e-mail]) the highlight state extends to the top
> and bottom of that toolbar with no vertical padding. Will this have to wait
> for the other "smaller toolbar" stuff (borderless buttons, etc)?

Exactly, you can see in the debugger in this screenshot that the buttons get squished when we get rid of the paddings.  Similar to the issues we bumped into in Bug 942292.

Comment 21

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #20)
> Created attachment 8365994 [details]
> full-toolbar-height-with-buttons.png
> 
> (In reply to Darrin Henein [:darrin] from comment #19)
> > Comment on attachment 8365978 [details]
> > breadcrumb-updated.png

umm.. they look same to me ...

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #20)
> Created attachment 8365994 [details]
> full-toolbar-height-with-buttons.png
> 
> (In reply to Darrin Henein [:darrin] from comment #19)
> > Comment on attachment 8365978 [details]
> > breadcrumb-updated.png
> > 
> > This looks better, though I was noticing in teh mockup
> > (http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/
> > [e-mail]) the highlight state extends to the top
> > and bottom of that toolbar with no vertical padding. Will this have to wait
> > for the other "smaller toolbar" stuff (borderless buttons, etc)?
> 
> Exactly, you can see in the debugger in this screenshot that the buttons get
> squished when we get rid of the paddings.  Similar to the issues we bumped
> into in Bug 942292.

Added shadows on overflow arrows after discussing with Darrin.  Here is the latest try push (binaries will be available for testing after the build finishes): https://tbpl.mozilla.org/?tree=Try&rev=402aade6c797.

Comment 23

[Brian Grinstead [:bgrins]]

I've been chatting with Darrin about adding an ellipsis to the end of long text, but am bumping into some of the same issues as in Bug 684398.  There is some tricky stuff going on causing this not to work as expected.

Related to this, when there is a long class name on an element, the max-width doesn't seem to fully constrain the scrollbox, which causes a bunch of empty scrolling.  I've opened Bug 965007 to address this.

I'd prefer to deal with these separately though, as they are existing issues.  Darrin, what do you think?

Comment 24

[Brian Grinstead [:bgrins]]

Attachment: theme-breadcrumbs.patch

Updated patch with code that is almost the same as Attachment 8364520 [details] [diff], but with some styling tweaks based on UI feedback.

Comment 25

[Brian Grinstead [:bgrins]]

Attachment: breadcrumb-ellipsed.png

Whew, thanks to Victor in Bug 965007 I have resolved the final two UI issues (which are actually both existing issues, so this patch resolve two other bugs).  Here is a screenshot of the clipped text.

Comment 26

[Brian Grinstead [:bgrins]]

Attachment: theme-breadcrumbs2.patch

Patrick, can you take a look at the changes I've added to breadcrumbs.js since the last patch that cause ellipsis to show up?

Comment 27

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #26)
> Created attachment 8367313 [details] [diff] [review]
> theme-breadcrumbs2.patch
> 
> Patrick, can you take a look at the changes I've added to breadcrumbs.js
> since the last patch that cause ellipsis to show up?

Online interdiff of Attachment 8367079 [details] [diff] and Attachment 8367313 [details] [diff]: http://benjamin.smedbergs.us/interdiff/interdiff.php?patch1url=https%3A%2F%2Fbug936421.bugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D8367079&patch2url=https%3A%2F%2Fbug936421.bugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D8367313

Comment 28

[Brian Grinstead [:bgrins]]

Darrin, binaries from the latest patch can be grabbed from https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c86281d09187/try-macosx64-debug/

Comment 29

[Brian Grinstead [:bgrins]]

Attachment: theme-breadcrumbs3.patch

Same as previous patch - except that it includes height on .breadcrumbs-widget-container to prevent the size from expanding a few px once the breadcrumbs become populated, and a max-height to fix an issue on Linux I noticed in testing where the breadcrumbs are too tall.

Comment 30

[Patrick Brosset <:pbro>]

Comment on attachment 8367719 [details] [diff] [review]
theme-breadcrumbs3.patch

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

I don't have anything to say about the code, looks good from what I could see (thanks for the inter-diff link).
I'm happy with the new comments you added there too.

I tested with long ids/classes and pseudo-locks, the ellipsis seems to work fine.

I discovered one bug though. I'm happy to deal with it in a follow-up bug if you want because it's minor I'd say.
I'll attach a video so you can understand better but it occurs when you drag the markup-view/rule-view slider to the left, up the very edge of the window. That's when the scrollbox arrows disappear, and when you move the slider back to the right again, they don't re-appear. So you're left with a breadcrumb that has neither arrows to navigate nor shadows to indicate there's more content below the fold.
The arrows come back if you resize the window wide enough so that the breadcrumbs all fit without scroll needed.

So it's minor in the sense that no-one might ever run in this issue.
And for what it's worth, it might even be a bug of the XUL component itself, not your patch.

Comment 31

[Patrick Brosset <:pbro>]

Attachment: scrollbox-disappears.mov

Comment 32

[Brian Grinstead [:bgrins]]

Comment on attachment 8367719 [details] [diff] [review]
theme-breadcrumbs3.patch

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

::: browser/themes/shared/devtools/widgets.inc.css
@@ +26,5 @@
>       breadcrumbs-widget-items, to match toolbar-buttons style.
>       This negative margin compensates the extra row of pixels created
>       by the shadow.*/
>    margin-bottom: -1px;
> +  overflow: hidden;

The weird arrow disappearing problem seems to be caused by this line.  It shouldn't actually be needed anymore now that we are trimming string length with js, so I'm going to remove this line.

Comment 33

[Brian Grinstead [:bgrins]]

Attachment: theme-breadcrumbs4.patch

Same as before, but fixes the magic disappearing scrollbox

Comment 34

[Brian Grinstead [:bgrins]]

Attachment: theme-breadcrumbs4.patch

Oh, now I remember why I had overflow:hidden.  My box shadows on the scroll buttons were bleeding over on the edges.  I used Patrick's handy box shadow generator tool (https://bug946198.bugzilla.mozilla.org/attachment.cgi?id=8342336) to make a better one so it isn't a problem anymore.

Comment 37

[Brian Grinstead [:bgrins]]

Attachment: theme-breadcrumbs4.patch

Same patch, but removed white borders around active crumb (at least for now, until we remove toolbar padding).

Comment 38

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/a610a7d5bbe3
https://tbpl.mozilla.org/?tree=Fx-Team&rev=a610a7d5bbe3

Comment 39

[Brian Grinstead [:bgrins]]

Comment on attachment 8367918 [details] [diff] [review]
theme-breadcrumbs4.patch

Copying ui-review+ from Bug 957117 (which included breadcrumbs)

Comment 40

[Carsten Book [:Tomcat]]

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