The highlighter should also highlight text nodes in the page

VERIFIED FIXED in Firefox 52

Status

()

Firefox
Developer Tools: Inspector
VERIFIED FIXED
11 months ago
8 months ago

People

(Reporter: pbro, Assigned: pbro)

Tracking

unspecified
Firefox 52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 verified, firefox53 verified)

Details

MozReview Requests

()

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

Attachments

(3 attachments)

(Assignee)

Description

11 months ago
Created attachment 8793720 [details]
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.
(Assignee)

Comment 1

11 months ago
Created attachment 8793721 [details]
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.
(Assignee)

Updated

11 months ago
Blocks: 1304685
(Assignee)

Updated

11 months ago
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 3

11 months ago
mozreview-review
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)
Attachment #8793737 - Flags: review?(gl) → review+
(Assignee)

Comment 4

11 months ago
(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 hidden (mozreview-request)
(Assignee)

Comment 6

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

Comment 7

11 months ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/2e46d0c12db6
Box-model highlighter now highlights text nodes; r=gl

Comment 8

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2e46d0c12db6
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
(Assignee)

Updated

11 months ago
Depends on: 1305401
(Assignee)

Updated

10 months ago
Blocks: 1309212

Comment 9

8 months ago
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]
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.
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
status-firefox53: --- → verified
You need to log in before you can comment on or make changes to this bug.