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)
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
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → paul
Assignee | ||
Comment 1•13 years ago
|
||
Kevin suggested to add the numeric info layout as well (margin sizes & co).
Assignee | ||
Updated•13 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•13 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•13 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•13 years ago
|
Status: NEW → ASSIGNED
Comment 4•13 years ago
|
||
created bug 666650 (bitchin' bug number) for the global toolbar.
Comment 5•13 years ago
|
||
I think the global toolbar might include a toggle for the dark background. What do you think?
Assignee | ||
Comment 6•13 years ago
|
||
Makes sense. It's node related to the node itself.
Assignee | ||
Comment 7•13 years ago
|
||
s/node/not/
Assignee | ||
Comment 8•13 years ago
|
||
× tag name, id and classes × dimensions and position (if the "layout info" are visible) × highlighter specific buttons . toggle layout information . unlock node
Comment 9•13 years ago
|
||
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•13 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•13 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.
Comment 12•13 years ago
|
||
> So far, I feel like the position:absolute div is the easiest way to do that.
I think you're probably right.
Updated•13 years ago
|
Whiteboard: minotaur
Comment 13•13 years ago
|
||
Paul, could you attach your WIP patch to this bug please? Thanks!
Priority: -- → P1
Whiteboard: minotaur → [minotaur][has-patch]
Updated•13 years ago
|
Whiteboard: [minotaur][has-patch] → [minotaur][has-patch][best: 2d; likely: 3d; worst: 6d]
Assignee | ||
Comment 14•13 years ago
|
||
Using absolute positioning.
Assignee | ||
Comment 15•13 years ago
|
||
Assignee | ||
Comment 16•13 years ago
|
||
Assignee | ||
Comment 17•13 years ago
|
||
Assignee | ||
Comment 18•13 years ago
|
||
Assignee | ||
Comment 19•13 years ago
|
||
We may want to do not use "<>" around the tag name to keep the text as a valid selector.
Assignee | ||
Comment 20•13 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•13 years ago
|
||
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•13 years ago
|
||
Assignee | ||
Comment 23•13 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 24•13 years ago
|
||
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 25•13 years ago
|
||
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.
Updated•13 years ago
|
Attachment #550380 -
Flags: ui-review?(shorlander)
Comment 26•13 years ago
|
||
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-
Comment 27•13 years ago
|
||
Image to go with ui-review
Assignee | ||
Comment 28•13 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•13 years ago
|
||
Attachment #550380 -
Attachment is obsolete: true
Attachment #551035 -
Attachment is obsolete: true
Attachment #551035 -
Flags: feedback?(dao)
Assignee | ||
Comment 30•13 years ago
|
||
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•13 years ago
|
||
Fix gradient for the position=bottom infobar.
Attachment #557140 -
Attachment is obsolete: true
Assignee | ||
Comment 32•13 years ago
|
||
Shorlander's design + test.
Attachment #557156 -
Attachment is obsolete: true
Attachment #557824 -
Flags: review?(mihai.sucan)
Comment 33•13 years ago
|
||
In general, please ask me for review for browser/themes/*/browser/browser.css changes.
Assignee | ||
Comment 34•13 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•13 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•13 years ago
|
Attachment #557824 -
Flags: review?(dao)
Comment 36•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
Attachment #557895 -
Flags: review?(dao)
Comment 38•13 years ago
|
||
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•13 years ago
|
||
Addressed Mihai's comments.
Attachment #557895 -
Attachment is obsolete: true
Attachment #557895 -
Flags: review?(dao)
Attachment #557924 -
Flags: review?(dao)
Comment 40•13 years ago
|
||
(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•13 years ago
|
Attachment #557148 -
Attachment is obsolete: true
Attachment #557148 -
Flags: ui-review?(shorlander)
Comment 41•13 years ago
|
||
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•13 years ago
|
||
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)
Comment 43•13 years ago
|
||
(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•13 years ago
|
||
(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 45•13 years ago
|
||
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•13 years ago
|
||
Addressed Dao's comments.
Attachment #560405 -
Attachment is obsolete: true
Attachment #560405 -
Flags: review?(dao)
Attachment #560605 -
Flags: review?(dao)
Comment 47•13 years ago
|
||
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•13 years ago
|
||
fixed RTL.
Attachment #560605 -
Attachment is obsolete: true
Attachment #561163 -
Flags: review?(dao)
Comment 49•13 years ago
|
||
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•13 years ago
|
||
erf, sorry for that.
Assignee | ||
Comment 51•13 years ago
|
||
addressed dao's comments.
Attachment #561163 -
Attachment is obsolete: true
Attachment #561167 -
Flags: review?(dao)
Comment 52•13 years ago
|
||
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•13 years ago
|
||
Addressed Dao's comments.
Attachment #561167 -
Attachment is obsolete: true
Attachment #561167 -
Flags: review?(dao)
Attachment #561474 -
Flags: review?(dao)
Comment 54•13 years ago
|
||
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•13 years ago
|
||
Addressed Dao's comments.
Attachment #561474 -
Attachment is obsolete: true
Attachment #561474 -
Flags: review?(dao)
Attachment #561525 -
Flags: review?(dao)
Comment 56•13 years ago
|
||
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•13 years ago
|
||
Attachment #561525 -
Attachment is obsolete: true
Assignee | ||
Comment 58•13 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•13 years ago
|
||
try server: looks good https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=a644921d9d4c
Assignee | ||
Comment 60•13 years ago
|
||
Needs to be rebased. Working on it.
Assignee | ||
Comment 61•13 years ago
|
||
rebased (moved the code and s/document/this.chromeDoc/ )
Attachment #561549 -
Attachment is obsolete: true
Attachment #562716 -
Flags: review?(mihai.sucan)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [minotaur][land-in-fx-team] → [minotaur]
Assignee | ||
Comment 62•13 years ago
|
||
Forgot to update the test.
Attachment #562716 -
Attachment is obsolete: true
Attachment #562716 -
Flags: review?(mihai.sucan)
Attachment #562721 -
Flags: review?(mihai.sucan)
Comment 63•13 years ago
|
||
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•13 years ago
|
||
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 65•13 years ago
|
||
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•13 years ago
|
Whiteboard: [minotaur] → [minotaur][land-in-fx-team]
Comment 66•13 years ago
|
||
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
Updated•13 years ago
|
Whiteboard: [minotaur][land-in-fx-team] → [minotaur][fixed-in-fx-team]
Comment 67•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/713f76a29f10
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
Comment 68•13 years ago
|
||
Verified Fixed using Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111006 Firefox/10.0a1.
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•