the highlighter nodeinfobar should also be displayed for text nodes

RESOLVED FIXED in Firefox 52

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: pbro, Assigned: jdescottes)

Tracking

unspecified
Firefox 52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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 hidden (mozreview-request)
(Reporter)

Comment 2

2 years ago
mozreview-review
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+
(Assignee)

Comment 3

2 years ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
(Reporter)

Comment 8

2 years ago
(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.
(Reporter)

Comment 9

2 years ago
mozreview-review
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

2 years ago
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

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/229524a1fa78
https://hg.mozilla.org/mozilla-central/rev/0f3da9163d9d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 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]
You need to log in before you can comment on or make changes to this bug.