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

VERIFIED FIXED in Firefox 10

Status

()

Firefox
Developer Tools
P1
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: paul, Assigned: paul)

Tracking

unspecified
Firefox 10
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 23 obsolete attachments)

626.93 KB, image/jpeg
Details
25.03 KB, patch
msucan
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Updated

6 years ago
Blocks: 663830
(Assignee)

Updated

6 years ago
Assignee: nobody → paul
(Assignee)

Comment 1

6 years ago
Kevin suggested to add the numeric info layout as well (margin sizes & co).
(Assignee)

Updated

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

Comment 2

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

Comment 3

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

Updated

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

Comment 6

6 years ago
Makes sense. It's node related to the node itself.
(Assignee)

Comment 7

6 years ago
s/node/not/
(Assignee)

Comment 8

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

Comment 10

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

Comment 11

6 years ago
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.
Blocks: 672003

Updated

6 years ago
Whiteboard: minotaur
Paul, could you attach your WIP patch to this bug please? Thanks!
Priority: -- → P1
Whiteboard: minotaur → [minotaur][has-patch]

Updated

6 years ago
Whiteboard: [minotaur][has-patch] → [minotaur][has-patch][best: 2d; likely: 3d; worst: 6d]
(Assignee)

Comment 14

6 years ago
Created attachment 550380 [details] [diff] [review]
patch v0.1 WIP

Using absolute positioning.
(Assignee)

Comment 15

6 years ago
Created attachment 550381 [details]
screenshot: normal view
(Assignee)

Comment 16

6 years ago
Created attachment 550382 [details]
screenshot: if no space on top
(Assignee)

Comment 17

6 years ago
Created attachment 550383 [details]
screenshot: if no space arround the node
(Assignee)

Comment 18

6 years ago
Created attachment 550384 [details]
screenshot: if node small
(Assignee)

Comment 19

6 years ago
We may want to do not use "<>" around the tag name to keep the text as a valid selector.
(Assignee)

Comment 20

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

Comment 21

6 years ago
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.
Attachment #550384 - Attachment is obsolete: true
Attachment #551035 - Flags: ui-review?(shorlander)
Attachment #551035 - Flags: review?(mihai.sucan)
Attachment #551035 - Flags: feedback?(dao)
(Assignee)

Comment 22

6 years ago
Created attachment 551036 [details]
screenshot: if node small
(Assignee)

Comment 23

6 years ago
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-
Created attachment 554557 [details]
InfoBar Colors/Thoughts

Image to go with ui-review
(Assignee)

Comment 28

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

Comment 29

6 years ago
Created attachment 557140 [details] [diff] [review]
patch v2.2 - WIP (no test)
Attachment #550380 - Attachment is obsolete: true
Attachment #551035 - Attachment is obsolete: true
Attachment #551035 - Flags: feedback?(dao)
(Assignee)

Comment 30

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

Comment 31

6 years ago
Created attachment 557156 [details] [diff] [review]
patch v2.3 - fix gradients - WIP (no test)

Fix gradient for the position=bottom infobar.
Attachment #557140 - Attachment is obsolete: true
(Assignee)

Comment 32

6 years ago
Created attachment 557824 [details] [diff] [review]
patch v3

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

Comment 34

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

Comment 35

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

Updated

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

Comment 37

6 years ago
Created attachment 557895 [details] [diff] [review]
patch v3.1 (with test)

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)
(Assignee)

Updated

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

Comment 39

6 years ago
Created attachment 557924 [details] [diff] [review]
patch v3.2

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

Updated

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

Comment 42

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

Comment 44

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

Comment 46

6 years ago
Created attachment 560605 [details] [diff] [review]
patch v3.6

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-
(Assignee)

Comment 48

6 years ago
Created attachment 561163 [details] [diff] [review]
patch v3.7

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-
(Assignee)

Comment 50

6 years ago
erf, sorry for that.
(Assignee)

Comment 51

6 years ago
Created attachment 561167 [details] [diff] [review]
patch v3.8

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

Comment 53

6 years ago
Created attachment 561474 [details] [diff] [review]
patch v3.9

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

Comment 55

6 years ago
Created attachment 561525 [details] [diff] [review]
patch v3.10

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+
(Assignee)

Comment 57

6 years ago
Created attachment 561549 [details] [diff] [review]
patch v3.11
Attachment #561525 - Attachment is obsolete: true
(Assignee)

Comment 58

6 years ago
Thank you for the reviews Dao.
Whiteboard: [minotaur][has-patch][best: 2d; likely: 3d; worst: 6d] → [minotaur][land-in-fx-team]
(Assignee)

Comment 59

6 years ago
try server: looks good https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=a644921d9d4c
(Assignee)

Comment 60

6 years ago
Needs to be rebased. Working on it.
(Assignee)

Comment 61

6 years ago
Created attachment 562716 [details] [diff] [review]
patch v3.11 - rebased

rebased (moved the code and s/document/this.chromeDoc/ )
Attachment #561549 - Attachment is obsolete: true
Attachment #562716 - Flags: review?(mihai.sucan)
(Assignee)

Updated

6 years ago
Whiteboard: [minotaur][land-in-fx-team] → [minotaur]
(Assignee)

Comment 62

6 years ago
Created attachment 562721 [details] [diff] [review]
patch v3.12 - rebased

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+
(Assignee)

Comment 64

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

Updated

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10

Comment 68

6 years ago
Verified Fixed using Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111006 Firefox/10.0a1.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.