Brian Grinstead [:bgrins] Reporter
	Description • 8 years ago

The breadcrumbs in the inspector use XUL elements, for example: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/breadcrumbs.js#172

For Bug 1259121 we'll need a version of this that works without XUL.  We could consider simplifying the UI if getting the 'notched' design is tricky to port, since this has been discussed anyway.  Here's the breadcrumb's CSS: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/widgets.css#173-320

	Patrick Brosset <:pbro>
	Comment 1 • 8 years ago

Triaging as P2, we might want to change it to P1 since XUL->HTML is something we want to work on in Q2, but that's up to you.

I think we should take this opportunity to remove the siblings menu feature from the breadcrumbs widget. I don't think it brings a lot of value (in fact, it often clutters the menu to the point where it becomes unusable) and contextual menus are a challenge of their own already in a non-XUL world, so I suggest we remove openSiblingMenu.

	Brian Grinstead [:bgrins] Reporter
	Comment 2 • 8 years ago

(In reply to Patrick Brosset [:pbro] from comment #1)
> I think we should take this opportunity to remove the siblings menu feature
> from the breadcrumbs widget. I don't think it brings a lot of value (in
> fact, it often clutters the menu to the point where it becomes unusable) and
> contextual menus are a challenge of their own already in a non-XUL world, so
> I suggest we remove openSiblingMenu.

That'd work for me.  Helen, what do you think?

	Brian Grinstead [:bgrins] Reporter
	Comment 3 • 8 years ago

To be clear, openSiblingMenu is talking about inserting context menu items when right clicking a breadcrumb.  I'm thinking we don't need any context menu at all when right clicking a breadcrumb.

	Helen V. Holmes (:helenvholmes) (:✨)
	Comment 4 • 8 years ago

(In reply to Brian Grinstead [:bgrins] from comment #3)
> To be clear, openSiblingMenu is talking about inserting context menu items
> when right clicking a breadcrumb.  I'm thinking we don't need any context
> menu at all when right clicking a breadcrumb.

Thank you for clarifying!

I agree, this seems like something we can safely deprecate.

	Steve Melia Assignee
	Comment 5 • 8 years ago

I'd like to take this case; it looks like I need to remove openSiblingMenu; replace the xul labels with html:labels (createElementNS) and have a look at fixing up the css & cleaning up or writing some mochitests.

It looks like doing this will also close Bug 1214708 and Bug 1260052?

	Brian Grinstead [:bgrins] Reporter
	Comment 6 • 8 years ago

Thanks Steve.  Yes, looks like those should be able to be closed out after it's done.

	Steve Melia Assignee
	Comment 8 • 8 years ago

So, done a little bit on this; removed the context menu, replaced XUL labels in the JS, and have also replaced the <toolbar> and <arrowscrollbox> in inspector.xul with <divs>. Can target the selected breadcrumb with element.scrollIntoView() so pretty easy going so far.

Got some tests to fix; and then the next step will be reimplementing the <arrowscrollbox> for the overflow case which does seem as though it should also be fairly straightforward, is there an existing example of this? Drag/swipe to scroll would be nice too.

However - i've found a FIXME comment referencing Bug 822388; which mandates reuse of the BreadcrumbsWidget used in the debugger for breadcrumbing the call stack. Has this case been closed in error? It seems as though the inspector is still using a separate implementation. This is https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/widgets/BreadcrumbsWidget.jsm which looks a bit more up to date e.g. linting etc.

	Brian Grinstead [:bgrins] Reporter
	Comment 9 • 8 years ago

(In reply to Steve Melia from comment #8)
> Got some tests to fix; and then the next step will be reimplementing the
> <arrowscrollbox> for the overflow case which does seem as though it should
> also be fairly straightforward, is there an existing example of this?
> Drag/swipe to scroll would be nice too.

Not that I know of.  I guess that behavior could get fairly complex though, so we should confirm that this is desired UI for overflowing breadcrumbs.  Helen, how do you think we should handle breadcrumbs once it becomes too wide to fit in the markup view?  Should we keep the current behavior or is there something simpler we could do?  For instance if you inspect the 'hi' text in: data:text/html,<body><div><div><div><div><div><div><div><div>hi

> However - i've found a FIXME comment referencing Bug 822388; which mandates
> reuse of the BreadcrumbsWidget used in the debugger for breadcrumbing the
> call stack. Has this case been closed in error? It seems as though the
> inspector is still using a separate implementation. This is
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/
> widgets/BreadcrumbsWidget.jsm which looks a bit more up to date e.g. linting
> etc.

May have been closed erroneously, but I don't think we should take that work on here.  There's some upcoming debugger designs in which I believe its breadcrumbs are gone, so I think it's alright to do these changes in place and remove the fixme comment.  Patrick, what do you think?

	Patrick Brosset <:pbro>
	Comment 10 • 8 years ago

Yeah, don't worry about reusing the same breadcrumbs in both places. This is not what this bug should really worry about at this stage. And I also heard that we were getting rid of the breadcrumbs in the debugger.

	Helen V. Holmes (:helenvholmes) (:✨)
	Comment 11 • 8 years ago

(In reply to (Unavailable until Apr 6) Brian Grinstead [:bgrins] from comment #9)
> Not that I know of.  I guess that behavior could get fairly complex though,
> so we should confirm that this is desired UI for overflowing breadcrumbs. 
> Helen, how do you think we should handle breadcrumbs once it becomes too
> wide to fit in the markup view?  Should we keep the current behavior or is
> there something simpler we could do?  For instance if you inspect the 'hi'
> text in: data:text/html,<body><div><div><div><div><div><div><div><div>hi

I think we should probably just continue doing what we've been doing. In the future we should consider changing the style of those arrows, but I don't think we should change the functionality.

	Brian Grinstead [:bgrins] Reporter
	Comment 12 • 8 years ago

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?)) from comment #11)
> (In reply to (Unavailable until Apr 6) Brian Grinstead [:bgrins] from
> comment #9)
> > Not that I know of.  I guess that behavior could get fairly complex though,
> > so we should confirm that this is desired UI for overflowing breadcrumbs. 
> > Helen, how do you think we should handle breadcrumbs once it becomes too
> > wide to fit in the markup view?  Should we keep the current behavior or is
> > there something simpler we could do?  For instance if you inspect the 'hi'
> > text in: data:text/html,<body><div><div><div><div><div><div><div><div>hi
> 
> I think we should probably just continue doing what we've been doing. In the
> future we should consider changing the style of those arrows, but I don't
> think we should change the functionality.

OK.  I'd be happy with simplifying the behavior - like clicking on an arrow shifts the contents N pixels, but click and hold doesn't do anything special.  That'd limit the amount of work / complexity needed to match the XUL behavior exactly, but let me know if you think that's an important thing to keep.  We could also make crumbs more narrow by default and less likely to overflow.

	Steve Melia Assignee
	Comment 13 • 8 years ago

I need some feedback please! There are some broken tests I think mainly related to keyboard input, and I haven't added any of the javadoc style comments yet.

- I've broken out the scrollbox behaviour into a different prototype chain; not so much for re-use but more because the behaviour is quite distinct from the rest of the breadcrumb stuff. If this is OK; should it be in a different file?

- r.e. the arrowscrollbox; bearing in mind YAGNI I am not sure whether I should be adding the arrows etc. dynamically; or just hooking into stuff specified in inspector.xul?

- do I need to provide a destroy function to clean up? i.e. breadcrumbs.js:461, i'm not clear on why, (GC) is this a perf opt?

- i'm not at all sure about the CSS changes i've made; esp. whether they are in the right place. 

Regarding the scrolling behaviour i've tried to match this using element.scrollIntoView and repeating this every 150ms while the mouse is held; which seems to work OK albeit doesn't feel very elegant w.r.t. to the code. I thought it would be inappropriate to add any kind of touch scroll since this would be an enhancement and require another case..?

	Brian Grinstead [:bgrins] Reporter
	Comment 14 • 8 years ago

(In reply to Steve Melia from comment #13)
> Created attachment 8739809 [details] [diff] [review]
> Recreate XUL ArrowScrollBox with XHTML, change breadcrumbs in inspector to
> use it

Thanks, I'm working through a review with some more comments but the code is looking good so far

> I need some feedback please! There are some broken tests I think mainly
> related to keyboard input, and I haven't added any of the javadoc style
> comments yet.

Of course some tests can be removed if they are dealing with the menu feature we are removing.  Seems that keyboard functionality should generally still work unless if (a) the scrollbox was handling that for us, in which case I guess we'll need to port some of that behavior over or (b) the tests are triggering events on elements that have changed and we need to update the test.

> - I've broken out the scrollbox behaviour into a different prototype chain;
> not so much for re-use but more because the behaviour is quite distinct from
> the rest of the breadcrumb stuff. If this is OK; should it be in a different
> file?

That makes sense to me, fine in this file

> - r.e. the arrowscrollbox; bearing in mind YAGNI I am not sure whether I
> should be adding the arrows etc. dynamically; or just hooking into stuff
> specified in inspector.xul?
> 
> - do I need to provide a destroy function to clean up? i.e.
> breadcrumbs.js:461, i'm not clear on why, (GC) is this a perf opt?

It's mostly to unbind events from objects that will live on longer than the widget.  I don't know how needed it is in general, but it's convention to add a destroy() method which unbinds events.  In this case I don't believe it's needed though since the elements will be removed when the breadcrumbs widget is destroyed.

> - i'm not at all sure about the CSS changes i've made; esp. whether they are
> in the right place. 

Please move them to devtools/client/themes/inspector.css near the #inspector-breadcrumbs-toolbar rule

> Regarding the scrolling behaviour i've tried to match this using
> element.scrollIntoView and repeating this every 150ms while the mouse is
> held; which seems to work OK albeit doesn't feel very elegant w.r.t. to the
> code. I thought it would be inappropriate to add any kind of touch scroll
> since this would be an enhancement and require another case..?

I don't think we need anything too fancy.  I think handling a click and scrolling over a bit is fine, although I'll apply the patch and give more some feedback.

	Brian Grinstead [:bgrins] Reporter
	Comment 15 • 8 years ago

Comment on attachment 8739809 [details] [diff] [review]
Recreate XUL ArrowScrollBox with XHTML, change breadcrumbs in inspector to use it

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

Applying it and it's working really nice.  The main note right off the bat is: can you make the arrows disappear when they aren't needed?  When I open up data:text/html,<body><div>hi and inspect the body it ends up using up only part of the space (see comment in the CSS file about this), but also the scroll arrows show up and they shouldn't be even when all of the contents are visible.

::: devtools/client/inspector/breadcrumbs.js
@@ +97,5 @@
> +      }
> +
> +      element.scrollIntoView({ block: "end", behavior: "smooth" });
> +    };
> +  

Nit: trailing whitespace

@@ +108,5 @@
> +      this.leftBtn.setAttribute("disabled", "true");
> +    } else {
> +      this.leftBtn.removeAttribute("disabled");
> +    }
> + 

Nit: trailing whitespace

@@ +172,5 @@
> +    let rightBtnIcon = this.divFactory("toolbarbutton-icon");
> +    this.rightBtn.appendChild(rightBtnIcon);
> +    this.container.appendChild(this.rightBtn);
> +  },
> +  

Nit: trailing whitespace

@@ +173,5 @@
> +    this.rightBtn.appendChild(rightBtnIcon);
> +    this.container.appendChild(this.rightBtn);
> +  },
> +  
> +  divFactory : function(className) {

Maybe just call this createElement(tagName, className)

@@ +178,5 @@
> +    let div = this.chromeDoc.createElementNS(NS_XHTML, "div");
> +    div.className = className;
> +    return div;
> +  },
> +}; 

Nit: trailing whitespace

::: devtools/client/inspector/inspector.css
@@ +41,5 @@
> +  box-sizing: border-box;
> +}
> +
> +#inspector-breadcrumbs {
> +  display: -moz-box;

Please use display: flex instead (so we can get out of using XUL flex here)

@@ +44,5 @@
> +#inspector-breadcrumbs {
> +  display: -moz-box;
> +}
> +
> +.scrollbutton-up, .scrollbutton-down {

Prefix these with #inspector-breadcrumbs

@@ +45,5 @@
> +  display: -moz-box;
> +}
> +
> +.scrollbutton-up, .scrollbutton-down {
> +  -moz-box-flex: 0;

Something like `flex: 0; display: block;` instead

@@ +49,5 @@
> +  -moz-box-flex: 0;
> +  display: -moz-box;
> +}
> +
> +.html-arrowscrollbox-inner {

Prefix these with #inspector-breadcrumbs

@@ +50,5 @@
> +  display: -moz-box;
> +}
> +
> +.html-arrowscrollbox-inner {
> +  display: -moz-box;

Something like `flex: 1; display: block;` instead

@@ +56,5 @@
> +  -moz-box-flex: 1;
> +}
> +
> +.breadcrumbs-widget-item {
> +  -moz-box-flex: 0 0 auto;

Please use `flex` property here (flex: 0?)

@@ +60,5 @@
> +  -moz-box-flex: 0 0 auto;
> +}
> +
> +#inspector-breadcrumbs-inner {
> +	overflow:hidden;

Nit: 2 spaces

	Steve Melia Assignee
	Comment 16 • 8 years ago

Unfortunately i'm staring at defeat with the use of new flexbox, looking to implement something like http://codepen.io/anon/pen/pyVwjN - i've attached a patch with display:flex; this causes the breadcrumb trail to wrap, as expected I guess; but when I force it white-space: nowrap; or use display: flex; to achieve a similar effect the breadcrumb trail just pushes out under the splitter (taking the rest of the left hand pane with it) instead of overflowing.

I've spent hours fiddling about with it and tried all sorts; so if you have any ideas i'd really appreciate it!

Regarding the left and right arrows; these do indeed disappear on underflow but it seems your simple test case breaks that too! I'm hoping if I can solve problem #1 then having the trail container flex properly will fix this.

I had hoped to finish this one a bit more quickly than this - I now won't be able to return to this until around the 27th; I hope this is OK as i'm keen to finish this off and pick up another!

	Brian Grinstead [:bgrins] Reporter
	Comment 17 • 8 years ago

Comment on attachment 8742159 [details] [diff] [review]
1259812-broken.diff

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

I think I figured out a way to style this (it's a tricky interaction between CSS flex and XUL flex so it's hard to get right).  I'll attach a patch with the changes included

	Brian Grinstead [:bgrins] Reporter
	Comment 18 • 8 years ago

Try this one out - does it handle the cases you've been testing?

	Steve Melia Assignee
	Comment 19 • 8 years ago

Brian your changes are fantastic and work great. I've started looking at the failing tests, but i'm now back in a massive tangle!

This issue with the focus is a bit more serious than a styling problem; it seems that previously the XUL #inspector-breadcrumbs would keep focus when a child was clicked (?) whereas now clicking an (XHTML) button steals focus from #inspector-breadcrumbs. This means that if you scroll so that the selected item is not visible; then click a different item; focus is regained, firing the handler which scrolls the originally selected item back into view! I.e. it's completely broken.

I'm looking at trying to preserve the original behaviour; but I thought instead I could just replace the buttons with divs. The breadcrumb trail will never get keyboard focus; and key events would instead be handled by the markup panel. This would also happen to fix Bug 1199102. All key handling would be removed from the breadcrumb trail. Can you advise? There are a few tests around this that I am not sure what to do with e.g. browser_inspector_breadcrumbs_keybindings.

I've also found another issue where the trail is not updated after deleting a node item; i've opened Bug 1269226 for this.

	Steve Melia Assignee
	Comment 20 • 8 years ago

Proof of concept for removing keyboard handling from breadcrumb; in this case by just swallowing the click on breadcrumb trail buttons so they never receive focus. Proposed solution would be to replace the buttons with divs.

	Steve Melia Assignee
	Comment 21 • 8 years ago

Sorry I think this is a bit of late night rambling - i'm going to push on and try and preserve the original behaviour and move the suggestion for bubbling up the keyboard handling to Bug 1199102

	Steve Melia Assignee
	Comment 22 • 8 years ago

Ok I think i'm there:
- Resolved focus issue by suspending refocus during click event (breadcrumbs.js:446)
- Remove focus border from XHTML button
- Fix various mochitests by taking account of new DOM and lack of focus change on XHTML click (needs to now be done explicitly)
- Remove sibling menu test

	Brian Grinstead [:bgrins] Reporter
	Comment 23 • 8 years ago

Comment on attachment 8747692 [details] [diff] [review]
1259812.patch

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

This is looking great, see some feedback inline.  I've went ahead and pushed to the test server just to see if there are any remaining test issues: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb8fd771c2a2

::: devtools/client/inspector/breadcrumbs.js
@@ +134,5 @@
> +      this.leftBtn.setAttribute("disabled", "true");
> +    } else {
> +      this.leftBtn.removeAttribute("disabled");
> +    }
> + 

Please remove all trailing whitespace in this file, and also check for eslint errors with: `./mach eslint --no-ignore devtools/client/inspector/breadcrumbs.js`.  If you want to just fix the stuff in new code that's fine

@@ +442,5 @@
>     */
>    handleFocus: function(event) {
>      let control = this.container.querySelector(
>        ".breadcrumbs-widget-item[checked]");
> +    if (!this.suspendFocus && control && control !== event.target) {

Hm yeah this focus thing is odd, it might be easier for me to debug once bug 1242851 is finished landing (happening now) so the focused element is more visually obvious.  I'll take another look at it later but want to leave the rest of my feedback in the meantime.  But in general I'm happy with not modifying / removing keyboard access.

::: devtools/client/inspector/test/browser_inspector_breadcrumbs.js
@@ +18,5 @@
>  
>  add_task(function*() {
>    let { inspector } = yield openInspectorForURL(TEST_URI);
> +  let breadcrumbs = inspector.panelDoc.getElementById("inspector-breadcrumbs");
> +  let container = breadcrumbs.getElementsByClassName("html-arrowscrollbox-inner").item(0);

breadcrumbs.querySelector(".html-arrowscrollbox-inner") should have the same effect

::: devtools/client/themes/inspector.css
@@ +50,5 @@
> +  overflow: hidden;
> +}
> +
> +#inspector-breadcrumbs .breadcrumbs-widget-item {
> +  flex-shrink: 0;

This needs white-space: nowrap; to prevent wrapping when the markup view area is small with long breadcrumb items (like when inspecting a node on cnn.com).

	Steve Melia Assignee
	Comment 24 • 8 years ago

Apologies not sure if this is the right flag for the attachment.

More work to do i'm afraid as cnn.com has exposed quite a few oddities in my scrolling... I thought it was worth posting an updated patch anyway while I look at this.

	Brian Grinstead [:bgrins] Reporter
	Comment 25 • 8 years ago

Thanks!  By the way, do you mind if we split this patch up into two parts, where the first part is removing the context menu integration with breadcrumbs and the second does the main conversion?  I ask because as part of the de-XUL project I'm migrating the Inspector's context menu away from XUL and if we can remove this part it will help speed that along (see Bug 1266478).

I'll file a new bug just for removing this feature, and I can help with splitting up your patch / rebasing if you'd like.

	Steve Melia Assignee
	Comment 26 • 8 years ago

Ok split up the patch as you will see - unless you say otherwise I guess it's best to wait until that lands before rebasing here.

	Brian Grinstead [:bgrins] Reporter
	Comment 27 • 8 years ago

(In reply to Steve Melia from comment #26)
> Ok split up the patch as you will see - unless you say otherwise I guess
> it's best to wait until that lands before rebasing here.

Agreed

	Brian Grinstead [:bgrins] Reporter
	Comment 28 • 8 years ago

Comment on attachment 8748391 [details] [diff] [review]
1259812-lint.patch

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

Clearing request for now while we wait for Bug 1269915

	Steve Melia Assignee
	Comment 30 • 8 years ago

The XUL arrowscrollbox neatly abuts the right edge of the crumb when navigating right; and the left edge of the crumb when navigating left.

This also happens with element.scrollIntoView({ block: "start" }) and block: "end"; unless the element overflows the container, (as in cnn.com) and this caused a problem with my calculation of the leftwards/rightwards next crumb. This is now fixed in getFirst/LastInvisibleElement functions. (breadcrumbs.js:174)

	Brian Grinstead [:bgrins] Reporter
	Comment 31 • 8 years ago

I'm not sure if this actually depends on Bug 1264907, but there's a patch there that modifies the breadcrumbs by removing the 'first child' thing where it shows a node to the right of the currently selected node.

So at the least I'm sure we'll need to rebase on top of that once it lands

	Patrick Brosset <:pbro>
	Comment 32 • 8 years ago

(In reply to Brian Grinstead [:bgrins] from comment #31)
> I'm not sure if this actually depends on Bug 1264907, but there's a patch
> there that modifies the breadcrumbs by removing the 'first child' thing
> where it shows a node to the right of the currently selected node.
> 
> So at the least I'm sure we'll need to rebase on top of that once it lands
Merging shouldn't be too bad I hope. The code I removed in that other bug don't overlap the code changed here. If it turns out to be a complex one anyway, I'm happy to help do the merge.

	Brian Grinstead [:bgrins] Reporter
	Comment 33 • 8 years ago

Comment on attachment 8750493 [details] [diff] [review]
1259812-long-ids.patch

I think Patrick is the best reviewer here given his context with the widget in Bug 1264907 and elsewhere.  So, to make sure we don't do any duplicate / counterproductive work, I'm going to forward it to him (thanks in advance).

Patrick, we've been through a few rounds of review - the basic approach is to build an ArrowScrollBox widget to replace the XUL scrollbox.  There were also some focus changes needed that may interact with work in Bug 1264907 (see suspendFocus and Comment 19).

	Steve Melia Assignee
	Comment 34 • 8 years ago

Ok rebased, but now - in addition to "suspendFocus" (breadcrumbs.js:433) I need to draw your attention to another change around breadcrumbs.js:826

This is to resolve an issue that has occurred since the rebasing; where, with a closed inspector; "Inspect element" on an item where the crumb will exceed the bounds of the container causes the crumb to be scrolled incorrectly; for example inspecting "select me first" in my manual test case. (attached)

N.b. this does not occur if the inspector is already open.

I think it is related to the container width being set after the scroll has been initiated; so  by placing the scroll inside the waitForTick I allow the container size to be set first.

	Patrick Brosset <:pbro>
	Comment 36 • 8 years ago

Comment on attachment 8752614 [details] [diff] [review]
update reviewer name, fix null assignment on destroy

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

Thanks, this seems to work pretty well.
I did notice a few inconsistencies compared to the current implementation:

- The dotted focus outline seems to be gone. It would be nice to add it again, for accessibility. I think it was added with :-moz-focusring in bug 1242851. Feel free to needinfo :yzen if you need help with this.

- The font-family is different. At least on windows (which is where I tested), the font appears to be slightly bigger. It should be message-box I think. I'll include a screenshot.

- The tooltip seems to be gone (when hovering a button, there should be a tooltip that appears, which shows the full text).

- In the current XUL implementation, holding the mouse button down on an arrow scrolls through the breadcrumbs linearly. With the new implementation, it "jumps" from button to button. It does it smoothly, so it actually looks good, but it's different from the current impl, so I thought I'd mention it.
After reading the comments in the bug again, I think this is what you explained in comment 13. I don't personally have a problem with this because this scrolling behavior feels good to me. But I'll ping :helenvholmes about this.

- There's a small scrolling problem when selecting a node that's deep in then tree: say you have <body> selected (so there's no arrows in the breadcrumbs, everything fits in the available width), then you select an element that's deeply nested (so that the breadcrumbs creates all the buttons and needs to scroll right to show the selected one) --> when that happens, the last node (the selected one) isn't entirely visible in the toolbar. For me, I only see the cut off part of the blue arrow before the button, and I need to click on the scrolling arrow to see it entirely.

::: devtools/client/inspector/breadcrumbs.js
@@ +21,5 @@
> +const SCROLL_REPEAT_MS = 100;
> +
> +/**
> + * Component to replicate functionality of XUL arrowscrollbox
> + * for breadcrumbs

Please add some jsdoc @param here.

@@ +23,5 @@
> +/**
> + * Component to replicate functionality of XUL arrowscrollbox
> + * for breadcrumbs
> + */
> +function ArrowScrollBox(chromeWin, chromeDoc, container) {

We usually use the prefix 'chrome' as a way to differentiate with 'content'. However here there's no content, just chrome code, so I would simplify these variable names to just win, doc.
Also, I think you could just pass win:

function ArrowScrollBox(win, container) {
  this.win = win;
  this.doc = win.document;
  this.container = container;
  this._init();
}

@@ +36,5 @@
> +  /**
> +   * Build the HTML, add to the DOM and start listening to
> +   * events
> +   */
> +  _init: function () {

You haven't named any other method with the _ prefix, so for consistency, I would remove it here. If you really want to make a distinction between private and public, then I guess most of the methods of this class should have an _ prefix.

@@ +45,5 @@
> +    this.inner.addEventListener("scroll",
> +        this.onScroll.bind(this), false);
> +    this.inner.addEventListener("underflow",
> +        this.onUnderflow.bind(this), false);
> +    this.inner.addEventListener("overflow",

This is the first time I see underflow and overflow events being used on HTML elements. I have seen them being used in XUL before, but not in HTML.
I'm fairly sure they are non-standard and most probably only work in Firefox.
So, not for this bug, but we need to keep in mind for the future that we'll need to change this to something standard.

@@ +50,5 @@
> +        this.onOverflow.bind(this), false);
> +    this.leftBtn.addEventListener("mousedown",
> +        this.onLeftBtnClick.bind(this), false);
> +    this.rightBtn.addEventListener("mousedown",
> +        this.onRightBtnClick.bind(this), false);

In order to be a bit more consistent with most of the other classes in devtools, and in order to allow to init and destroy the widget whenever we want I would suggest the following:

- move the function bindings to the constructor,
- add a destroy method to the class that removes the event listeners, and call this.arrowScrollBox.destroy() in the destroy of HTMLBreadcrumbs.

@@ +59,5 @@
> +   * while the mouse button is held
> +   * @param {repeatFn} the function to repeat while the button is held
> +   */
> +  clickOrHold: function (repeatFn) {
> +    repeatFn = repeatFn.bind(this);

I think it should be the responsibility of the caller to bind the function properly. So this line should be removed, and see my comments in onLeft/RightBtnClick.

@@ +90,5 @@
> +  /**
> +   * When left arrow button is clicked scroll left
> +   */
> +  onLeftBtnClick: function () {
> +    let scrollLeft = function () {

If you use an arrow function, there will be no need to bind it to this:

let scrollLeft = () => {
  ...
};

@@ +92,5 @@
> +   */
> +  onLeftBtnClick: function () {
> +    let scrollLeft = function () {
> +      let element = this.getFirstInvisibleElement();
> +      if (element === null) {

if (!element) works just as well here and is shorter

@@ +106,5 @@
> +  /**
> +   * When right arrow button is clicked scroll right
> +   */
> +  onRightBtnClick: function () {
> +    let scrollRight = function () {

If you use an arrow function, there will be no need to bind it to this:

let scrollRight = () => {
  ...
};

@@ +108,5 @@
> +   */
> +  onRightBtnClick: function () {
> +    let scrollRight = function () {
> +      let element = this.getLastInvisibleElement();
> +      if (element == null) {

if (!element) works just as well here and is shorter

@@ +124,5 @@
> +   * enabled/disabled status of the arrow buttons
> +   */
> +  onScroll: function () {
> +    let first = this.getFirstInvisibleElement();
> +    if (first === null) {

Same comment about if (!first)

@@ +131,5 @@
> +      this.leftBtn.removeAttribute("disabled");
> +    }
> +
> +    let last = this.getLastInvisibleElement();
> +    if (last === null) {

Same here.

@@ +155,5 @@
> +    this.rightBtn.style.visibility = "visible";
> +  },
> +
> +  /**
> +   * Get the first (i.e. furthest left)

In RTL locales, the breadcrumbs is right to left unfortunately (in Firefox in Arabic for instance). So you can't assume that first == left and last == right.
And therefore, using scrollLeft below will lead to incorrect results.

For the sake of landing something and not making this patch too big and this bug too complex, feel free to file a separate bug for this.

@@ +231,5 @@
> +
> +  /**
> +   * Create an XHTML element with the given class name
> +   * @param {tagName} name of the tag to create
> +   * @param {className} class of the element

The jsdoc format we usually use these days is:
@param {Type} name Description.
So:
@param {String} tagName The name of the tag to create.
@param {String} className The className for the element.
Also, you're missing a return:
@return {DOMNode} The new element.

@@ +346,5 @@
>     * @param {NodeFront} node The node to pretty-print
>     * @returns {DocumentFragment}
>     */
> +  prettyPrintNodeAsXHTML: function (node) {
> +    let tagLabel = this.chromeDoc.createElementNS(NS_XHTML, "label");

<label> elements are normally associated to other elements (like inputs in forms). They act as caption for other elements.
Not a big deal at all, but I think <span>s would work just as well here.

::: devtools/client/themes/inspector.css
@@ +17,5 @@
> +
> +#inspector-breadcrumbs-toolbar,
> +#inspector-breadcrumbs-toolbar * {
> +  /* From minimal-xul.css */
> +  -moz-user-focus: ignore;

Oh, so you removed focus here. Can you explain why? Again, for accessibility reasons, we want the breadcrumbs to be keyboard navigable.

	Patrick Brosset <:pbro>
	Comment 37 • 8 years ago

> The font-family is different. At least on windows (which is where I
> tested), the font appears to be slightly bigger. It should be message-box I
> think. I'll include a screenshot.
Here's a comparison screenshot. Top is new, bottom is old.

	Patrick Brosset <:pbro>
	Comment 38 • 8 years ago

(In reply to Patrick Brosset <:pbro> from comment #36)
> - In the current XUL implementation, holding the mouse button down on an
> arrow scrolls through the breadcrumbs linearly. With the new implementation,
> it "jumps" from button to button. It does it smoothly, so it actually looks
> good, but it's different from the current impl, so I thought I'd mention it.
> After reading the comments in the bug again, I think this is what you
> explained in comment 13. I don't personally have a problem with this because
> this scrolling behavior feels good to me. But I'll ping :helenvholmes about
> this.
Helen: I thought I'd mention this to you. What do you think? 
Both implementations do scroll left and right correctly, it's just that previously it used to scroll linearly (same speed at all time), whereas now it looks like it snaps at each button. It does keep on scrolling if you hold the mouse button down, so there's no real change in functionality.

	Helen V. Holmes (:helenvholmes) (:✨)
	Comment 39 • 8 years ago

(In reply to Patrick Brosset <:pbro> from comment #38)
> Created attachment 8752770 [details]
> scroll.gif
> 
> (In reply to Patrick Brosset <:pbro> from comment #36)
> > - In the current XUL implementation, holding the mouse button down on an
> > arrow scrolls through the breadcrumbs linearly. With the new implementation,
> > it "jumps" from button to button. It does it smoothly, so it actually looks
> > good, but it's different from the current impl, so I thought I'd mention it.
> > After reading the comments in the bug again, I think this is what you
> > explained in comment 13. I don't personally have a problem with this because
> > this scrolling behavior feels good to me. But I'll ping :helenvholmes about
> > this.
> Helen: I thought I'd mention this to you. What do you think? 
> Both implementations do scroll left and right correctly, it's just that
> previously it used to scroll linearly (same speed at all time), whereas now
> it looks like it snaps at each button. It does keep on scrolling if you hold
> the mouse button down, so there's no real change in functionality.

This sounds fine to me. I think I'd need to see a try build to say definitively yes/no, but if everyone's feeling good about it it's probably fine to just move forward with it.

	Steve Melia Assignee
	Comment 40 • 8 years ago

Thanks Patrick! Here's another patch.

Regarding the scrolling problem with overflow; I have opened Bug 1274838 for this and as a workaround have changed the scroll triggered by an overflow to occur instantly instead of smooth, which unfortunately differs from the current behaviour. It is also possible to workaround this and keep the smooth scroll by forcing a reflow (e.g. calling this.container.offsetWidth) before the waitForTick ~breadcrumbs.js:879, but this seems a little nasty. The only other thing I can think of is to reimplement scrollIntoView in JS.

May I also draw your attention to breadcrumbs.js:191; I am a bit concerned that by checking for [checked] I break the encapsulation of the arrow scroll box which otherwise has no concept of a currently selected element, but to pass this event up to HTMLBreadcrumbs seems long winded.

- R.e. scrolling - yes let's see what Helen thinks after a try push and I can fix if necessary.

- R.e. RTL... it never crossed my mind! Yes I would like to file a separate bug for this.

- Regarding "-moz-user-focus: ignore", this was cargo-culted across from the XUL CSS without my really thinking about it, apologies.

Additionally:
- I forgot to implement double clicking the arrows; should skip to the first/last crumb.

- I encountered an odd rendering issue while looking at the build on Windows 10, the crumb text is only rendered after a short delay. This does not occur on Linux. I don't think it is related to my changes and i'm going to try and create a test case for it before submitting a bug.

Finally, regarding suspendFocus I have been busy making a fool of myself on Bug 1273125 and this will need excising once a solution is found; i've left it in for now since it's difficult to make any sense of the patch without it.

	Steve Melia Assignee
	Comment 41 • 8 years ago

Here's a gif of the font rendering issue; I thought they weren't being rendered at all but actually looks as though they're in white for a brief period before showing correctly.

This is on Windows 10; i'm going to file a separate defect but will try and create a test case first.

	Patrick Brosset <:pbro>
	Comment 42 • 8 years ago

(In reply to Steve Melia from comment #41)
> Created attachment 8755234 [details]
> 1259812-windows-10-white-crumb-rendering.gif
> 
> Here's a gif of the font rendering issue; I thought they weren't being
> rendered at all but actually looks as though they're in white for a brief
> period before showing correctly.
> 
> This is on Windows 10; i'm going to file a separate defect but will try and
> create a test case first.
I see it too on windows 10, but also without your patch, so it's definitely not your fault. Having said that, I do believe I see it more often with your patch than without, but I couldn't be sure.
It looks like a rendering issue for sure because it goes away if you scroll or hover the elements with the mouse (at least in my case). It'd be great if you could indeed log a bug with steps as specific as you can make them and then we'll try to have someone from graphics to investigate it.

	Patrick Brosset <:pbro>
	Comment 43 • 8 years ago

(In reply to Patrick Brosset <:pbro> from comment #42)
> (In reply to Steve Melia from comment #41)
> > Created attachment 8755234 [details]
> > 1259812-windows-10-white-crumb-rendering.gif
> > 
> > Here's a gif of the font rendering issue; I thought they weren't being
> > rendered at all but actually looks as though they're in white for a brief
> > period before showing correctly.
> > 
> > This is on Windows 10; i'm going to file a separate defect but will try and
> > create a test case first.
> I see it too on windows 10, but also without your patch, so it's definitely
> not your fault. Having said that, I do believe I see it more often with your
> patch than without, but I couldn't be sure.
> It looks like a rendering issue for sure because it goes away if you scroll
> or hover the elements with the mouse (at least in my case). It'd be great if
> you could indeed log a bug with steps as specific as you can make them and
> then we'll try to have someone from graphics to investigate it.
After messing around with the CSS of the breadcrumbs, disabling things randomly, I finally found out that the problem seems to come from background: -moz-element
See here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/widgets.css#271-281
The various arrows of the widget are generated with DOM elements used as backgrounds. Maybe there's a graphics bug with using them within a XUL document, not sure. In any case, removing these 3 backgrounds make the problem go away. So one option for that other bug will be to switch from using -moz-element to using normal background images, or maybe positioned pseudo-elements, or something else.

	Patrick Brosset <:pbro>
	Comment 44 • 8 years ago

(In reply to Steve Melia from comment #40)
> Regarding the scrolling problem with overflow; I have opened Bug 1274838 for
> this and as a workaround have changed the scroll triggered by an overflow to
> occur instantly instead of smooth, which unfortunately differs from the
> current behaviour. It is also possible to workaround this and keep the
> smooth scroll by forcing a reflow (e.g. calling this.container.offsetWidth)
> before the waitForTick ~breadcrumbs.js:879, but this seems a little nasty.
> The only other thing I can think of is to reimplement scrollIntoView in JS.
That's fine, the instant scroll didn't strike me as a problem when testing the new patch locally. No need to add complexity for this.

> May I also draw your attention to breadcrumbs.js:191; I am a bit concerned
> that by checking for [checked] I break the encapsulation of the arrow scroll
> box which otherwise has no concept of a currently selected element, but to
> pass this event up to HTMLBreadcrumbs seems long winded.
Ah, I see. Well, it's not a really big deal, it's not like we're going to reuse ArrowScrollBox somewhere else in another context where there isn't going to be a [checked] element. So I would R+ something like this. But, because you mention it, I can still suggest doing the following:

- Loading EventEmitter in the module:
  loader.lazyRequireGetter(this, "EventEmitter", "devtools/shared/event-emitter");
- Defining the ArrowScrollBox as being observable by adding this in its contructor:
  EventEmitter.decorate(this);
- change onUnderflow to this:
  onOverflow: function () {
    this.leftBtn.style.visibility = "visible";
    this.rightBtn.style.visibility = "visible";
    this.emit("underflow");
  },
- in HTMLBreadcrumbs:
  this.arrowScrollBox.on("underflow", this.scroll); in _init
  and 
  this.arrowScrollBox.off("underflow", this.scroll); in destroy

Should work, right? And doesn't seem too bad to me. At least the ArrowScrollBox only deals with the visibility of the arrows.

> - R.e. RTL... it never crossed my mind! Yes I would like to file a separate
> bug for this.
Ok, please file it now so we can start investigating how to fix this.

	Patrick Brosset <:pbro>
	Comment 45 • 8 years ago

Comment on attachment 8755233 [details] [diff] [review]
1259812-scrollfix.patch

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

R+ with the comments below addressed and a green try push. Here's one since I don't know if you have access:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b6946cca7b7

::: devtools/client/inspector/breadcrumbs.js
@@ +23,5 @@
> +/**
> + * Component to replicate functionality of XUL arrowscrollbox
> + * for breadcrumbs
> + *
> + * @param {ChromeWindow} win The window containing the breadcrumbs

Just Window instead of ChromeWindow here would be fine.

@@ +159,5 @@
> +   */
> +  onScroll: function () {
> +    let first = this.getFirstInvisibleElement();
> +    if (!first) {
> +      this.leftBtn.setAttribute("disabled", "true");

I know we said we'd fix the RTL-related problems in a follow-up bug, but please rename left to start and right to end (also the variables in getFirstInvisibleElement and getLastInvisibleElement). This will make sense when we work on RTL.

@@ +271,5 @@
> +   * @param {String} tagName name of the tag to create
> +   * @param {String} className class of the element
> +   * @return {DOMNode} The new element
> +   */
> +  createElement: function (tagName, className) {

A suggestion to make the code in constructHtml somewhat shorter:

createElement: function ({tagName, className, parent }) {
  let el = this.doc.createElementNS(NS_XHTML, tagName);
  el.className = className;
  if (parent) {
    parent.appendChild(el);
  }
  return el;
}

Also, see how I renamed div to el. Makes more sense since the element can have any tagName.

@@ +482,5 @@
>     */
>    handleFocus: function (event) {
>      let control = this.container.querySelector(
>        ".breadcrumbs-widget-item[checked]");
> +    if (!this.suspendFocus && control && control !== event.target) {

Ok, I hadn't fully read comment 19 up until now, so I didn't exactly know what problem this was solving. In fact, removing this.suspendFocus altogether seemed to have no effect at all for me. But I later understood that the problem occurred when the selected button was out of view.

So, I think it's fine to keep it here. But I would simply add a comment (maybe in the comment you already have in handleClick) that describes exactly the problem this is solving, and that references bug 1272011.
Indeed, the goal in bug 1272011 is to have focus on the breadcrumbs container only, never on the buttons themselves. So we'll fix this then.

::: devtools/client/themes/inspector.css
@@ +31,5 @@
> +  left: 0;
> +  bottom: 0;
> +  right: 0;
> +}
> +

nit: there are 2 empty lines here. Please separate rules with just 1 empty line.

	arni2033
	Comment 46 • 8 years ago

(In reply to Steve Melia from comment #41)
> Created attachment 8755234 [details]
> 1259812-windows-10-white-crumb-rendering.gif
Bug 1185349

	Steve Melia Assignee
	Comment 47 • 8 years ago

Fantastic! see attached. I was not sure what flags to set for this.

Handily, the extra journey round the event loop for the overflow event has worked around the scrollIntoView issue.

	Patrick Brosset <:pbro>
	Comment 48 • 8 years ago

Comment on attachment 8756037 [details] [diff] [review]
1259812-cleanup.patch

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

Thanks for the code changes. These look good. R+ for that.
However, the try build I launched last time shows a few test failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b6946cca7b7&selectedJob=21345082

TEST-UNEXPECTED-FAIL | devtools/client/framework/test/browser_toolbox_hosts.js | A promise chain failed to handle a rejection: - at resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/breadcrumbs.js:750 - TypeError: this.nodeHierarchy is null
TEST-UNEXPECTED-FAIL | devtools/client/animationinspector/test/browser_animation_refresh_on_removed_animation.js | A promise chain failed to handle a rejection: - at resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/breadcrumbs.js:750 - TypeError: this.nodeHierarchy is null
TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_initialization.js | Crumb refers to the right node - Got null, expected p#salutation
TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_initialization.js | Crumb refers to the right node - Got null, expected span
TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_initialization.js | Crumb refers to the right node - Got null, expected p#salutation
TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_initialization.js | Crumb refers to the right node - Got null, expected p#closing 

They probably all come from the same few code changes. These tests need to be changed. Can you please run them locally, investigate the failures, and find a fix for them?
You can run these tests with ./mach test path/to/test.js
There is documentation about tests here: https://wiki.mozilla.org/DevTools/Hacking#DevTools_Automated_Tests
Don't hesitate to ask for help if needed.

Once you find a fix, please upload it in a separate patch. Let's keep this one R+'d and unchanged, and once we have a green try with this second patch, we can fold them together and land the thing at once.

	Steve Melia Assignee
	Comment 49 • 8 years ago

Ran into Bug 1276533 but don't think it will affect the try push so I created a separate case for it; apologies if that is overkill.

This patch also updates a recently added test. (browser_inspector_breadcrumbs_namespaced.js)

	Patrick Brosset <:pbro>
	Comment 50 • 8 years ago

Comment on attachment 8757731 [details] [diff] [review]
1259812-tests.patch

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

Looks good to me.
Did all tests pass for you locally? In any case, here's a new TRY push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d33b802cf96

	Steve Melia Assignee
	Comment 51 • 8 years ago

Ahh well the last time I ran the full inspector suite must have been about comment 35, and this was mochitests only so perhaps missed simple tests?

this time I just ran the failed tests from the try and browser_inspector_breadcrumbs* (which caught the new one) - I'm glad this didn't turn out to be a waste of your time.

The try looks good I'm not sure about checkin-needed because there are two patches here?

	Patrick Brosset <:pbro>
	Comment 52 • 8 years ago

(In reply to Steve Melia from comment #51)
> The try looks good I'm not sure about checkin-needed because there are two
> patches here?
checkin-needed is fine with multiple patches too. Both need to be checked in, and will.
Thanks for your help Steve.

	Carsten Book [:Tomcat]
	Comment 53 • 8 years ago

needs rebasing is guess, because has problems to apply:

(eg '1-3,5', or 's' to toggle the sort order between id & patch description) 1
adding 1259812 to series file
renamed 1259812 -> 1259812-cleanup.patch
applying 1259812-cleanup.patch
patching file devtools/client/inspector/inspector.xul
Hunk #1 FAILED at 163
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/inspector/inspector.xul.rej
patching file devtools/client/themes/inspector.css
Hunk #1 FAILED at 5
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/themes/inspector.css.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 1259812-cleanup.patch

	Steve Melia Assignee
	Comment 54 • 8 years ago

see attached; the tests patch does not need rebasing. 

Re-ran the tests locally for devtools/client/inspector/tests and also re-ran the tests specified in Comment 48. Passing the needinfo to Patrick as i'm not sure what the procedure is now... e.g. try, re-reviewing etc.

	Matthew N. [:MattN]
	Comment 55 • 8 years ago

(In reply to Steve Melia from comment #54)
> Re-ran the tests locally for devtools/client/inspector/tests and also re-ran
> the tests specified in Comment 48. Passing the needinfo to Patrick as i'm
> not sure what the procedure is now... e.g. try, re-reviewing etc.

It depends on how trivial the rebase was. If it was trivial then there's no need for re-review or try.

	Steve Melia Assignee
	Comment 56 • 8 years ago

Comment on attachment 8758427 [details] [diff] [review]
1259812-cleanup-rebased.patch

Yes the rebasing was trivial - sounds like I should add the r+ and checkin-needed

	Carsten Book [:Tomcat]
	Comment 57 • 8 years ago

has problems to apply:

applying 1259812-tests.patch
patching file devtools/client/inspector/breadcrumbs.js
Hunk #1 FAILED at 747
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/inspector/breadcrumbs.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 1259812-tests.patch

could you take a look, thanks!

	Patrick Brosset <:pbro>
	Comment 58 • 8 years ago

I will rebase the patches and land them.

	Pulsebot
	Comment 59 • 8 years ago

Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/7cd31f8bca62
Replace XUL elements with XHTML in inspector breadcrumbs;r=pbro
https://hg.mozilla.org/integration/fx-team/rev/5d2cb7fff024
Fix tests to account for change in tooltip text and breadcrumb lifecycle;r=pbro

	Steve Melia Assignee
	Comment 60 • 8 years ago

Apologies looks like my error is failing to specify the ordering of the patches. Thanks for helping me land this.

	Carsten Book [:Tomcat]
	Comment 61 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/7cd31f8bca62
https://hg.mozilla.org/mozilla-central/rev/5d2cb7fff024

	Ada [:adalucinet]
	Comment 63 • 8 years ago

Verified with latest Aurora 49.0a2, across platforms [1]. 
Found 1 issue though: while navigating through HTML tree, intermittently, some of the breadcrumbs are not displayed and the right arrow from the breadcrumbs toolbar is enabled, but not functional. Here is a screen recording - https://goo.gl/LBmkFQ  
Steve, any ideas? Thanks in advance!

[1] Windows 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.11.1

	Steve Melia Assignee
	Comment 64 • 8 years ago

The right arrow issue is probably the bounds checking I have refactored as part of Bug 1275399 so I will see if I can get a repro case and check if this still occurs with that patch. I am not sure about the breadcrumb not scrolling with the markup pane selection - I'll take a look, hopefully sometime this week.

	Steve Melia Assignee
	Comment 65 • 8 years ago

I've got a repro and fix for the issue identified in Comment 63; see Bug 1275399 Comment 5

	Ada [:adalucinet]
	Comment 66 • 8 years ago

Thanks for the feedback, Steve!
Since the issue mentioned in comment 63 is already tracked separately, marking here accordingly.