Closed Bug 663833 Opened 13 years ago Closed 13 years ago

[highlighter] Add a floating toolbar next to the selected node

Categories

(DevTools :: General, defect, P1)

x86
Linux
defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 10

People

(Reporter: paul, Assigned: paul)

References

Details

(Whiteboard: [minotaur][fixed-in-fx-team])

Attachments

(2 files, 23 obsolete files)

626.93 KB, image/jpeg
Details
25.03 KB, patch
msucan
: review+
Details | Diff | Splinter Review
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
Blocks: 663830
Assignee: nobody → paul
Kevin suggested to add the numeric info layout as well (margin sizes & co).
Summary: [highlighter] Add tag name, id and classes of the selected node into the <iframe> → [highlighter] Add a floating toolbar next to the selected node
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
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
Status: NEW → ASSIGNED
created bug 666650 (bitchin' bug number) for the global toolbar.
I think the global toolbar might include a toggle for the dark background. What do you think?
Makes sense. It's node related to the node itself.
s/node/not/
× tag name, id and classes
× dimensions and position (if the "layout info" are visible)
× highlighter specific buttons
  . toggle layout information
  . unlock node
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?
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.
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.
> So far, I feel like the position:absolute div is the easiest way to do that.

I think you're probably right.
Whiteboard: minotaur
Paul, could you attach your WIP patch to this bug please? Thanks!
Priority: -- → P1
Whiteboard: minotaur → [minotaur][has-patch]
Whiteboard: [minotaur][has-patch] → [minotaur][has-patch][best: 2d; likely: 3d; worst: 6d]
Attached patch patch v0.1 WIP (obsolete) — Splinter Review
Using absolute positioning.
Attached image screenshot: normal view (obsolete) —
Attached image screenshot: if no space on top (obsolete) —
Attached image screenshot: if no space arround the node (obsolete) —
Attached image screenshot: if node small (obsolete) —
We may want to do not use "<>" around the tag name to keep the text as a valid selector.
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
Attachment #550380 - Flags: ui-review?(shorlander)
Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #550384 - Attachment is obsolete: true
Attachment #551035 - Flags: ui-review?(shorlander)
Attachment #551035 - Flags: review?(mihai.sucan)
Attachment #551035 - Flags: feedback?(dao)
Attached image screenshot: if node small (obsolete) —
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?
Attachment #551035 - Flags: review?(mihai.sucan) → feedback?(mihai.sucan)
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.
Attachment #551035 - Flags: feedback?(mihai.sucan) → feedback+
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.
Attachment #550380 - Flags: ui-review?(shorlander)
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.
Attachment #551035 - Flags: ui-review?(shorlander) → ui-review-
Attached image InfoBar Colors/Thoughts
Image to go with ui-review
(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.
Attached patch patch v2.2 - WIP (no test) (obsolete) — Splinter Review
Attachment #550380 - Attachment is obsolete: true
Attachment #551035 - Attachment is obsolete: true
Attachment #551035 - Flags: feedback?(dao)
Attached image Screenshot (v2.2) (obsolete) —
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).
Attachment #550381 - Attachment is obsolete: true
Attachment #550382 - Attachment is obsolete: true
Attachment #550383 - Attachment is obsolete: true
Attachment #551036 - Attachment is obsolete: true
Attachment #557148 - Flags: ui-review?(shorlander)
Fix gradient for the position=bottom infobar.
Attachment #557140 - Attachment is obsolete: true
Attached patch patch v3 (obsolete) — Splinter Review
Shorlander's design + test.
Attachment #557156 - Attachment is obsolete: true
Attachment #557824 - Flags: review?(mihai.sucan)
In general, please ask me for review for browser/themes/*/browser/browser.css changes.
(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.
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.
Attachment #557824 - Flags: review?(dao)
(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)
Attached patch patch v3.1 (with test) (obsolete) — Splinter Review
Sorry, forgot to include the test
Attachment #557824 - Attachment is obsolete: true
Attachment #557824 - Flags: review?(mihai.sucan)
Attachment #557824 - Flags: review?(dao)
Attachment #557895 - Flags: review?(mihai.sucan)
Attachment #557895 - Flags: review?(dao)
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?
Attachment #557895 - Flags: review?(mihai.sucan) → review+
Attached patch patch v3.2 (obsolete) — Splinter Review
Addressed Mihai's comments.
Attachment #557895 - Attachment is obsolete: true
Attachment #557895 - Flags: review?(dao)
Attachment #557924 - Flags: review?(dao)
(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.
Attachment #557148 - Attachment is obsolete: true
Attachment #557148 - Flags: ui-review?(shorlander)
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.
Attachment #557924 - Flags: review?(dao) → review-
Attached patch patch v3.3 (obsolete) — Splinter Review
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.
Attachment #557924 - Attachment is obsolete: true
Attachment #560008 - Flags: review?(dao)
(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.
Attached patch patch v3.4 (obsolete) — Splinter Review
(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.
Attachment #560008 - Attachment is obsolete: true
Attachment #560008 - Flags: review?(dao)
Attachment #560405 - Flags: review?(dao)
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.
Attached patch patch v3.6 (obsolete) — Splinter Review
Addressed Dao's comments.
Attachment #560405 - Attachment is obsolete: true
Attachment #560405 - Flags: review?(dao)
Attachment #560605 - Flags: review?(dao)
Comment on attachment 560605 [details] [diff] [review]
patch v3.6

The arrow is misaligned for RTL.
Attachment #560605 - Flags: review?(dao) → review-
Attached patch patch v3.7 (obsolete) — Splinter Review
fixed RTL.
Attachment #560605 - Attachment is obsolete: true
Attachment #561163 - Flags: review?(dao)
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?
Attachment #561163 - Flags: review?(dao) → review-
erf, sorry for that.
Attached patch patch v3.8 (obsolete) — Splinter Review
addressed dao's comments.
Attachment #561163 - Attachment is obsolete: true
Attachment #561167 - Flags: review?(dao)
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.
Attached patch patch v3.9 (obsolete) — Splinter Review
Addressed Dao's comments.
Attachment #561167 - Attachment is obsolete: true
Attachment #561167 - Flags: review?(dao)
Attachment #561474 - Flags: review?(dao)
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.
Attached patch patch v3.10 (obsolete) — Splinter Review
Addressed Dao's comments.
Attachment #561474 - Attachment is obsolete: true
Attachment #561474 - Flags: review?(dao)
Attachment #561525 - Flags: review?(dao)
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.)
Attachment #561525 - Flags: review?(dao) → review+
Attached patch patch v3.11 (obsolete) — Splinter Review
Attachment #561525 - Attachment is obsolete: true
Thank you for the reviews Dao.
Whiteboard: [minotaur][has-patch][best: 2d; likely: 3d; worst: 6d] → [minotaur][land-in-fx-team]
Needs to be rebased. Working on it.
Attached patch patch v3.11 - rebased (obsolete) — Splinter Review
rebased (moved the code and s/document/this.chromeDoc/ )
Attachment #561549 - Attachment is obsolete: true
Attachment #562716 - Flags: review?(mihai.sucan)
Whiteboard: [minotaur][land-in-fx-team] → [minotaur]
Attached patch patch v3.12 - rebased (obsolete) — Splinter Review
Forgot to update the test.
Attachment #562716 - Attachment is obsolete: true
Attachment #562716 - Flags: review?(mihai.sucan)
Attachment #562721 - Flags: review?(mihai.sucan)
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!
Attachment #562721 - Flags: review?(mihai.sucan) → review+
addressed Mihai's comment (moved moveInfobar inside the highlight function to make sure the notification is sent only once the infobar has been moved).
Attachment #562721 - Attachment is obsolete: true
Attachment #562787 - Flags: review?(mihai.sucan)
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!
Attachment #562787 - Flags: review?(mihai.sucan) → review+
Whiteboard: [minotaur] → [minotaur][land-in-fx-team]
Comment on attachment 562787 [details] [diff] [review]
[in-fx-team] patch v3.13

https://hg.mozilla.org/integration/fx-team/rev/713f76a29f10
Attachment #562787 - Attachment description: patch v3.13 → [in-fx-team] patch v3.13
Whiteboard: [minotaur][land-in-fx-team] → [minotaur][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/713f76a29f10
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
Verified Fixed using Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111006 Firefox/10.0a1.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: