Closed Bug 1309212 Opened 8 years ago Closed 8 years ago

the highlighter nodeinfobar should also be displayed for text nodes

Categories

(DevTools :: Inspector, defect)

Product:
DevTools
Component:
Inspector
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: pbro, Assigned: jdescottes)

References

Details

Attachments

(2 files)

Bug 1309212 - display highlighter infobar for #text nodes;
58 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1309212 - request longer timeout for browser_inspector_select-last-selected.js;
58 bytes, text/x-review-board-request
pbro
: review+
Details 
Bug 1304679 makes text nodes highlightable in the page. However the nodeinfobar (the tooltip that usually appears next to the highlighter) isn't shown for text nodes. It is displayed only for element nodes.

In this bug we should change this. The nodeinfobar should also be displayed, and only contain the size of the node in the page.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment on attachment 8800850 [details]
Bug 1309212 - display highlighter infobar for #text nodes;

https://reviewboard.mozilla.org/r/85638/#review84456

Really great patch. Thanks. Pnly a couple of minor comments that don't need to block R+.

::: devtools/server/actors/highlighters/box-model.js:683
(Diff revision 1)
> +    let isElementNode = node.nodeType === nodeConstants.ELEMENT_NODE;
> +    if (!isElementNode) {

Maybe no need to initialize `isElementNode` since you're not using it anywhere else than within the `if` condition and it's not particularly hard to read.

`if (node.nodeType !== nodeConstants.ELEMENT_NODE)`

::: devtools/server/actors/inspector.js:196
(Diff revision 1)
>   */
>  const getNodeDisplayName = function (rawNode) {
> +  if (rawNode.nodeName && !rawNode.localName) {
> +    // The localName API has been moved from the Node interface to the Element interface.
> +    // Use Node.nodeName as a fallback.
> +    return rawNode.nodeName;

What about the `rawNode.prefix` here? Shouldn't we also concatenate it to the nodeName in this code path too?
Attachment #8800850 - Flags: review?(pbrosset) → review+
Comment on attachment 8800850 [details]
Bug 1309212 - display highlighter infobar for #text nodes;

https://reviewboard.mozilla.org/r/85638/#review84456

Thanks for the review!

> Maybe no need to initialize `isElementNode` since you're not using it anywhere else than within the `if` condition and it's not particularly hard to read.
> 
> `if (node.nodeType !== nodeConstants.ELEMENT_NODE)`

Sure!

> What about the `rawNode.prefix` here? Shouldn't we also concatenate it to the nodeName in this code path too?

The prefix property is also only available on the Element interface, but I will update the comment to mention this as well.
Looking at my last try push, this changeset seems to be increasing the timeout frequency of browser_inspector_select-last-selected.js on Linux debug. 

The test is usually passing with a running time of 40+ seconds, dangerously close to a timeout and there is already Bug 1051968 logged for this intermittent. We can see that regardless of my patch it was becoming more frequent anyway. But I do think it's worst with my patch (probably just because I added a new test, which moved this test into another batch).

I would like to simply increase the timeout here. Let me know if you'd prefer to tackle that differently.
(In reply to Julian Descottes [:jdescottes] from comment #7)
> Looking at my last try push, this changeset seems to be increasing the
> timeout frequency of browser_inspector_select-last-selected.js on Linux
> debug. 
> 
> The test is usually passing with a running time of 40+ seconds, dangerously
> close to a timeout and there is already Bug 1051968 logged for this
> intermittent. We can see that regardless of my patch it was becoming more
> frequent anyway. But I do think it's worst with my patch (probably just
> because I added a new test, which moved this test into another batch).
> 
> I would like to simply increase the timeout here. Let me know if you'd
> prefer to tackle that differently.
Agreed, let's increase the timeout.
Comment on attachment 8801156 [details]
Bug 1309212 - request longer timeout for browser_inspector_select-last-selected.js;

https://reviewboard.mozilla.org/r/85922/#review85038
Attachment #8801156 - Flags: review?(pbrosset) → review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/229524a1fa78
display highlighter infobar for #text nodes;r=pbro
https://hg.mozilla.org/integration/autoland/rev/0f3da9163d9d
request longer timeout for browser_inspector_select-last-selected.js;r=pbro
https://hg.mozilla.org/mozilla-central/rev/229524a1fa78
https://hg.mozilla.org/mozilla-central/rev/0f3da9163d9d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
I have reproduced this bug with Nightly 52.0a1 (2016-10-11) (64-bit) on Windows 7 , 64 Bit ! 

This bug's fix is verified with latest Developer Edition (Aurora)

Build ID    :  20170104004006
User Agent  :  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0
[bugday-20170104]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: