Last Comment Bug 672006 - [highlighter] Selecting a Node should update the Highlighter's main toolbar with a breadcrumb display
: [highlighter] Selecting a Node should update the Highlighter's main toolbar w...
Status: RESOLVED FIXED
[minotaur][best: 3d; likely: 1w; wors...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: Firefox 10
Assigned To: Paul Rouget [:paul]
:
Mentors:
: 680977 (view as bug list)
Depends on: 666650 676253 676255 683906 690713 691712
Blocks: 663830 680977 683662 683873 684352 684398
  Show dependency treegraph
 
Reported: 2011-07-15 17:24 PDT by Rob Campbell [:rc] (:robcee)
Modified: 2013-11-12 00:57 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v0.1 WIP (10.38 KB, patch)
2011-08-03 14:35 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v0.2 WIP (14.67 KB, patch)
2011-08-04 13:50 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v0.3 WIP (37.38 KB, patch)
2011-08-31 14:20 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch Experimenting with fading-out text (4.75 KB, patch)
2011-08-31 14:22 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
Screenshot (v0.3 + fade-out) (116.32 KB, image/png)
2011-08-31 14:24 PDT, Paul Rouget [:paul]
no flags Details
patch: mechanism + osx design (64.21 KB, patch)
2011-09-02 16:12 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
Screenshot LTR (73.92 KB, image/png)
2011-09-02 16:15 PDT, Paul Rouget [:paul]
no flags Details
Screenshot RTL (58.02 KB, image/png)
2011-09-02 16:18 PDT, Paul Rouget [:paul]
ehsan: review-
Details
patch v1 (156.12 KB, patch)
2011-09-08 04:24 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
Screenshot RTL (35.41 KB, image/png)
2011-09-08 04:25 PDT, Paul Rouget [:paul]
ehsan: review+
Details
patch v1.1 (156.49 KB, patch)
2011-09-15 13:21 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.2 (156.21 KB, patch)
2011-09-20 04:08 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1.3 (155.96 KB, patch)
2011-09-20 05:37 PDT, Paul Rouget [:paul]
rcampbell: review+
Details | Diff | Splinter Review
patch v1.4 (156.42 KB, patch)
2011-09-30 11:05 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v2.1 (156.56 KB, patch)
2011-10-04 09:11 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v2.2 (155.75 KB, patch)
2011-10-10 05:52 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v2.3 (156.55 KB, patch)
2011-10-10 08:40 PDT, Paul Rouget [:paul]
dao+bmo: review+
Details | Diff | Splinter Review
patch v2.4 (156.94 KB, patch)
2011-10-21 05:01 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review

Description Rob Campbell [:rc] (:robcee) 2011-07-15 17:24:58 PDT
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) 2011-07-25 17:32:13 PDT
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) 2011-08-03 13:58:05 PDT
Paul, could you put up your WIP patch here, please?
Comment 3 Paul Rouget [:paul] 2011-08-03 14:35:06 PDT
Created attachment 550517 [details] [diff] [review]
patch v0.1 WIP
Comment 4 Paul Rouget [:paul] 2011-08-04 13:50:05 PDT
Created attachment 550822 [details] [diff] [review]
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] 2011-08-04 13:54:39 PDT
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) 2011-08-05 06:10:03 PDT
taking this while paul's away.
Comment 7 Paul Rouget [:paul] 2011-08-31 14:20:36 PDT
Created attachment 557320 [details] [diff] [review]
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] 2011-08-31 14:21:39 PDT
Missing:
- RTL
Comment 9 Paul Rouget [:paul] 2011-08-31 14:22:45 PDT
Created attachment 557322 [details] [diff] [review]
patch Experimenting with fading-out text
Comment 10 Paul Rouget [:paul] 2011-08-31 14:24:28 PDT
Created attachment 557323 [details]
Screenshot (v0.3 + fade-out)
Comment 11 Paul Rouget [:paul] 2011-08-31 14:25:46 PDT
Missing:
- drop shadow (Shorlander will update the border-images)
Comment 12 Paul Rouget [:paul] 2011-08-31 14:27:06 PDT
Instead of a scrollbox, we may want to use menus. See bug 683662 (screenhost inside).
Comment 13 Paul Rouget [:paul] 2011-09-02 03:05:30 PDT
Designs updated:
http://cl.ly/0o1Q2a3t0K1m2N3j180h/o
http://cl.ly/251w1u2Q190x332z2l2V/o
Comment 14 Dave Camp (:dcamp) 2011-09-02 14:13:36 PDT
*** Bug 680977 has been marked as a duplicate of this bug. ***
Comment 15 Paul Rouget [:paul] 2011-09-02 16:12:29 PDT
Created attachment 557979 [details] [diff] [review]
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] 2011-09-02 16:15:40 PDT
Created attachment 557981 [details]
Screenshot LTR
Comment 17 Paul Rouget [:paul] 2011-09-02 16:18:03 PDT
Created attachment 557982 [details]
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] 2011-09-02 16:20:17 PDT
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 :Ehsan Akhgari 2011-09-05 12:17:30 PDT
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] 2011-09-05 12:31:01 PDT
(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] 2011-09-05 12:34:12 PDT
(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] 2011-09-06 01:47:49 PDT
(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] 2011-09-06 01:57:47 PDT
(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] 2011-09-08 04:24:22 PDT
Created attachment 559102 [details] [diff] [review]
patch v1

Includes designs for the 3 platforms, and tests.
Comment 25 Paul Rouget [:paul] 2011-09-08 04:25:28 PDT
Created attachment 559103 [details]
Screenshot RTL
Comment 26 Rob Campbell [:rc] (:robcee) 2011-09-08 15:30:59 PDT
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) 2011-09-08 17:56:30 PDT
built locally and the sibling selection menu items don't seem to be working. Nothing in the Error Console.
Comment 28 Dave Camp (:dcamp) 2011-09-08 18:22:50 PDT
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) 2011-09-08 18:25:23 PDT
Would be nice if that menu showed on right click too, like the back button.
Comment 30 :Ehsan Akhgari 2011-09-08 19:21:20 PDT
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] 2011-09-15 13:21:11 PDT
Created attachment 560437 [details] [diff] [review]
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] 2011-09-16 13:05:09 PDT
Todo: remove the resize on hover (Shorlander's comment).
Comment 33 Paul Rouget [:paul] 2011-09-20 04:08:04 PDT
Created attachment 561161 [details] [diff] [review]
patch v1.2

Addressed Shorlander's comments (removed the size-change on over).
Comment 34 Paul Rouget [:paul] 2011-09-20 04:44:29 PDT
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] 2011-09-20 05:37:03 PDT
Created attachment 561168 [details] [diff] [review]
patch v1.3
Comment 36 Rob Campbell [:rc] (:robcee) 2011-09-20 07:20:44 PDT
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] 2011-09-23 09:40:22 PDT
(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] 2011-09-30 11:05:27 PDT
Created attachment 563779 [details] [diff] [review]
patch v1.4
Comment 39 Paul Rouget [:paul] 2011-10-04 09:11:55 PDT
Created attachment 564574 [details] [diff] [review]
patch v2.1

- rebased
- removed the separator
- now locks the inspector once a button is click
Comment 40 Paul Rouget [:paul] 2011-10-06 08:58:14 PDT
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] 2011-10-10 05:52:45 PDT
Created attachment 565915 [details] [diff] [review]
patch v2.2

rebased
Comment 42 Dão Gottwald [:dao] 2011-10-10 07:42:22 PDT
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] 2011-10-10 08:38:02 PDT
(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] 2011-10-10 08:40:55 PDT
Created attachment 565946 [details] [diff] [review]
patch v2.3

Addressed Dao's comments.
Comment 45 Rob Campbell [:rc] (:robcee) 2011-10-11 20:01:28 PDT
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] 2011-10-21 04:52:15 PDT
(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] 2011-10-21 05:01:45 PDT
Created attachment 568634 [details] [diff] [review]
patch v2.4

rebased
Comment 48 Rob Campbell [:rc] (:robcee) 2011-10-21 05:22:10 PDT
(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) 2011-10-27 07:28:00 PDT
https://hg.mozilla.org/integration/fx-team/rev/fc67f7acac80
Comment 50 Rob Campbell [:rc] (:robcee) 2011-10-27 08:38:43 PDT
repushed https://hg.mozilla.org/integration/fx-team/rev/449e7eb936cd
Comment 52 Teodosia Pop 2011-11-25 08:13:41 PST
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

Note You need to log in before you can comment on or make changes to this bug.