Last Comment Bug 663833 - [highlighter] Add a floating toolbar next to the selected node
: [highlighter] Add a floating toolbar next to the selected node
Status: VERIFIED FIXED
[minotaur][fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: x86 Linux
: P1 normal (vote)
: Firefox 10
Assigned To: Paul Rouget [:paul]
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on:
Blocks: 663830 672003
  Show dependency treegraph
 
Reported: 2011-06-13 08:36 PDT by Paul Rouget [:paul]
Modified: 2011-10-07 02:44 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v0.1 WIP (9.62 KB, patch)
2011-08-03 07:40 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
screenshot: normal view (229.94 KB, image/png)
2011-08-03 07:42 PDT, Paul Rouget [:paul]
no flags Details
screenshot: if no space on top (194.37 KB, image/png)
2011-08-03 07:43 PDT, Paul Rouget [:paul]
no flags Details
screenshot: if no space arround the node (253.43 KB, image/png)
2011-08-03 07:44 PDT, Paul Rouget [:paul]
no flags Details
screenshot: if node small (69.83 KB, image/png)
2011-08-03 07:45 PDT, Paul Rouget [:paul]
no flags Details
patch v1 (18.50 KB, patch)
2011-08-05 07:41 PDT, Paul Rouget [:paul]
shorlander: ui‑review-
mihai.sucan: feedback+
Details | Diff | Splinter Review
screenshot: if node small (23.10 KB, image/png)
2011-08-05 07:41 PDT, Paul Rouget [:paul]
no flags Details
InfoBar Colors/Thoughts (626.93 KB, image/jpeg)
2011-08-19 14:40 PDT, Stephen Horlander [:shorlander]
no flags Details
patch v2.2 - WIP (no test) (18.48 KB, patch)
2011-08-31 06:13 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
Screenshot (v2.2) (241.95 KB, image/png)
2011-08-31 06:23 PDT, Paul Rouget [:paul]
no flags Details
patch v2.3 - fix gradients - WIP (no test) (18.23 KB, patch)
2011-08-31 07:03 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v3 (18.54 KB, patch)
2011-09-02 07:28 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v3.1 (with test) (22.67 KB, patch)
2011-09-02 11:33 PDT, Paul Rouget [:paul]
mihai.sucan: review+
Details | Diff | Splinter Review
patch v3.2 (25.95 KB, patch)
2011-09-02 13:05 PDT, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch v3.3 (25.97 KB, patch)
2011-09-13 11:48 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v3.4 (26.13 KB, patch)
2011-09-15 11:03 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v3.6 (25.85 KB, patch)
2011-09-16 12:22 PDT, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch v3.7 (26.00 KB, patch)
2011-09-20 04:23 PDT, Paul Rouget [:paul]
dao+bmo: review-
Details | Diff | Splinter Review
patch v3.8 (25.87 KB, patch)
2011-09-20 05:12 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v3.9 (25.54 KB, patch)
2011-09-21 08:23 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v3.10 (25.44 KB, patch)
2011-09-21 11:23 PDT, Paul Rouget [:paul]
dao+bmo: review+
Details | Diff | Splinter Review
patch v3.11 (25.38 KB, patch)
2011-09-21 12:52 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v3.11 - rebased (25.29 KB, patch)
2011-09-27 04:37 PDT, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v3.12 - rebased (25.36 KB, patch)
2011-09-27 04:51 PDT, Paul Rouget [:paul]
mihai.sucan: review+
Details | Diff | Splinter Review
[in-fx-team] patch v3.13 (25.03 KB, patch)
2011-09-27 09:16 PDT, Paul Rouget [:paul]
mihai.sucan: review+
Details | Diff | Splinter Review

Description Paul Rouget [:paul] 2011-06-13 08:36:06 PDT
While selecting nodes, showing the tag name, the id and the classes of the hovered nodes can be useful.

2 options:
× a floating div following the node
× a fixed div at the bottom or the top of the iframe
Comment 1 Paul Rouget [:paul] 2011-06-13 09:08:22 PDT
Kevin suggested to add the numeric info layout as well (margin sizes & co).
Comment 2 Paul Rouget [:paul] 2011-06-20 06:15:31 PDT
The toolbar could include:
× tag name, id and classes
× sizes
× developer tools buttons (see bug 660606)
× highlighter specific buttons
  . toggle dark background
  . toggle layout information
  . unlock node
Comment 3 Paul Rouget [:paul] 2011-06-22 09:06:28 PDT
We think that the "developer tools buttons" should lie in a "global toolbar".

The toolbar could include:
× tag name, id and classes
× dimensions (if the "layout info" are visible)
× highlighter specific buttons
  . toggle dark background
  . toggle layout information
  . unlock node
Comment 4 Rob Campbell [:rc] (:robcee) 2011-06-23 10:59:30 PDT
created bug 666650 (bitchin' bug number) for the global toolbar.
Comment 5 Rob Campbell [:rc] (:robcee) 2011-06-23 11:00:15 PDT
I think the global toolbar might include a toggle for the dark background. What do you think?
Comment 6 Paul Rouget [:paul] 2011-06-26 10:25:50 PDT
Makes sense. It's node related to the node itself.
Comment 7 Paul Rouget [:paul] 2011-06-26 10:49:13 PDT
s/node/not/
Comment 8 Paul Rouget [:paul] 2011-06-29 02:26:24 PDT
× tag name, id and classes
× dimensions and position (if the "layout info" are visible)
× highlighter specific buttons
  . toggle layout information
  . unlock node
Comment 9 Rob Campbell [:rc] (:robcee) 2011-06-29 08:25:16 PDT
if you're wanting for space, we can (and maybe should) move "unlock node" and toggle layout into the global toolbar. Currently, the "Inspect" button in there acts as a lock/unlock toggle, so I'm not sure we need an additional unlock button. Then again, it might be nice to have one close to the node itself.

What do you think?
Comment 10 Paul Rouget [:paul] 2011-07-06 05:16:42 PDT
After different discussion, this toolbar will only include:
× tag name, id and classes
× maybe dimensions

Other buttons should be moved to the inspector toolbar (aka global-toolbar here).
We could also consider having an unlock button on the highlighted node directly.
Comment 11 Paul Rouget [:paul] 2011-07-15 15:05:36 PDT
So I've been experimenting with 2 approaches:
× a <panel>
× a position:absolute div 

The problem with a position:absolute div is that we have to deal with the window overflow.

The problem with the panel is that it can't be animated with CSS and the resize is buggy (since the content of the panel change often, the panel is resized very often. Sometimes, the panel stop resizing. I didn't determine yet what causes this bug).

So far, I feel like the position:absolute div is the easiest way to do that.
Comment 12 Rob Campbell [:rc] (:robcee) 2011-07-15 16:59:52 PDT
> So far, I feel like the position:absolute div is the easiest way to do that.

I think you're probably right.
Comment 13 Rob Campbell [:rc] (:robcee) 2011-07-25 17:26:39 PDT
Paul, could you attach your WIP patch to this bug please? Thanks!
Comment 14 Paul Rouget [:paul] 2011-08-03 07:40:49 PDT
Created attachment 550380 [details] [diff] [review]
patch v0.1 WIP

Using absolute positioning.
Comment 15 Paul Rouget [:paul] 2011-08-03 07:42:35 PDT
Created attachment 550381 [details]
screenshot: normal view
Comment 16 Paul Rouget [:paul] 2011-08-03 07:43:20 PDT
Created attachment 550382 [details]
screenshot: if no space on top
Comment 17 Paul Rouget [:paul] 2011-08-03 07:44:12 PDT
Created attachment 550383 [details]
screenshot: if no space arround the node
Comment 18 Paul Rouget [:paul] 2011-08-03 07:45:02 PDT
Created attachment 550384 [details]
screenshot: if node small
Comment 19 Paul Rouget [:paul] 2011-08-03 07:46:23 PDT
We may want to do not use "<>" around the tag name to keep the text as a valid selector.
Comment 20 Paul Rouget [:paul] 2011-08-03 08:40:46 PDT
Comment on attachment 550380 [details] [diff] [review]
patch v0.1 WIP

Please check the different attached screenshot. Also, this patch involve animations. You may want to apply this patch to check this as well.

My main concerns are about:
. font-family, which one?
. borders (I can't use a shadow)
. color for classes
. horizontal alignment of the infobar
. shape of the arrows when the node is small
Comment 21 Paul Rouget [:paul] 2011-08-05 07:41:15 PDT
Created attachment 551035 [details] [diff] [review]
patch v1

Mihai, the design is not clearly defined yet. Please focus on the "mechanic".

Dao, can you please just take a look at this patch and confirm that there's no better way to do build and animate this infobar? I could have used a panel, but the fact that we can't animate them and that the positioning is hard to control made me choose a position:absolute box. Once I got a r+ from Mihai, I'll ask you for a real review. Thanks.

Stephen, please check comment 20. I've updated the screenshot.
Comment 22 Paul Rouget [:paul] 2011-08-05 07:41:59 PDT
Created attachment 551036 [details]
screenshot: if node small
Comment 23 Paul Rouget [:paul] 2011-08-05 07:52:10 PDT
Comment on attachment 551035 [details] [diff] [review]
patch v1

Canceling review, no tests. Mihai, can you take a look at the patch and tell me how I can test this feature?
Comment 24 Mihai Sucan [:msucan] 2011-08-05 09:20:43 PDT
Comment on attachment 551035 [details] [diff] [review]
patch v1

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

The patch looks fine, it's definitely in the right direction. Feedback+!

Also, the UI is awesome! This looks very, very good! Great work!

General comments:

- In Highlighter.destroy() please clean up the nodeinfo stuff. This code leaks nodes.
- Some tests are needed for this new functionality. You can make a test that highlights several elements that have a mix of ids, class names, and such. Then from the test code check if InspectorUI.highlighter.nodeInfo contains the correct information. Check if the ids, class names are populated in the right places. Also check if the nodeinfo elements themselves hold the correct information. If you want to get complicated, you can also check if the container position attribute is correct for some cases.

Looking forward for the updated patch. Thank you!

::: browser/base/content/highlighter.css
@@ +41,5 @@
> +
> +#highlighter-nodeinfobar-container {
> +  z-index: 1;
> +  position: absolute;
> +  visibility: visible;

If I am not mistaken, this is the default. So, this is not needed.

::: browser/base/content/inspector.js
@@ +209,5 @@
> +   *   </vbox>
> +   *   <box id="Highlighter-nodeinfobar-arrow"/>
> +   * </box>
> +   *
> +   * @param nsIDOMNode aParent

aParent needs to be an nsIDOMElement. Please describe what aParent is supposed to be.

Same comment applies to buildControls().

@@ +213,5 @@
> +   * @param nsIDOMNode aParent
> +   */
> +  buildNodeInfobar: function Highlighter_buildNodeInfobar(aParent)
> +  {
> +    this.nodeInfobarContainer = document.createElement("box"); //TODELETE

Why do you have "//TODELETE" in several places? Please clean up. :)

@@ +227,5 @@
> +    let tagname = document.createElement("span");
> +    tagname.id = "highlighter-nodeinfobar-tagname";
> +
> +    let id = document.createElement("span");
> +    id.id = "highlighter-nodeinfobar-id";

id.id looks kinda funny.

Maybe idElement?

@@ +252,5 @@
> +
> +    style = window.getComputedStyle(arrow, null);
> +    let left = parseInt(style.getPropertyValue("left"));
> +    let width = parseInt(style.getPropertyValue("width"));
> +    this.infobarArrowSize = 2 * left + width; //TODELETE

I think you should group these new properties better. For example:

this.nodeInfo = {
  tagName: tagname,
  id: id,
  classes: classes,
  container: ...,
  barHeight: ...,
  arrowSize: ...,
};

So you add only one property to the Highlighter object.

@@ +379,5 @@
>    {
>      this.node = aNode;
>      this.highlight(aParams && aParams.scroll);
> +    this.updateNodeInfo();
> +    this.moveNodeInfobar();

Why not call moveNodeInfobar() inside updateNodeInfo()?

@@ +453,5 @@
> +      classes.removeChild(classes.firstChild);
> +    }
> +    if (this.node.className) {
> +      // Should we use a fragment here?
> +      for (let i = 0; i < this.node.classList.length; i++) {

You do not need to wrap the for loop inside the if clause. If there are not classes classList.length is 0, so the loop won't execute.

@@ +615,5 @@
>          aEvent.preventDefault();
>          break;
>        case "scroll":
>          this.highlight();
> +        this.moveNodeInfobar();

You also need to handle the resize. Currently the node info bar does not update when I resize the browser.

See Highlighter.handleResize().

::: browser/themes/gnomestripe/browser/browser.css
@@ +1979,5 @@
> +  border: 1px solid #AAA;
> +  border-radius: 4px;
> +  height: 34px;
> +  padding: 0 16px;
> +  min-width: 50px;

I believe this causes:

http://img.i7m.de/show/rbwad.png

Notice that the tag name is not centered. Please fix this.
Comment 25 Mihai Sucan [:msucan] 2011-08-05 10:15:47 PDT
Comment on attachment 551035 [details] [diff] [review]
patch v1

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

I forgot to mention a problem with one of the code comments. See below.

::: browser/base/content/inspector.js
@@ +469,5 @@
> +  {
> +    let rect = this._highlightRect;
> +    if (rect && this._highlighting) {
> +      this.nodeInfobarContainer.removeAttribute("hidden");
> +      // Is the bar can be on the top of the node?

I do not understand the comment. Please update this.
Comment 26 Stephen Horlander [:shorlander] 2011-08-19 14:37:49 PDT
Comment on attachment 551035 [details] [diff] [review]
patch v1

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

Will followup with an attachment for visual aid.

> . font-family, which one?

We should use the system font. Lucida Grande on OSX, Segoe UI for Windows Vista/7, Tahoma for Windows XP, etc.

> . borders (I can't use a shadow)

If not shadows, can we use transparency on the border at all (see attachment)?

>  . color for classes

I like the green id, the magenta seems hard to read though, how about a bright blue for classes? 

> . horizontal alignment of the infobar

Centering the bar on the box would seem more balanced and would be the same distance from your cursor no matter where you enter the box area. 

Aligning left on large nodes feel ok but feels kind of awkward/heavy on smaller nodes. What was the rationale for left alignment?

>. shape of the arrows when the node is small

The half-arrow works pretty well. We could minimize the occurrence of this by making the arrow smaller.

Other things I noticed:
- Infobar gets cutoff if it is on the side
- Infobar disappears if you disable a class that contains the box's dimensions
- Switching from a half-arrow bar to a full-arrow bar results in an offset on any further full-arrow bars
- Text is a little hard to read, a darker background would help
- Text is not vertically aligned

Looks good, awesome stuff! I really like the animations. Maybe putting a slight delay on the infobar when switching from box to box would make it appear less bouncy.
Comment 27 Stephen Horlander [:shorlander] 2011-08-19 14:40:22 PDT
Created attachment 554557 [details]
InfoBar Colors/Thoughts

Image to go with ui-review
Comment 28 Paul Rouget [:paul] 2011-08-31 02:41:10 PDT
(In reply to Mihai Sucan [:msucan] from comment #24)
> Comment on attachment 551035 [details] [diff] [review]
> patch v1
> 
> Review of attachment 551035 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch looks fine, it's definitely in the right direction. Feedback+!
> 
> Also, the UI is awesome! This looks very, very good! Great work!
> 
> General comments:
> 
> - In Highlighter.destroy() please clean up the nodeinfo stuff. This code
> leaks nodes.

Ok.

> - Some tests are needed for this new functionality. You can make a test that
> highlights several elements that have a mix of ids, class names, and such.
> Then from the test code check if InspectorUI.highlighter.nodeInfo contains
> the correct information. Check if the ids, class names are populated in the
> right places. Also check if the nodeinfo elements themselves hold the
> correct information. If you want to get complicated, you can also check if
> the container position attribute is correct for some cases.

Ok, I'll try that.

> Looking forward for the updated patch. Thank you!
> 
> ::: browser/base/content/highlighter.css
> @@ +41,5 @@
> > +
> > +#highlighter-nodeinfobar-container {
> > +  z-index: 1;
> > +  position: absolute;
> > +  visibility: visible;
> 
> If I am not mistaken, this is the default. So, this is not needed.

Right. Legacy code from the layout patch.

> ::: browser/base/content/inspector.js
> @@ +209,5 @@
> > +   *   </vbox>
> > +   *   <box id="Highlighter-nodeinfobar-arrow"/>
> > +   * </box>
> > +   *
> > +   * @param nsIDOMNode aParent
> 
> aParent needs to be an nsIDOMElement.

I only use appendChild. So I guess nsIDOMNode is right.

> Please describe what aParent is
> supposed to be.

Sure.

> Same comment applies to buildControls().

Not part of this bug. Should I change the description anyway?

> @@ +213,5 @@
> > +   * @param nsIDOMNode aParent
> > +   */
> > +  buildNodeInfobar: function Highlighter_buildNodeInfobar(aParent)
> > +  {
> > +    this.nodeInfobarContainer = document.createElement("box"); //TODELETE
> 
> Why do you have "//TODELETE" in several places? Please clean up. :)

Things I should not forget to destroy. And yes, I'll clean these!

> @@ +227,5 @@
> > +    let tagname = document.createElement("span");
> > +    tagname.id = "highlighter-nodeinfobar-tagname";
> > +
> > +    let id = document.createElement("span");
> > +    id.id = "highlighter-nodeinfobar-id";
> 
> id.id looks kinda funny.
> 
> Maybe idElement?

Ok.

> @@ +252,5 @@
> > +
> > +    style = window.getComputedStyle(arrow, null);
> > +    let left = parseInt(style.getPropertyValue("left"));
> > +    let width = parseInt(style.getPropertyValue("width"));
> > +    this.infobarArrowSize = 2 * left + width; //TODELETE
> 
> I think you should group these new properties better. For example:
> 
> this.nodeInfo = {
>   tagName: tagname,
>   id: id,
>   classes: classes,
>   container: ...,
>   barHeight: ...,
>   arrowSize: ...,
> };
> 
> So you add only one property to the Highlighter object.

Agreed.

> @@ +379,5 @@
> >    {
> >      this.node = aNode;
> >      this.highlight(aParams && aParams.scroll);
> > +    this.updateNodeInfo();
> > +    this.moveNodeInfobar();
> 
> Why not call moveNodeInfobar() inside updateNodeInfo()?

Because we may want to only move the bar (on resize or scroll for example).

> @@ +453,5 @@
> > +      classes.removeChild(classes.firstChild);
> > +    }
> > +    if (this.node.className) {
> > +      // Should we use a fragment here?
> > +      for (let i = 0; i < this.node.classList.length; i++) {
> 
> You do not need to wrap the for loop inside the if clause. If there are not
> classes classList.length is 0, so the loop won't execute.

Right.

> @@ +615,5 @@
> >          aEvent.preventDefault();
> >          break;
> >        case "scroll":
> >          this.highlight();
> > +        this.moveNodeInfobar();
> 
> You also need to handle the resize. Currently the node info bar does not
> update when I resize the browser.
> 
> See Highlighter.handleResize().

Ok.

> ::: browser/themes/gnomestripe/browser/browser.css
> @@ +1979,5 @@
> > +  border: 1px solid #AAA;
> > +  border-radius: 4px;
> > +  height: 34px;
> > +  padding: 0 16px;
> > +  min-width: 50px;
> 
> I believe this causes:
> 
> http://img.i7m.de/show/rbwad.png
> 
> Notice that the tag name is not centered. Please fix this.

Will be with the new design.

Thank you for this. Working on a new patch right now.
Comment 29 Paul Rouget [:paul] 2011-08-31 06:13:21 PDT
Created attachment 557140 [details] [diff] [review]
patch v2.2 - WIP (no test)
Comment 30 Paul Rouget [:paul] 2011-08-31 06:23:05 PDT
Created attachment 557148 [details]
Screenshot (v2.2)

I have updated the design in the patch v2.2, and addressed Shorlander's comments.

There is still a case I don't handle correctly: when the infobar overflows (horizontally), I move it to make sure it is completely visible. In this case, I don't draw the arrow because calculating how it is supposed to be drawn is complicated (a simple offset is not enough, we also need to resize the arrow and orient it (left/right for a small arrow)). So for now, I believe we can just keep this behavior, and fix this arrow positioning later in a new bug.

Is that ok? (this case is not that frequent, and the degradation is not that bad).
Comment 31 Paul Rouget [:paul] 2011-08-31 07:03:17 PDT
Created attachment 557156 [details] [diff] [review]
patch v2.3 - fix gradients - WIP (no test)

Fix gradient for the position=bottom infobar.
Comment 32 Paul Rouget [:paul] 2011-09-02 07:28:48 PDT
Created attachment 557824 [details] [diff] [review]
patch v3

Shorlander's design + test.
Comment 33 Dão Gottwald [:dao] 2011-09-02 08:49:28 PDT
In general, please ask me for review for browser/themes/*/browser/browser.css changes.
Comment 34 Paul Rouget [:paul] 2011-09-02 09:59:10 PDT
(In reply to Dão Gottwald [:dao] from comment #33)
> In general, please ask me for review for
> browser/themes/*/browser/browser.css changes.

Ok. I was waiting for a positive review from someone in devtools before asking you.
Comment 35 Paul Rouget [:paul] 2011-09-02 10:04:34 PDT
Dão, can please take a look at how I managed to draw the arrow: 14x14 box rotated (45deg) with a gradient (transparent 50%, black 100%).

Looks like it is working well, but I'd like to have your opinion on this.
Comment 36 Mihai Sucan [:msucan] 2011-09-02 10:59:38 PDT
(In reply to Paul Rouget [:paul] from comment #28)
> (In reply to Mihai Sucan [:msucan] from comment #24)
> > ::: browser/base/content/inspector.js
> > @@ +209,5 @@
> > > +   *   </vbox>
> > > +   *   <box id="Highlighter-nodeinfobar-arrow"/>
> > > +   * </box>
> > > +   *
> > > +   * @param nsIDOMNode aParent
> > 
> > aParent needs to be an nsIDOMElement.
> 
> I only use appendChild. So I guess nsIDOMNode is right.

A node can be an attribute, comment or text node. I believe you want an element as an argument to the method.

(this comment applies to buildControls(), buildInfobar() and buildCloseButton())

> > Please describe what aParent is
> > supposed to be.
> 
> Sure.
> 
> > Same comment applies to buildControls().
> 
> Not part of this bug. Should I change the description anyway?

You change buildControls() in this patch, so I would prefer you to also improve the description. Thanks!

> Thank you for this. Working on a new patch right now.

Thank you for the updated patch! (currently reviewing it)
Comment 37 Paul Rouget [:paul] 2011-09-02 11:33:26 PDT
Created attachment 557895 [details] [diff] [review]
patch v3.1 (with test)

Sorry, forgot to include the test
Comment 38 Mihai Sucan [:msucan] 2011-09-02 12:38:21 PDT
Comment on attachment 557895 [details] [diff] [review]
patch v3.1 (with test)

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

Thanks for your patch changes. Giving it r+ but please address the comments. See also comment 36.

::: browser/base/content/highlighter.css
@@ +52,5 @@
> +}
> +
> +#highlighter-nodeinfobar-arrow {
> +  display: block;
> +  position: absolute;

AFAIK display:block is implied when position:absolute is used.

::: browser/base/content/test/inspector/browser_inspector_infobar.js
@@ +57,5 @@
> +  {
> +    executeSoon(function() {
> +      performTest();
> +      cursor++;
> +      ok(true, cursor + "/" + nodes.length);

This is debug output. Please don't use test functions for debug output.

IIRC you can use info() or, when possible, just entirely remove the debug info.

(please update the test file accordingly. there are multiple places where you have such debug output. thanks!)

::: browser/themes/gnomestripe/browser/browser.css
@@ +1994,5 @@
> +/* Highlighter - Node Infobar - text */
> +
> +#highlighter-nodeinfobar-tagname {
> +  text-transform: lowercase;
> +  color: #FFFFFF;

Nit: color values are inconsistent in this patch and in the file, in general. Please pick one style for color values. Thanks!

@@ +2036,5 @@
> +  margin-left: -8px; /* (14px + 1px + 1px) / 2 */
> +  -moz-transform: rotate(-45deg);
> +  border-width: 1px;
> +  border-style: solid;
> +  border-color: transparent;

Why not use a single border: shorthand?
Comment 39 Paul Rouget [:paul] 2011-09-02 13:05:08 PDT
Created attachment 557924 [details] [diff] [review]
patch v3.2

Addressed Mihai's comments.
Comment 40 Dão Gottwald [:dao] 2011-09-05 12:18:45 PDT
(In reply to Mihai Sucan [:msucan] from comment #38)
> > +#highlighter-nodeinfobar-arrow {
> > +  display: block;
> > +  position: absolute;
> 
> AFAIK display:block is implied when position:absolute is used.

Yes, but that's a bug.
Comment 41 Dão Gottwald [:dao] 2011-09-09 07:51:52 PDT
Comment on attachment 557924 [details] [diff] [review]
patch v3.2

>--- a/browser/base/content/highlighter.css
>+++ b/browser/base/content/highlighter.css

>+#highlighter-nodeinfobar-id:before {
>+  content: "#";
>+}
>+#highlighter-nodeinfobar-classes > label:before {
>+  content: ".";
>+}

Can these labels share a CSS class that you can use instead of #foo > label?

nit: ::before with two colons

>--- a/browser/base/content/inspector.js
>+++ b/browser/base/content/inspector.js

>+    let style = window.getComputedStyle(container, null);

the second argument is optional

>+      this.nodeInfo.container.style.left = "0px";
>+      this.nodeInfo.container.style.top = "0px";

nit: "0" instead of "0px"

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

>+#highlighter-nodeinfobar-tagname {
>+  text-transform: lowercase;

Should this be in inspector.css?

>+#highlighter-nodeinfobar-tagname,
>+#highlighter-nodeinfobar-id,
>+#highlighter-nodeinfobar-classes > label {
>+  font-size: 12px;

Not sure why you set a font size here.

>+#highlighter-nodeinfobar-container {
>+  height: 48px;
>+}

What is this needed for? In general, automatic sizing is preferable to magic numbers.

Is all of this tested with RTL?

r- per comment 40. If you want display:block, you should set it explicitly.
Comment 42 Paul Rouget [:paul] 2011-09-13 11:48:38 PDT
Created attachment 560008 [details] [diff] [review]
patch v3.3

Addressed Dao's comments.

(In reply to Dão Gottwald [:dao] from comment #41)
> Comment on attachment 557924 [details] [diff] [review]
> patch v3.2
> >+#highlighter-nodeinfobar-container {
> >+  height: 48px;
> >+}
> 
> What is this needed for? In general, automatic sizing is preferable to magic
> numbers.

I understand that, but its child nodes use absolute positioning, so it needs a height (the arrow uses bottom:…). Also in inspector.js I need to know the height
of the nodeinfobar. I use the computedStyle for that. I could use getBoundingBoxRect, but then the nodeinfobar will need to be displayed first.

So I could avoid using height:48px, but using it makes my life easier.

> Is all of this tested with RTL?

Now it is.
Comment 43 Dão Gottwald [:dao] 2011-09-13 12:19:56 PDT
(In reply to Paul Rouget [:paul] from comment #42)
> I understand that, but its child nodes use absolute positioning, so it needs
> a height (the arrow uses bottom:…).

So the arrow is positioned absolutely for moving it to the top / bottom? And you can't use -moz-box-ordinal-group because the container has display:block? Maybe the solution would be to have two arrows, a top and a bottom one, each one hidden when not needed?

> Also in inspector.js I need to know the height
> of the nodeinfobar. I use the computedStyle for that. I could use
> getBoundingBoxRect, but then the nodeinfobar will need to be displayed first.

Why is this a problem? Does it flicker? Can you hide the info bar with visibility:hidden, then call getBoundingBoxRect, then reset the visibility?

> So I could avoid using height:48px, but using it makes my life easier.

This is making assumptions about the font size, which doesn't seem sane.
Comment 44 Paul Rouget [:paul] 2011-09-15 11:03:13 PDT
Created attachment 560405 [details] [diff] [review]
patch v3.4

(In reply to Dão Gottwald [:dao] from comment #43)
> Maybe the solution would be to have two arrows, a top and a
> bottom one, each one hidden when not needed?

Done.
Comment 45 Dão Gottwald [:dao] 2011-09-15 11:17:11 PDT
Comment on attachment 560405 [details] [diff] [review]
patch v3.4

>+#highlighter-nodeinfobar-id[empty] {
>+  display: none;
>+}

>+    if (this.node.id) {
>+      this.nodeInfo.idLabel.removeAttribute("empty");
>+      this.nodeInfo.idLabel.textContent = this.node.id;
>+    } else {
>+      this.nodeInfo.idLabel.textContent = "";
>+      this.nodeInfo.idLabel.setAttribute("empty", "true");
>+    }

Can you use :empty?

>+  background-image: -moz-linear-gradient(bottom left, transparent, transparent 50%, hsl(209, 18%, 30%) 50%);

The leading "transparent, " is a no-op, I think.
Comment 46 Paul Rouget [:paul] 2011-09-16 12:22:21 PDT
Created attachment 560605 [details] [diff] [review]
patch v3.6

Addressed Dao's comments.
Comment 47 Dão Gottwald [:dao] 2011-09-17 09:54:45 PDT
Comment on attachment 560605 [details] [diff] [review]
patch v3.6

The arrow is misaligned for RTL.
Comment 48 Paul Rouget [:paul] 2011-09-20 04:23:16 PDT
Created attachment 561163 [details] [diff] [review]
patch v3.7

fixed RTL.
Comment 49 Dão Gottwald [:dao] 2011-09-20 04:36:53 PDT
Comment on attachment 561163 [details] [diff] [review]
patch v3.7

(In reply to Paul Rouget [:paul] from comment #48)
> Created attachment 561163 [details] [diff] [review]
> patch v3.7
> 
> fixed RTL.

Apparently only in pinstripe?!

Can you use -moz-margin-start instead of margin-left for LTR and margin-right for RTL?
Comment 50 Paul Rouget [:paul] 2011-09-20 04:42:54 PDT
erf, sorry for that.
Comment 51 Paul Rouget [:paul] 2011-09-20 05:12:19 PDT
Created attachment 561167 [details] [diff] [review]
patch v3.8

addressed dao's comments.
Comment 52 Dão Gottwald [:dao] 2011-09-20 08:35:23 PDT
Comment on attachment 561167 [details] [diff] [review]
patch v3.8

>+    let tagNameLabel = document.createElement("label");
>+    tagNameLabel.id = "highlighter-nodeinfobar-tagname";
>+
>+    let idLabel = document.createElement("label");
>+    idLabel.id = "highlighter-nodeinfobar-id";

>+        let classLabel = document.createElement("label");
>+        classLabel.className = "highlighter-nodeinfobar-class";

>+#highlighter-nodeinfobar-tagname,
>+#highlighter-nodeinfobar-id,
>+.highlighter-nodeinfobar-class {
>+  margin: 0;
>+}

Instead of removing the margin in the themes, can these nodes have the "plain" CSS class?

>+#highlighter-nodeinfobar {
>+  line-height: 32px;
>+  padding: 0 16px;

The magic line height should be dropped in favor of vertical padding.
Comment 53 Paul Rouget [:paul] 2011-09-21 08:23:08 PDT
Created attachment 561474 [details] [diff] [review]
patch v3.9

Addressed Dao's comments.
Comment 54 Dão Gottwald [:dao] 2011-09-21 08:44:53 PDT
Comment on attachment 561474 [details] [diff] [review]
patch v3.9

>+#highlighter-nodeinfobar {
>+  border: 1px solid hsla(210, 19%, 63%, .5);
>+  background-clip: padding-box;
>+  border-radius: 3px;
>+  padding: 8px 16px;
>+  background-clip: padding-box;
>+  background: -moz-linear-gradient(hsl(209, 18%, 30%), hsl(210, 24%, 16%)) no-repeat;
>+}

background-clip is duplicated. Also, AFAIK and according to <http://www.w3.org/TR/css3-background/#the-background>, the background shorthand resets background-clip to its initial value, border-box.
Comment 55 Paul Rouget [:paul] 2011-09-21 11:23:57 PDT
Created attachment 561525 [details] [diff] [review]
patch v3.10

Addressed Dao's comments.
Comment 56 Dão Gottwald [:dao] 2011-09-21 11:30:06 PDT
Comment on attachment 561525 [details] [diff] [review]
patch v3.10

>+  background: -moz-linear-gradient(hsl(209, 18%, 30%), hsl(210, 24%, 16%)) no-repeat;
>+  background-clip: padding-box;

You can just add padding-box to the background shorthand. (background-origin already defaults to padding-box, this will just make it explicit.)
Comment 57 Paul Rouget [:paul] 2011-09-21 12:52:20 PDT
Created attachment 561549 [details] [diff] [review]
patch v3.11
Comment 58 Paul Rouget [:paul] 2011-09-21 12:54:21 PDT
Thank you for the reviews Dao.
Comment 59 Paul Rouget [:paul] 2011-09-26 04:14:11 PDT
try server: looks good https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=a644921d9d4c
Comment 60 Paul Rouget [:paul] 2011-09-26 05:31:36 PDT
Needs to be rebased. Working on it.
Comment 61 Paul Rouget [:paul] 2011-09-27 04:37:06 PDT
Created attachment 562716 [details] [diff] [review]
patch v3.11 - rebased

rebased (moved the code and s/document/this.chromeDoc/ )
Comment 62 Paul Rouget [:paul] 2011-09-27 04:51:43 PDT
Created attachment 562721 [details] [diff] [review]
patch v3.12 - rebased

Forgot to update the test.
Comment 63 Mihai Sucan [:msucan] 2011-09-27 06:02:59 PDT
Comment on attachment 562721 [details] [diff] [review]
patch v3.12 - rebased

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

Patch looks fine. Giving it a tentative r+, but you have a non-fatal error that shows during the execution of tests:

TEST-INFO | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_bug_665880.js | Console message: [JavaScript Error: "this.nodeInfo is null" {file: "resource:///modules/inspector.jsm" line: 485}]

Please fix this before we land the patch. Thank you!
Comment 64 Paul Rouget [:paul] 2011-09-27 09:16:15 PDT
Created attachment 562787 [details] [diff] [review]
[in-fx-team] patch v3.13

addressed Mihai's comment (moved moveInfobar inside the highlight function to make sure the notification is sent only once the infobar has been moved).
Comment 65 Mihai Sucan [:msucan] 2011-09-27 10:13:48 PDT
Comment on attachment 562787 [details] [diff] [review]
[in-fx-team] patch v3.13

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

Ready to land. No more errors. Thanks for your quick fix!
Comment 66 Rob Campbell [:rc] (:robcee) 2011-09-27 13:02:26 PDT
Comment on attachment 562787 [details] [diff] [review]
[in-fx-team] patch v3.13

https://hg.mozilla.org/integration/fx-team/rev/713f76a29f10
Comment 67 Rob Campbell [:rc] (:robcee) 2011-09-28 06:49:02 PDT
https://hg.mozilla.org/mozilla-central/rev/713f76a29f10
Comment 68 Teodosia Pop 2011-10-07 02:44:03 PDT
Verified Fixed using Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111006 Firefox/10.0a1.

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