The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 10

Status

()

Firefox
Developer Tools
P1
normal
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: rc, Assigned: paul)

Tracking

Trunk
Firefox 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [minotaur][best: 3d; likely: 1w; worst: 2w][testday-20111125])

Attachments

(5 attachments, 13 obsolete attachments)

4.75 KB, patch
Details | Diff | Splinter Review
73.92 KB, image/png
Details
35.41 KB, image/png
Ehsan
: review+
Details
156.55 KB, patch
dao
: review+
Details | Diff | Splinter Review
156.94 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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

Updated

6 years ago
Whiteboard: [minotaur]
(Assignee)

Updated

6 years ago
Assignee: nobody → paul
(Reporter)

Comment 1

6 years ago
We spoke to Frank Yan briefly about this. We need to loop back and ask him what he thinks. CC'ing.
Whiteboard: [minotaur] → [minotaur][best: 3d; likely: 1w; worst: 2w]
(Reporter)

Comment 2

6 years ago
Paul, could you put up your WIP patch here, please?
(Assignee)

Comment 3

6 years ago
Created attachment 550517 [details] [diff] [review]
patch v0.1 WIP
(Assignee)

Comment 4

6 years ago
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
Attachment #550517 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Assignee: paul → nobody
(Assignee)

Comment 5

6 years ago
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.
(Reporter)

Comment 6

6 years ago
taking this while paul's away.
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED

Updated

6 years ago
Assignee: rcampbell → mihai.sucan

Updated

6 years ago
Priority: -- → P1
(Reporter)

Updated

6 years ago
Blocks: 680977

Updated

6 years ago
Assignee: mihai.sucan → paul
(Assignee)

Updated

6 years ago
Depends on: 676253
(Assignee)

Comment 7

6 years ago
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
Attachment #550822 - Attachment is obsolete: true
(Assignee)

Comment 8

6 years ago
Missing:
- RTL
(Assignee)

Comment 9

6 years ago
Created attachment 557322 [details] [diff] [review]
patch Experimenting with fading-out text
(Assignee)

Comment 10

6 years ago
Created attachment 557323 [details]
Screenshot (v0.3 + fade-out)
(Assignee)

Comment 11

6 years ago
Missing:
- drop shadow (Shorlander will update the border-images)
(Assignee)

Comment 12

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

Comment 13

6 years ago
Designs updated:
http://cl.ly/0o1Q2a3t0K1m2N3j180h/o
http://cl.ly/251w1u2Q190x332z2l2V/o
(Assignee)

Updated

6 years ago
Depends on: 683906
No longer depends on: 663830
(Assignee)

Updated

6 years ago
Depends on: 676255

Updated

6 years ago
Duplicate of this bug: 680977
(Assignee)

Updated

6 years ago
Blocks: 684352
(Assignee)

Comment 15

6 years ago
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
Attachment #557320 - Attachment is obsolete: true
Attachment #557979 - Flags: review?(rcampbell)
(Assignee)

Comment 16

6 years ago
Created attachment 557981 [details]
Screenshot LTR
Attachment #557323 - Attachment is obsolete: true
(Assignee)

Comment 17

6 years ago
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.
Attachment #557982 - Flags: review?(ehsan)
(Assignee)

Comment 18

6 years ago
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?
Attachment #557979 - Flags: review?(dao)
(Assignee)

Updated

6 years ago
Blocks: 683662
(Assignee)

Updated

6 years ago
Blocks: 684398
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.
Attachment #557982 - Flags: review?(ehsan) → review-
(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.
(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.
(Assignee)

Comment 22

6 years ago
(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?
(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.
(Assignee)

Comment 24

6 years ago
Created attachment 559102 [details] [diff] [review]
patch v1

Includes designs for the 3 platforms, and tests.
Attachment #557979 - Attachment is obsolete: true
Attachment #559102 - Flags: review?(rcampbell)
Attachment #557979 - Flags: review?(rcampbell)
Attachment #557979 - Flags: review?(dao)
(Assignee)

Comment 25

6 years ago
Created attachment 559103 [details]
Screenshot RTL
Attachment #557982 - Attachment is obsolete: true
Attachment #559103 - Flags: review?(ehsan)
(Reporter)

Comment 26

6 years ago
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.
(Reporter)

Comment 27

6 years ago
built locally and the sibling selection menu items don't seem to be working. Nothing in the Error Console.
Priority: P1 → --
Target Milestone: --- → Firefox 8
Version: unspecified → Trunk

Comment 28

6 years ago
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

6 years ago
Would be nice if that menu showed on right click too, like the back button.
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.
Attachment #559103 - Flags: review?(ehsan) → review+
(Assignee)

Comment 31

6 years ago
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).
Attachment #559102 - Attachment is obsolete: true
Attachment #560437 - Flags: review?(rcampbell)
Attachment #559102 - Flags: review?(rcampbell)
(Assignee)

Comment 32

6 years ago
Todo: remove the resize on hover (Shorlander's comment).
(Assignee)

Comment 33

6 years ago
Created attachment 561161 [details] [diff] [review]
patch v1.2

Addressed Shorlander's comments (removed the size-change on over).
Attachment #560437 - Attachment is obsolete: true
Attachment #561161 - Flags: review?(rcampbell)
Attachment #560437 - Flags: review?(rcampbell)
(Assignee)

Comment 34

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

Only made the changes for pinstripe. Will update the patch soon.
Attachment #561161 - Attachment is obsolete: true
Attachment #561161 - Flags: review?(rcampbell)
(Assignee)

Comment 35

6 years ago
Created attachment 561168 [details] [diff] [review]
patch v1.3
Attachment #561168 - Flags: review?(rcampbell)
(Reporter)

Comment 36

6 years ago
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.
Attachment #561168 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 37

6 years ago
(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)
(Assignee)

Updated

6 years ago
Depends on: 690713
(Assignee)

Comment 38

6 years ago
Created attachment 563779 [details] [diff] [review]
patch v1.4
Attachment #561168 - Attachment is obsolete: true
Attachment #563779 - Flags: review?(dao)
(Assignee)

Updated

6 years ago
Depends on: 691712
(Assignee)

Comment 39

6 years ago
Created attachment 564574 [details] [diff] [review]
patch v2.1

- rebased
- removed the separator
- now locks the inspector once a button is click
Attachment #563779 - Attachment is obsolete: true
Attachment #564574 - Flags: review?(dao)
Attachment #563779 - Flags: review?(dao)
(Assignee)

Comment 40

6 years ago
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.
Attachment #564574 - Flags: review?(dao)
(Assignee)

Comment 41

6 years ago
Created attachment 565915 [details] [diff] [review]
patch v2.2

rebased
Attachment #564574 - Attachment is obsolete: true
Attachment #565915 - Flags: review?(dao)
(Assignee)

Updated

6 years ago
Blocks: 663830
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.
(Assignee)

Comment 43

6 years ago
(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.
(Assignee)

Comment 44

6 years ago
Created attachment 565946 [details] [diff] [review]
patch v2.3

Addressed Dao's comments.
Attachment #565915 - Attachment is obsolete: true
Attachment #565946 - Flags: review?(dao)
Attachment #565915 - Flags: review?(dao)
(Reporter)

Comment 45

6 years ago
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
Priority: -- → P1
Target Milestone: Firefox 8 → Firefox 9

Updated

6 years ago
Attachment #565946 - Flags: review?(dao) → review+
(Assignee)

Comment 46

6 years ago
(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"`.
(Assignee)

Comment 47

6 years ago
Created attachment 568634 [details] [diff] [review]
patch v2.4

rebased
(Assignee)

Updated

6 years ago
Blocks: 683873
(Reporter)

Comment 48

6 years ago
(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.
(Reporter)

Comment 49

6 years ago
https://hg.mozilla.org/integration/fx-team/rev/fc67f7acac80
Assignee: paul → nobody
Whiteboard: [minotaur][best: 3d; likely: 1w; worst: 2w] → [minotaur][best: 3d; likely: 1w; worst: 2w][fixed-in-fx-team]
Target Milestone: Firefox 9 → ---
(Reporter)

Comment 50

6 years ago
repushed https://hg.mozilla.org/integration/fx-team/rev/449e7eb936cd
(Reporter)

Comment 51

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

https://hg.mozilla.org/mozilla-central/rev/322354df233d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10

Updated

6 years ago
Assignee: nobody → paul
Whiteboard: [minotaur][best: 3d; likely: 1w; worst: 2w][fixed-in-fx-team] → [minotaur][best: 3d; likely: 1w; worst: 2w]

Comment 52

5 years ago
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
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: FIXED → ---
Whiteboard: [minotaur][best: 3d; likely: 1w; worst: 2w] → [minotaur][best: 3d; likely: 1w; worst: 2w][testday-20111125]

Updated

5 years ago
Status: UNCONFIRMED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.