1304679 - The highlighter should also highlight text nodes in the page

Status: VERIFIED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

Attachment: text-node-highlighting.PNG

The default highlighter in devtools (the box-model one) only highlights element nodes.
I'd like it to also highlight text nodes.

This is very useful in a variety of cases where the textnode inside of an element has a different geometry than the element itself:

- when the text wraps, the parent block element still only consists of 1 box, while the text node consists of multiple line boxes,
- when there are floated elements in the flow, then the parent block element still extends all the way to the edges, but the text node inside wraps around the floated elements,
- in mutli-columns layouts, the parent also is just one box, while the text inside it is split in multiple boxes.

So for theses reasons it just makes sense to be highlighting text nodes in the page.
Today, if you hover over a text node in the markup-view, nothing happens, with this bug, we'd highlight the boxes generated for that text node.

Also, this is very low risk, because this wouldn't change any existing behavior when highlighting elements.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: text-node-highlight.html

A few test cases for this. Right now nothing happens when you hover the text nodes in this test case. Hopefully when this bug is fixed, we should highlight the boxes in the page.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: Bug 1304679 - Box-model highlighter now highlights text nodes;

Review commit: https://reviewboard.mozilla.org/r/80416/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80416/

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8793737 [details]
Bug 1304679 - Box-model highlighter now highlights text nodes;

https://reviewboard.mozilla.org/r/80416/#review79086

::: devtools/client/inspector/test/browser_inspector_highlighter-comments.js:86
(Diff revision 1)
>    }
>  
> +  function hoverTextNode(text) {
> +    info(`Hovering the text node "${text}" in the markup view`);
> +    let container = [...markupView._containers].filter(([nodeFront]) => {
> +      return nodeFront.nodeType === 3 && nodeFront._form.nodeValue.trim() === text.trim();

Ci.nsIDOMNode.TEXT_NODE should be used instead of 3.

::: devtools/server/actors/highlighters/box-model.js:10
(Diff revision 1)
>  "use strict";
>  
>  const { extend } = require("sdk/core/heritage");
>  const { AutoRefreshHighlighter } = require("./auto-refresh");
> -const {
> -  CanvasFrameAnonymousContentHelper, moveInfobar,
> +const { CanvasFrameAnonymousContentHelper, moveInfobar, getBindingElementAndPseudo,
> +        hasPseudoClassLock, createSVGNode, createNode, isNodeValid } = require("./utils/markup");

This change makes it go over 90 character limit. I usually prefer 1 function import per line as seen in css-grid.js to keep from formatting our requires every time we import more functions.

::: devtools/server/actors/highlighters/utils/markup.js:138
(Diff revision 1)
>    if (!node || Cu.isDeadWrapper(node)) {
>      return false;
>    }
>  
> -  // Is it an element node
> -  if (node.nodeType !== node.ELEMENT_NODE) {
> +  // Is it of the right type?
> +  nodeType = nodeType || node.ELEMENT_NODE;

I think we can set a default value in the parameters instead. 
function isNodeValid(node, nodeType =  Ci.nsIDOMNode.ELEMENT_NODE)

Comment 4

[Patrick Brosset <:pbro>]

(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #3)
> Comment on attachment 8793737 [details]
> Bug 1304679 - Box-model highlighter now highlights text nodes;
> 
> https://reviewboard.mozilla.org/r/80416/#review79086
> 
> :::
> devtools/client/inspector/test/browser_inspector_highlighter-comments.js:86
> (Diff revision 1)
> >    }
> >  
> > +  function hoverTextNode(text) {
> > +    info(`Hovering the text node "${text}" in the markup view`);
> > +    let container = [...markupView._containers].filter(([nodeFront]) => {
> > +      return nodeFront.nodeType === 3 && nodeFront._form.nodeValue.trim() === text.trim();
> 
> Ci.nsIDOMNode.TEXT_NODE should be used instead of 3.
Good catch, will change it.

> ::: devtools/server/actors/highlighters/box-model.js:10
> (Diff revision 1)
> >  "use strict";
> >  
> >  const { extend } = require("sdk/core/heritage");
> >  const { AutoRefreshHighlighter } = require("./auto-refresh");
> > -const {
> > -  CanvasFrameAnonymousContentHelper, moveInfobar,
> > +const { CanvasFrameAnonymousContentHelper, moveInfobar, getBindingElementAndPseudo,
> > +        hasPseudoClassLock, createSVGNode, createNode, isNodeValid } = require("./utils/markup");
> 
> This change makes it go over 90 character limit. I usually prefer 1 function
> import per line as seen in css-grid.js to keep from formatting our requires
> every time we import more functions.
We have a special option in our ESLint config to ignore requires, so that we can make them as wide as we want. I don't know if we have a common way of writing these long imports in our code base. I'll check.

> ::: devtools/server/actors/highlighters/utils/markup.js:138
> (Diff revision 1)
> >    if (!node || Cu.isDeadWrapper(node)) {
> >      return false;
> >    }
> >  
> > -  // Is it an element node
> > -  if (node.nodeType !== node.ELEMENT_NODE) {
> > +  // Is it of the right type?
> > +  nodeType = nodeType || node.ELEMENT_NODE;
> 
> I think we can set a default value in the parameters instead. 
> function isNodeValid(node, nodeType =  Ci.nsIDOMNode.ELEMENT_NODE)
That's what I did originally, but since I was using node.ELEMENT_NODE and since node may be a DeadWrapper, that was causing errors. But you're right I can totally use Ci.nsIDOMNode.ELEMENT_NODE instead. Will do.

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8793737 [details]
Bug 1304679 - Box-model highlighter now highlights text nodes;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/80416/diff/1-2/

Comment 6

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9db552cc7727&group_state=expanded

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/2e46d0c12db6
Box-model highlighter now highlights text nodes; r=gl

Comment 8

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/2e46d0c12db6

Comment 9

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 52.0a1 (2016-09-22) (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]

Comment 10

[Mihai Boldan, Desktop QA [:mboldan]]

Thanks Maruf Rahman for helping us.
I've also verified this bug on Firefox 53.0a1 (2017-01-04) and Firefox 52.0a2 (2017-01-04), under Mac OS X 10.12.1 and under Ubuntu 14.04x64.