672006 - [highlighter] Selecting a Node should update the Highlighter's main toolbar with a breadcrumb display

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Rob Campbell [:rc] (:robcee)]

the breadcrumbs should show all of the parent nodes up to the selected node and probably the first child as well, if any.

e.g.,
body > div#first > div#content > p

Comment 1

[Rob Campbell [:rc] (:robcee)]

We spoke to Frank Yan briefly about this. We need to loop back and ask him what he thinks. CC'ing.

Comment 2

[Rob Campbell [:rc] (:robcee)]

Paul, could you put up your WIP patch here, please?

Comment 3

[Paul Rouget [:paul]]

Attachment: patch v0.1 WIP

Comment 4

[Paul Rouget [:paul]]

Attachment: patch v0.2 WIP

TODO:

x Add buttons to scroll left and right
x what about RTL?
x move the style to the platform-dependent locations
x implement Shorlander's design
x support mutation events
x animate scrolling
x show id + classes (with ellipsis if to large) in buttons
x add the dropdown menus to show the siblings

Comment 5

[Paul Rouget [:paul]]

As discussed with :robcee, I un-assign myself from this bug. I'm not sure to have the time to finish this code in time.

Comment 6

[Rob Campbell [:rc] (:robcee)]

taking this while paul's away.

Comment 7

[Paul Rouget [:paul]]

Attachment: patch v0.3 WIP

Includes:
- OSX design
- sibling menu
- scrollbox

Missing:
- Windows and Linux designs
- ellipsis or fading-out text for long text
- tests tests tests

Comment 8

[Paul Rouget [:paul]]

Missing:
- RTL

Comment 9

[Paul Rouget [:paul]]

Attachment: patch Experimenting with fading-out text

Comment 10

[Paul Rouget [:paul]]

Attachment: Screenshot (v0.3 + fade-out)

Comment 11

[Paul Rouget [:paul]]

Missing:
- drop shadow (Shorlander will update the border-images)

Comment 12

[Paul Rouget [:paul]]

Instead of a scrollbox, we may want to use menus. See bug 683662 (screenhost inside).

Comment 13

[Paul Rouget [:paul]]

Designs updated:
http://cl.ly/0o1Q2a3t0K1m2N3j180h/o
http://cl.ly/251w1u2Q190x332z2l2V/o

Comment 15

[Paul Rouget [:paul]]

Attachment: patch: mechanism + osx design

Patch includes the breadcrumbs mechanism + osx theme.
To come:
- patch for windows theme
- patch for linux theme
- patch for the tests

Comment 16

[Paul Rouget [:paul]]

Attachment: Screenshot LTR

Comment 17

[Paul Rouget [:paul]]

Attachment: Screenshot RTL

Ehsan, can you look at this RTL screenshot? I don't think this is really usable. We may not want to have a reversed breadcrumbs for RTL since that would involve LTR selectors (I guess people don't write selectors in RTL) in a RTL breadcrumbs.

Comment 18

[Paul Rouget [:paul]]

Comment on attachment 557979 [details] [diff] [review]
patch: mechanism + osx design

Dao, the code in browser.css (only mac for now) is quite big and intrusive (see all the border-image code). Do you think we should move that somewhere else?

Also, do you think it's possible to use sprites for border-images?

Comment 19

[(no longer active)]

Comment on attachment 557982 [details]
Screenshot RTL

This looks good, except for the way that the selectors are displayed.  CSS Selectors are inherently LTR.  It's possible to use a classname including RTL characters, for example, but that should not affect the base flow direction for the selector.  For example, for a selector like this:

html#facebook p.ehsan > span

If I change the classname "ehsan" to "احسان", this is how the entire selector should be displayed:

html#facebook p.احسان > span

Whereas with the current patch, this is how the selector will be displayed:

span < احسان.p facebook#html

Which makes no sense!

In order to get the desired behavior, I think you want to set the direction property for .inspector-breadcrumbs-button to ltr.

Comment 20

[Dão Gottwald [:dao]]

(In reply to Ehsan Akhgari [:ehsan] from comment #19)
> Comment on attachment 557982 [details]
> Screenshot RTL
> 
> This looks good, except for the way that the selectors are displayed.  CSS
> Selectors are inherently LTR.  It's possible to use a classname including
> RTL characters, for example, but that should not affect the base flow
> direction for the selector.  For example, for a selector like this:
> 
> html#facebook p.ehsan > span
> 
> If I change the classname "ehsan" to "احسان", this is how the entire
> selector should be displayed:
> 
> html#facebook p.احسان > span
> 
> Whereas with the current patch, this is how the selector will be displayed:
> 
> span < احسان.p facebook#html
> 
> Which makes no sense!
> 
> In order to get the desired behavior, I think you want to set the direction
> property for .inspector-breadcrumbs-button to ltr.

This all would make sense if this was a selector. It's not a selector, though, it's breadcrumbs navigation. Interpreting the arrows as child combinators isn't even sane, as the resulting selector could select multiple nodes.

Comment 21

[Dão Gottwald [:dao]]

(In reply to Paul Rouget [:paul] from comment #18)
> Dao, the code in browser.css (only mac for now) is quite big and intrusive
> (see all the border-image code). Do you think we should move that somewhere
> else?

Ideally this would be completely separate from the browser window DOM and therefore from browser.css. Basically this would mean undoing bug 664436 and solving the mouse event problem differently.

Comment 22

[Paul Rouget [:paul]]

(In reply to Ehsan Akhgari [:ehsan] from comment #19)
> In order to get the desired behavior, I think you want to set the direction
> property for .inspector-breadcrumbs-button to ltr.

So we want arrows like that:

| … < …  < … < … < … |

with the content of each arrow in LTR. Right?

Comment 23

[Dão Gottwald [:dao]]

(In reply to Paul Rouget [:paul] from comment #22)
> (In reply to Ehsan Akhgari [:ehsan] from comment #19)
> > In order to get the desired behavior, I think you want to set the direction
> > property for .inspector-breadcrumbs-button to ltr.
> 
> So we want arrows like that:
> 
> | … < …  < … < … < … |
> 
> with the content of each arrow in LTR. Right?

I think so, but I don't know what the point of Ehsan's "html#facebook p.احسان > span" example was.

Comment 24

[Paul Rouget [:paul]]

Attachment: patch v1

Includes designs for the 3 platforms, and tests.

Comment 25

[Paul Rouget [:paul]]

Attachment: Screenshot RTL

Comment 26

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 559102 [details] [diff] [review]
patch v1

Inspector.js


I have a nit!

 +
 +    // initialize the HTML Breadcrumbs

extra line-break.

in prettyPrintNode,

+      let tagLabel = document.createElement("label");
+      tagLabel.className = "inspector-breadcrumbs-tag"

missing semi-colon here and a few lines down. (and then again)

You could break up prettyPrintNode into two methods here. One for plaintext and the other for the labeled, document fragment approach. Not strictly necessary, but an easy bit of refactoring.

openSiblingtMenu: function Breadcrumbs_open...
           ^- typo.

in handleEvent:

+      container.addEventListener("mouseup", handleClick, false);
+      timer = setTimeout(openMenu, 500, aEvent);

is that 500 a standard length for a "long click" event? Is there such a thing?

Might want to put it in a constant or even a preference.

(fwiw, I think it's a fine value)


minor grammatical quibble:
+  /**
+   * Make sure that the latest node in the breadcrumbs is not the selected node
+   * if the selected node still have children.

still _has_ children.

+   */
+  ensureFirstChild: function Breadcrumbs_ensureFirstChild()

applying this to a build to see how it feels.

Comment 27

[Rob Campbell [:rc] (:robcee)]

built locally and the sibling selection menu items don't seem to be working. Nothing in the Error Console.

Comment 28

[Dave Camp (:dcamp)]

Comment on attachment 559102 [details] [diff] [review]
patch v1


>+        item.onclick = (function(aNode) {
>+          return function() {
>+            InspectorUI.select(aNode, true, true);
>+          }
>+        })(nodes[i]);

onclick is only called with a mousedown+mouseup pair.  If you hold to get the menu, move to the sibling you want, and let go this doesn't work.

Comment 29

[Dave Camp (:dcamp)]

Would be nice if that menu showed on right click too, like the back button.

Comment 30

[(no longer active)]

Comment on attachment 559103 [details]
Screenshot RTL

Sorry, I wasn't CCed on this bug, so I missed the discussion.  Dao is right, the selector parts of my comment were irrelevant.  But maintaining the LTR base direction was valid, and according to this screenshot, this version of the patch achieves that, so rtl-r=me.

Comment 31

[Paul Rouget [:paul]]

Attachment: patch v1.1

(In reply to Dave Camp (:dcamp) from comment #28)
> Comment on attachment 559102 [details] [diff] [review] 
> onclick is only called with a mousedown+mouseup pair.  If you hold to get
> the menu, move to the sibling you want, and let go this doesn't work.

fixed.

(In reply to Dave Camp (:dcamp) from comment #29)
> Would be nice if that menu showed on right click too, like the back button.

done.

(In reply to Rob Campbell [:rc] (robcee) from comment #26)
> Comment on attachment 559102 [details] [diff] [review]
> patch v1
> 
> Inspector.js
> 
> 
> I have a nit!
> 
>  +
>  +    // initialize the HTML Breadcrumbs
> 
> extra line-break.

done.

> in prettyPrintNode,
> 
> +      let tagLabel = document.createElement("label");
> +      tagLabel.className = "inspector-breadcrumbs-tag"
> 
> missing semi-colon here and a few lines down. (and then again)

done.

> You could break up prettyPrintNode into two methods here. One for plaintext
> and the other for the labeled, document fragment approach. Not strictly
> necessary, but an easy bit of refactoring.

done.

> openSiblingtMenu: function Breadcrumbs_open...
>            ^- typo.

done.

> in handleEvent:
> 
> +      container.addEventListener("mouseup", handleClick, false);
> +      timer = setTimeout(openMenu, 500, aEvent);
> 
> is that 500 a standard length for a "long click" event? Is there such a
> thing?

Standard (same hard-coded value for the back/forward buttons).

Comment 32

[Paul Rouget [:paul]]

Todo: remove the resize on hover (Shorlander's comment).

Comment 33

[Paul Rouget [:paul]]

Attachment: patch v1.2

Addressed Shorlander's comments (removed the size-change on over).

Comment 34

[Paul Rouget [:paul]]

Comment on attachment 561161 [details] [diff] [review]
patch v1.2

Only made the changes for pinstripe. Will update the patch soon.

Comment 35

[Paul Rouget [:paul]]

Attachment: patch v1.3

Comment 36

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 561168 [details] [diff] [review]
patch v1.3

This needs a rebasing. Probably minor stuff, but...

patching file browser/base/content/inspector.js
Hunk #1 FAILED at 243
1 out of 6 hunks FAILED -- saving rejects to file browser/base/content/inspector.js.rej
patching file browser/base/content/test/inspector/Makefile.in
Hunk #1 FAILED at 52
1 out of 1 hunks FAILED -- saving rejects to file browser/base/content/test/inspector/Makefile.in.rej
patching file browser/themes/gnomestripe/browser/browser.css
Hunk #1 FAILED at 2032
1 out of 1 hunks FAILED -- saving rejects to file browser/themes/gnomestripe/browser/browser.css.rej
patching file browser/themes/winstripe/browser/browser.css
Hunk #1 FAILED at 2595
1 out of 1 hunks FAILED -- saving rejects to file browser/themes/winstripe/browser/browser.css.rej
patching file browser/themes/winstripe/browser/jar.mn
Hunk #2 FAILED at 247
1 out of 2 hunks FAILED -- saving rejects to file browser/themes/winstripe/browser/jar.mn.rej

in browser.xul:

       <toolbarseparator />
+      <arrowscrollbox id="inspector-breadcrumbs" orient="horizontal" clicktoscroll="true"/>

please break this up into multiple lines, with attributes on separate lines as in the surrounding area.

in inspector.js:

+    // We will save a list of already displayed node in this array.
+    this.nodeHierarchy = [];

// ... already displayed nodes - plural.

in prettyPrintNodeAsText:

+    text += aNode.id ? ("#" + aNode.id) : "";

you could break this into an if (aNode.id) condition and add the text only if present. Not critical, but it saves the unnecessary += operation.

+  prettyPrintNodeAsXML: function Breadcrumbs_prettyPrintNodeXML(aNode)

should this really be prettyPrintNodeAsXUL?

+    let tagLabel = document.createElement("label");

You could use createElementNS here instead.

in openSiblingMenu: function Breadcrumbs_openSiblingMenu(aButton, aNode)
+  {
+    let title = document.createElement("menuitem");

createElementNS again. We should use that for any xul document createElement calls, though I guess we're doing that elsewhere in this file already, so maybe it's no big deal.

nit (comment for cutAfter:):

+   * Remove all the buttons and their reference in the cache
+   * after a given index.

"their references"

nit:

+  getFirstHighlightableChild:
+    function Breadcrumbs_getFirstHighlightableChild(aNode)

Usually indent the second line level with the preceding one, as in:

+  getFirstHighlightableChild:
+  function Breadcrumbs_getFirstHighlightableChild(aNode)

You could also abbreviate Breadcrumbs_ as BC_ if you wanted to save space.

One thing I see is that you're not removing the popuphiding event from the breadcrumb's menu property. This is probably OK as the menu's getting removed on destroy(), but I'd like to see this take a run through the try server's debug machinery to make sure their aren't any leaks.

R+ with a successful run.

Thanks! Works great, btw.

Comment 37

[Paul Rouget [:paul]]

(In reply to Rob Campbell [:rc] (robcee) from comment #36)
> Comment on attachment 561168 [details] [diff] [review] [diff] [details] [review]
> patch v1.3
> 
> This needs a rebasing. Probably minor stuff, but...
> 
> patching file browser/base/content/inspector.js
> Hunk #1 FAILED at 243
> 1 out of 6 hunks FAILED -- saving rejects to file
> browser/base/content/inspector.js.rej
> patching file browser/base/content/test/inspector/Makefile.in
> Hunk #1 FAILED at 52
> 1 out of 1 hunks FAILED -- saving rejects to file
> browser/base/content/test/inspector/Makefile.in.rej
> patching file browser/themes/gnomestripe/browser/browser.css
> Hunk #1 FAILED at 2032
> 1 out of 1 hunks FAILED -- saving rejects to file
> browser/themes/gnomestripe/browser/browser.css.rej
> patching file browser/themes/winstripe/browser/browser.css
> Hunk #1 FAILED at 2595
> 1 out of 1 hunks FAILED -- saving rejects to file
> browser/themes/winstripe/browser/browser.css.rej
> patching file browser/themes/winstripe/browser/jar.mn
> Hunk #2 FAILED at 247
> 1 out of 2 hunks FAILED -- saving rejects to file
> browser/themes/winstripe/browser/jar.mn.rej
> 
> in browser.xul:
> 
>        <toolbarseparator />
> +      <arrowscrollbox id="inspector-breadcrumbs" orient="horizontal"
> clicktoscroll="true"/>
> 
> please break this up into multiple lines, with attributes on separate lines
> as in the surrounding area.
> 
> in inspector.js:
> 
> +    // We will save a list of already displayed node in this array.
> +    this.nodeHierarchy = [];
> 
> // ... already displayed nodes - plural.
> 
> in prettyPrintNodeAsText:
> 
> +    text += aNode.id ? ("#" + aNode.id) : "";
> 
> you could break this into an if (aNode.id) condition and add the text only
> if present. Not critical, but it saves the unnecessary += operation.
> 
> +  prettyPrintNodeAsXML: function Breadcrumbs_prettyPrintNodeXML(aNode)
> 
> should this really be prettyPrintNodeAsXUL?
> 
> +    let tagLabel = document.createElement("label");
> 
> You could use createElementNS here instead.
> 
> in openSiblingMenu: function Breadcrumbs_openSiblingMenu(aButton, aNode)
> +  {
> +    let title = document.createElement("menuitem");
> 
> createElementNS again. We should use that for any xul document createElement
> calls, though I guess we're doing that elsewhere in this file already, so
> maybe it's no big deal.
> 
> nit (comment for cutAfter:):
> 
> +   * Remove all the buttons and their reference in the cache
> +   * after a given index.
> 
> "their references"
> 
> nit:
> 
> +  getFirstHighlightableChild:
> +    function Breadcrumbs_getFirstHighlightableChild(aNode)
> 
> Usually indent the second line level with the preceding one, as in:
> 
> +  getFirstHighlightableChild:
> +  function Breadcrumbs_getFirstHighlightableChild(aNode)
> 
> You could also abbreviate Breadcrumbs_ as BC_ if you wanted to save space.
> 
> One thing I see is that you're not removing the popuphiding event from the
> breadcrumb's menu property. This is probably OK as the menu's getting
> removed on destroy(), but I'd like to see this take a run through the try
> server's debug machinery to make sure their aren't any leaks.
> 
> R+ with a successful run.
> 
> Thanks! Works great, btw.

Thanks for the review. I'll address your comments (I first need to take care of the different themes).

For the tests, looks good: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=11d48244b76c (beside an unrelated orange)

Comment 38

[Paul Rouget [:paul]]

Attachment: patch v1.4

Comment 39

[Paul Rouget [:paul]]

Attachment: patch v2.1

- rebased
- removed the separator
- now locks the inspector once a button is click

Comment 40

[Paul Rouget [:paul]]

Comment on attachment 564574 [details] [diff] [review]
patch v2.1

Cancelling the review. I found a bug (firstChild not always displayed) and the tests don't pass anymore.

Comment 41

[Paul Rouget [:paul]]

Attachment: patch v2.2

rebased

Comment 42

[Dão Gottwald [:dao]]

Comment on attachment 565915 [details] [diff] [review]
patch v2.2

>--- a/browser/themes/gnomestripe/browser/browser.css
>+++ b/browser/themes/gnomestripe/browser/browser.css

>+#inspector-breadcrumbs {
>+  padding: 0 6px;
>+  overflow: auto;

What exactly is overflow:auto doing here?

>+  -moz-box-flex: 1;

nit: replace with flex="1" in the xul markup

>+  margin-bottom: -1px;

Can you add a comment on what this is doing?

>+.inspector-breadcrumbs-button {

>+  overflow: hidden;

ditto

>+  margin: 0 -11px 0 0;

Is this correct for RTL?

>+/* Highlighter toolbar - breadcrumbs - LTR */
>+
>+.inspector-breadcrumbs-button:first-of-type {
>+  margin-left: 0;
>+}

>+/* Highlighter toolbar - breadcrumbs - RTL */
>+
>+.inspector-breadcrumbs-button:-moz-locale-dir(rtl):first-of-type {
>+  margin-right: 0;
>+}

This looks wrong for RTL, as you're setting margin-left to 0 regardless of the locale direction.

Comment 43

[Paul Rouget [:paul]]

(In reply to Dão Gottwald [:dao] from comment #42)
> >+#inspector-breadcrumbs {
> >+  padding: 0 6px;
> >+  overflow: auto;
> 
> What exactly is overflow:auto doing here?

Nothing. I'll remove that.
 
> >+  -moz-box-flex: 1;
> 
> nit: replace with flex="1" in the xul markup

Ok.

> >+  margin-bottom: -1px;
> 
> Can you add a comment on what this is doing?
> 
> >+.inspector-breadcrumbs-button {
> 
> >+  overflow: hidden;
> 
> ditto

Ok.

> >+  margin: 0 -11px 0 0;
> 
> Is this correct for RTL?

Yes. The direction of the button is force to rtl (in highligher.css).

> >+/* Highlighter toolbar - breadcrumbs - LTR */
> >+
> >+.inspector-breadcrumbs-button:first-of-type {
> >+  margin-left: 0;
> >+}
> 
> >+/* Highlighter toolbar - breadcrumbs - RTL */
> >+
> >+.inspector-breadcrumbs-button:-moz-locale-dir(rtl):first-of-type {
> >+  margin-right: 0;
> >+}
> 
> This looks wrong for RTL, as you're setting margin-left to 0 regardless of
> the locale direction.

Right.

Comment 44

[Paul Rouget [:paul]]

Attachment: patch v2.3

Addressed Dao's comments.

Comment 45

[Rob Campbell [:rc] (:robcee)]

I added the following to fix up the toolbar spacing:

     1.1 --- a/browser/base/content/browser.xul
     1.2 +++ b/browser/base/content/browser.xul
     1.3 @@ -982,20 +982,21 @@
     1.4            <toolbarbutton id="inspector-inspect-toolbutton"
     1.5                           label="&inspectButton.label;"
     1.6                           accesskey="&inspectButton.accesskey;"
     1.7                           command="Inspector:Inspect"/>
     1.8            <toolbarseparator />
     1.9            <arrowscrollbox id="inspector-breadcrumbs"
    1.10                            flex="1" orient="horizontal"
    1.11                            clicktoscroll="true"/>
    1.12 -          <hbox id="inspector-tools">
    1.13 +          <toolbarseparator />
    1.14 +          <hbox id="inspector-tools"
    1.15 +		align="end">
    1.16              <!-- registered tools go here -->
    1.17            </hbox>
    1.18 -          <spacer flex="1"/>
    1.19            <resizer id="inspector-end-resizer"
    1.20                     class="inspector-resizer"
    1.21                     dir="top" disabled="true"
    1.22                     element="inspector-tree-box"/>
    1.23          </hbox>
    1.24        </vbox>
    1.25      </toolbar>
    1.26      <toolbar id="addon-bar"

https://hg.mozilla.org/projects/devtools/rev/6dc539d8b0c4

Comment 46

[Paul Rouget [:paul]]

(In reply to Rob Campbell [:rc] (robcee) from comment #45)
> I added the following to fix up the toolbar spacing:
>     1.12 -          <hbox id="inspector-tools">
>     1.13 +          <toolbarseparator />
>     1.14 +          <hbox id="inspector-tools"
>     1.15 +		align="end">
>     1.16              <!-- registered tools go here -->
>     1.17            </hbox>

Sorry, but I don't understand what this is supposed to change. The `inspector-tools` hbox won't expand anyway. `align="end"` should not be different from `align="start"`.

Comment 47

[Paul Rouget [:paul]]

Attachment: patch v2.4

rebased

Comment 48

[Rob Campbell [:rc] (:robcee)]

(In reply to Paul Rouget [:paul] from comment #46)
> (In reply to Rob Campbell [:rc] (robcee) from comment #45)
> > I added the following to fix up the toolbar spacing:
> >     1.12 -          <hbox id="inspector-tools">
> >     1.13 +          <toolbarseparator />
> >     1.14 +          <hbox id="inspector-tools"
> >     1.15 +		align="end">
> >     1.16              <!-- registered tools go here -->
> >     1.17            </hbox>
> 
> Sorry, but I don't understand what this is supposed to change. The
> `inspector-tools` hbox won't expand anyway. `align="end"` should not be
> different from `align="start"`.

does the breadcrumb display position things correctly without it?

I guess align="end" describes the positioning of the hbox' child-elements so if the arrowbox is filling up the toolbar, you're right that it shouldn't be needed.

Comment 49

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/fc67f7acac80

Comment 50

[Rob Campbell [:rc] (:robcee)]

repushed https://hg.mozilla.org/integration/fx-team/rev/449e7eb936cd

Comment 51

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/449e7eb936cd

https://hg.mozilla.org/mozilla-central/rev/322354df233d

Comment 52

[Teodosia Pop]

Verified fixed using
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0a2) Gecko/20111124 Firefox/10.0a2
and
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a2) Gecko/20111123 Firefox/10.0a2