Closed Bug 936421 Opened 6 years ago Closed 6 years ago

Update the breadcrumbs as per Shorlander's new designs.

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: Optimizer, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 11 obsolete files)

1.18 KB, image/png
Details
241.64 KB, image/png
Details
145.32 KB, image/png
Details
65.14 KB, patch
bgrins
: review+
bgrins
: ui-review+
Details | Diff | Splinter Review
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/DarkTheme-Inspector-Active@2x.png
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/DarkTheme-Color-Palette@2x.png
http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/LightTheme-Color-Palette@2x.png
But don't start on this until we're sure of what we're doing. See bug 935956 for details.
Blocks: 935956
How does highlighter v4 affect the breadcrumbs ?
Are we gonna lose breadcrumbs anytime soon ?
(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.
(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 .
(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.
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.
Depends on: 929127
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.
Blocks: 916766
Depends on: 943883
No longer depends on: 943883
Depends on: 943883
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.
Flags: needinfo?(dhenein)
Attached image 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.
Flags: needinfo?(dhenein)
To clarify, when I said 48px that was @2x, so it should be actually 24px tall. My bad.
Attached image breadcrumb-compiled.png (obsolete) —
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).
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8363883 - Flags: ui-review?(dhenein)
(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/bgrinstead@mozilla.com-33db3aff6c3c/try-macosx64-debug/ to play with it.
Attached patch theme-breadcrumbs.patch (obsolete) — Splinter Review
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.
Attachment #8364520 - Flags: review?(pbrosset)
Attached image breadcrumb-ltr-rtl.png (obsolete) —
Comparison of left to right and right to left styles for the new breadcrumbs style
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(breadcrumbs-divider@2x.png) 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.
Attachment #8364520 - Flags: review?(pbrosset) → review+
Attached image breadcrumbs.png (obsolete) —
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?
(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).
Attached image breadcrumb-updated.png (obsolete) —
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?
Attachment #8363883 - Attachment is obsolete: true
Attachment #8364525 - Attachment is obsolete: true
Attachment #8363883 - Flags: ui-review?(dhenein)
Attachment #8365978 - Flags: ui-review?(dhenein)
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/DarkTheme-Inspector-Locked@2x.png) 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)?
Attachment #8365978 - Flags: ui-review?(dhenein) → ui-review-
(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/
> DarkTheme-Inspector-Locked@2x.png) 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.
(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 ...
(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/
> > DarkTheme-Inspector-Locked@2x.png) 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.
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?
Flags: needinfo?(dhenein)
Attached patch theme-breadcrumbs.patch (obsolete) — Splinter Review
Updated patch with code that is almost the same as Attachment 8364520 [details] [diff], but with some styling tweaks based on UI feedback.
Attachment #8364520 - Attachment is obsolete: true
Attachment #8367079 - Flags: review+
Attached image 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.
Flags: needinfo?(dhenein)
Attached patch theme-breadcrumbs2.patch (obsolete) — Splinter Review
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?
Attachment #8367079 - Attachment is obsolete: true
Attachment #8367313 - Flags: review?(pbrosset)
(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
Attachment #8365978 - Attachment is obsolete: true
Attachment #8364969 - Attachment is obsolete: true
Attached patch theme-breadcrumbs3.patch (obsolete) — Splinter Review
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.
Attachment #8367313 - Attachment is obsolete: true
Attachment #8367313 - Flags: review?(pbrosset)
Attachment #8367719 - Flags: review?(pbrosset)
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.
Attachment #8367719 - Flags: review?(pbrosset) → review+
Attached video scrollbox-disappears.mov (obsolete) —
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.
Attached patch theme-breadcrumbs4.patch (obsolete) — Splinter Review
Same as before, but fixes the magic disappearing scrollbox
Attachment #8367719 - Attachment is obsolete: true
Attachment #8367816 - Attachment is obsolete: true
Attachment #8367880 - Flags: review+
Attached patch theme-breadcrumbs4.patch (obsolete) — Splinter Review
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.
Attachment #8367880 - Attachment is obsolete: true
Attachment #8367892 - Flags: review+
Duplicate of this bug: 965007
Duplicate of this bug: 684398
Same patch, but removed white borders around active crumb (at least for now, until we remove toolbar padding).
Attachment #8367918 - Flags: ui-review?(dhenein)
Attachment #8367918 - Flags: review+
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [fixed-in-fx-team]
Attachment #8367892 - Attachment is obsolete: true
Comment on attachment 8367918 [details] [diff] [review]
theme-breadcrumbs4.patch

Copying ui-review+ from Bug 957117 (which included breadcrumbs)
Attachment #8367918 - Flags: ui-review?(dhenein) → ui-review+
https://hg.mozilla.org/mozilla-central/rev/a610a7d5bbe3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.