Help debug spacing issues caused by whitespace text nodes

RESOLVED FIXED in Firefox 52

Status

()

Firefox
Developer Tools: Inspector
P2
enhancement
RESOLVED FIXED
11 months ago
2 months ago

People

(Reporter: pbro, Assigned: pbro)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug, {dev-doc-needed})

unspecified
Firefox 52
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

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

Attachments

(7 attachments, 3 obsolete attachments)

(Assignee)

Description

11 months ago
Created attachment 8793727 [details]
whitespace-spacing-issues.html

Whitespace text nodes are very common nodes in any DOM tree, simply because you tend to format your HTML markup like this:

<body>
  <p>a paragraph</p>
  <p>another one</p>
</body>

If you looked at the DOM tree inside <body> that's generated from the above markup, you'd see that body has 5 childNodes: 3 text nodes (the spaces and line breaks) and 2 element nodes.

Various browser devtools don't show these empty text nodes, because people usually don't care about them, they're just formatting information that's useless in the inspector. They would unnecessarily clutter the inspector.

Now, of course, that's not always true, hence this bug.

Whitespace text nodes inside inline formatting contexts are a very common source of frustration and causes problems that are really hard to investigate.

Take the attached test case for example, you'll see that there's an empty space between each and every element in the page. At least that's how it seems if you open the inspector, because you're not seeing the text nodes.
Searching in the rule-view for a potential margin doesn't help because there's none. What usually happens is you spend 15 minutes trying to figure this out, searching for answers on google, and finally facepalming yourself realizing it's just an extra space in the markup somewhere ...

I'd like us to figure out a way to help with this common problem.

The issue here is that people loose time because they're not thinking that this could come from whitespace text nodes, so having a mode where you ask people to switch ON whitespace text nodes wouldn't help. By then, they'd have figured out the problem and moved on.
So maybe we should show these nodes in inline elements only? Or show them only when they actually have boxes generated for them.

For info, whitespace text nodes are filtered out by the walker actor here http://searchfox.org/mozilla-central/rev/8910ca900f826a9b714607fd23bfa1b37a191eca/devtools/server/actors/inspector.js#2961-2965
(Assignee)

Comment 1

11 months ago
Created attachment 8793729 [details] [diff] [review]
show_empty_text_nodes.diff

So, I just experimented with this and I think the following solution is actually pretty cool:
instead of always skipping whitespace text nodes, only skip them if they have a width and height of 0.
This will happen in a block formatting context for example, whitespace text nodes have no effect there.
But inside an inline context, they will have a non-null width and height. So we can use that information to show them.

If, on top of this, you have the patch in bug 1304679 applied, then you'd even be able to highlight the whitespace text node and see it in then page!

Right now, whitespace text nodes appear as empty lines in the markup-view though, which isn't great, and you can't edit the text content. We should find a proper way to display them.

But I believe this would solve the problem described in comment 0. People would see that there's a node here, would see where it is in the page, and would even be able to delete it by pressing BACKSPACE on their keyboards.
(Assignee)

Comment 2

11 months ago
Created attachment 8793730 [details]
whitespace-textnodes-with-patch-applied.PNG
(Assignee)

Comment 3

11 months ago
Comment on attachment 8793729 [details] [diff] [review]
show_empty_text_nodes.diff

What do you think of this feature (needs bug 1304679 applied too for maximum effect)?
Attachment #8793729 - Flags: feedback?(jdescottes)
Created attachment 8794103 [details]
whitespace-char-markup-view.png

(In reply to Patrick Brosset <:pbro> from comment #1)
> Created attachment 8793729 [details] [diff] [review]
> show_empty_text_nodes.diff
> 
> So, I just experimented with this and I think the following solution is
> actually pretty cool:
> instead of always skipping whitespace text nodes, only skip them if they
> have a width and height of 0.
> This will happen in a block formatting context for example, whitespace text
> nodes have no effect there.
> But inside an inline context, they will have a non-null width and height. So
> we can use that information to show them.
> 

Great idea!

> If, on top of this, you have the patch in bug 1304679 applied, then you'd
> even be able to highlight the whitespace text node and see it in then page!
> 
> Right now, whitespace text nodes appear as empty lines in the markup-view
> though, which isn't great, and you can't edit the text content. We should
> find a proper way to display them.
> 

What about showing a whitespace "dot" character similar to the ones you can find in text editors? 
We'll need to make sure this does not look like "regular" text. I'm thinking faded out font-color and maybe a border around it. (see screenshot here)

> But I believe this would solve the problem described in comment 0. People
> would see that there's a node here, would see where it is in the page, and
> would even be able to delete it by pressing BACKSPACE on their keyboards.

I agree, whitespace gaps can be very frustrating to debug.

We could go a step further and show even more whitespace characters! In your example, take the "some text" text node. There are gaps before and after it because of whitespace characters, even though they don't create individual text nodes of their own. Maybe we could have a toggle to show "all whitespace characters"?
Comment on attachment 8793729 [details] [diff] [review]
show_empty_text_nodes.diff

Review of attachment 8793729 [details] [diff] [review]:
-----------------------------------------------------------------

See comment above.
Attachment #8793729 - Flags: feedback?(jdescottes) → feedback+
(Assignee)

Comment 6

11 months ago
Inspector bug triage (filter on CLIMBING SHOES).
Severity: normal → enhancement
Priority: -- → P2
(Assignee)

Updated

11 months ago
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
(Assignee)

Comment 7

11 months ago
Created attachment 8794870 [details]
whitespace-text-nodes.png

Helen, see comment 0 for an explanation of why I'm working on this.
Also, in a little while, there should be builds available here to test this out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=65f8a9935c7c
And, finally, here's a screenshot of what it currently looks like.
Attachment #8794870 - Flags: ui-review?(hholmes)
(Assignee)

Comment 8

11 months ago
Here's a build for Mac: https://archive.mozilla.org/pub/firefox/try-builds/pbrosset@mozilla.com-65f8a9935c7c538d44ea1e9222a82a3ffd7cda83/try-macosx64/firefox-52.0a1.en-US.mac.dmg
Comment on attachment 8794870 [details]
whitespace-text-nodes.png

I really liked Julian's suggestion of showing white space nodes like Sublime/other code editors do; but I worry we're conflating ideas here.

"◦" typically denotes the white space you get from hitting the space bar.

Otherwise, white space from returns is normally a "⏎".

I'm not certain what's causing the white space nodes to be created here; looks like a return. If that's the case, I'd use the second symbol ("⏎").

If the white space node is being caused by a carriage return or a space, it might be helpful to label it as such so the user knows to go to their editor and delete it—"Space", or, "Carriage return", even if "Whitespace-only text node" is technically more descriptive.
Attachment #8794870 - Flags: ui-review?(hholmes) → feedback+
(Assignee)

Comment 10

11 months ago
I think you're right, we should find a symbol that does not only denote a space.
But note that whether you have 1 space only, or a lot of spaces, returns and tabs makes no difference to the layout.
For instance, consider the 2 following URLs:

data:text/html,<span></span><script>document.querySelector("span").innerHTML = "<img src='chrome://global/skin/icons/warning-64.png'> <img src='chrome://global/skin/icons/warning-64.png'>";</script>

data:text/html,<span></span><script>document.querySelector("span").innerHTML = "<img src='chrome://global/skin/icons/warning-64.png'>\n\n \n\n\n\n\n\n    \t  \t\t\t                  \n\n\n  \t\t\<img src='chrome://global/skin/icons/warning-64.png'>";</script>

If you open those in 2 tabs, you'll see the exact same result, even though one has just 1 space character separating the 2 images, and the other has a myriad of tabs, returns and spaces.

So, I don't think we need to worry too much about representing which characters are present in a whitespace text node, because they have no impact on the size or shape of this text node. I think the most important here is to show users that there is a whitespace text node.
Then they can always double-click on it to get the usual textarea editor, and they'll see what's in the textnode exactly.

So I think we need a simple way to illustrate that a given line in the markupview is in fact a text node composed only of tabs, spaces or returns.
Flags: needinfo?(hholmes)
Created attachment 8796073 [details]
whitespace_nodes_proposals.png

Some other suggestions :
- empty circle
- empty rectangle
- rectangle with \s (everybody loves regex right?)
- rectangle with "whitespace"
 
Of course with tooltips to help the user understand what this is about.

That being said, I agree with Patrick's comment that any sequence of whitespace characters ends up being treated as a single space for the browser. And we've said repeatedly that the markup view is not about displaying the authored markup but rather how it is interpreted by the browser. So I think we can still defend the whitespace character case here :)
Julian's small circle suggestion is really nice visually. That would be my pick for that reason.

I will make one last argument for the tooltip, which is that I still think it would be useful to state the whitespace the user has entered to create it there—that would be the most useful for debugging the space itself, I think.
Flags: needinfo?(hholmes)
(Assignee)

Comment 13

11 months ago
Created attachment 8796564 [details]
whitespaces.gif

Thanks for the feedback, here's what it looks like now with the changes implemented.
A small circle is used as the icon.
And hovering over it shows a tooltip that explains users that this is a whitespace-only text node and also gives the list of characters using ⏎ for return, ⇥ for tab and ◦ for space.
I haven't implemented a logic to cut the string after a certain length, I'm wondering if I should do this or not.
I'll post a patch up for review anyway.
(In reply to Patrick Brosset <:pbro> from comment #13)
> Created attachment 8796564 [details]
> whitespaces.gif
> 
> Thanks for the feedback, here's what it looks like now with the changes
> implemented.
> A small circle is used as the icon.
> And hovering over it shows a tooltip that explains users that this is a
> whitespace-only text node and also gives the list of characters using ⏎ for
> return, ⇥ for tab and ◦ for space.
> I haven't implemented a logic to cut the string after a certain length, I'm
> wondering if I should do this or not.
> I'll post a patch up for review anyway.

Looks awesome! 
(Assignee)

Updated

11 months ago
Duplicate of this bug: 1273000
Comment hidden (mozreview-request)
(Assignee)

Comment 17

10 months ago
Comment on attachment 8793729 [details] [diff] [review]
show_empty_text_nodes.diff

This old patch is now obsolete. I just pushed an up to date commit to mozreview and pinged Julian for a review.
Also, here's a try push for this commit: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a20aefc35be899c8615396e6cad63f5e3c1271f&group_state=expanded
Attachment #8793729 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Blocks: 1309014

Comment 18

10 months ago
mozreview-review
Comment on attachment 8799484 [details]
Bug 1304685 - Show empty text nodes in markupview if they impact layout;

https://reviewboard.mozilla.org/r/84648/#review83370

Great patch Patrick, thanks!

::: devtools/client/inspector/markup/markup.js:2956
(Diff revision 1)
> +
> +      let chars = str.replace(/\n/g, "⏎")
> +                     .replace(/\t/g, "⇥")
> +                     .replace(/ /g, "◦");
> +      this.value.setAttribute("title", isWhitespace
> +        ? INSPECTOR_L10N.getStr("markupView.whitespaceOnly") + " : " + chars

I think markupView.whitespaceOnly should have a placeholder for chars instead of combining the string here.

So INSPECTOR_L10N.getFormatStr("markupView.whitespaceOnly", chars).

And in the enUS locale I don't think we should have a whitespace before ":".

::: devtools/client/locales/en-US/inspector.properties:37
(Diff revision 1)
>  # LOCALIZATION NOTE (markupView.more.showAll2): Semi-colon list of plural forms.
>  # See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
>  markupView.more.showAll2=Show one more node;Show all #1 nodes
>  
> +# LOCALIZATION NOTE (markupView.whitespaceOnly)
> +# Show in a tooltip that appears when the user hovers over whitespace-only text nodes in

nit: (might be wrong but the tense feels weird) Show -> Shown/Used?

::: devtools/client/themes/markup.css:252
(Diff revision 1)
>    font: inherit;
>  }
>  
> +/* Whitespace only text nodes are sometimes shown in the markup-view, and when they do
> +   they get a greyed-out whitespace symbol so users know what they are */
> +.editor.text pre.whitespace {

Is "pre" needed for selector specificity here? If not best to avoid relying on tag names in selectors.

::: devtools/client/themes/markup.css:253
(Diff revision 1)
>  }
>  
> +/* Whitespace only text nodes are sometimes shown in the markup-view, and when they do
> +   they get a greyed-out whitespace symbol so users know what they are */
> +.editor.text pre.whitespace {
> +  padding-inline-end: 1em;

The whitespace symbol is currently aligned to the left, same as if it was a text node, which is nice. But if you focus it by clicking on it, the focus outline overlaps with the symbol and really shows the symbol is not centered in its container. 

Maybe padding: 0 .5em; instead? Will attach a screenshot of the difference.

::: devtools/server/actors/inspector.js:3016
(Diff revision 1)
> +    return false;
> +  }
> +
> +  let quads = node.getBoxQuads();
> +  return quads.length &&
> +         quads.filter(q => q.bounds.width && q.bounds.height).length > 0;

replace quads.filter by quads.some
Attachment #8799484 - Flags: review?(jdescottes) → review+
Attachment #8796073 - Attachment is obsolete: true
Attachment #8794103 - Attachment is obsolete: true
Created attachment 8799702 [details]
centered-whitespace-symbol.png

As mentioned in the review. Top: padding:0 .5em; Bottom: padding-inline-end: 1em;
Created attachment 8799703 [details]
whitespace-issues-continued.html

Not directly related to this bug, but yet another whitespace related issue.

If you inspect the divs from this page, you can see they are displayed in the exact same way in the markup view, no hint is available to explain where the gap between "hi," and "2" comes from. Maybe you already have an idea how such nodes could be differentiated.
Comment hidden (mozreview-request)
(Assignee)

Comment 22

10 months ago
Just pushed a rebased version that also addresses Julian's review comments.
Hopefully TRY is still happy about it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6624992b02ad6c865d3e91e1772552d2e4aa0f81
(Assignee)

Comment 23

10 months ago
(In reply to Patrick Brosset <:pbro> from comment #22)
> Just pushed a rebased version that also addresses Julian's review comments.
> Hopefully TRY is still happy about it:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=6624992b02ad6c865d3e91e1772552d2e4aa0f81
I messed up the rebase. One small change I had made in the previous commit didn't make it to the new commit, and so a test I had fixed before started to fail again. I'll address this now and land the change.
Comment hidden (mozreview-request)

Comment 25

10 months ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/fe1d7c28668f
Show empty text nodes in markupview if they impact layout; r=jdescottes
(Assignee)

Comment 26

10 months ago
(In reply to Julian Descottes [:jdescottes] from comment #20)
> Created attachment 8799703 [details]
> whitespace-issues-continued.html
> 
> Not directly related to this bug, but yet another whitespace related issue.
> 
> If you inspect the divs from this page, you can see they are displayed in
> the exact same way in the markup view, no hint is available to explain where
> the gap between "hi," and "2" comes from. Maybe you already have an idea how
> such nodes could be differentiated.
No I don't. It's true that we trim the text content before displaying it in the inspector, so you can't see that "2" is in fact " 2" (leading space) just by looking at the nodes in the inspector panel.
As you hover this text node though, the highlighter does show that the textnode covers more than just the character 2.
I'm not sure what we can do here. We can't not trim, because of line feeds and tabs. And replacing tabs, spaces and line feeds with ◦⇥⏎ would make it hard to see the actual non-whitespace content.

Comment 27

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fe1d7c28668f
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Posted: https://blog.nightly.mozilla.org/2016/10/17/devtools-now-display-white-space-text-nodes-in-the-dom-inspector/
Resources: https://github.com/mozdevs/devtools-demos/tree/master/debugging-inline-whitespace
Keywords: dev-doc-needed
I've added some stuff on this here: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_HTML#Whitespace-only_text_nodes (mostly stolen from you - so thanks!).

Please let me know if this covers it.
Flags: needinfo?(sole)

Updated

6 months ago
Depends on: 1337944

Updated

6 months ago
Depends on: 1341258

Updated

5 months ago
Depends on: 1345529

Updated

5 months ago
Depends on: 1347934
No longer depends on: 1347934

Updated

4 months ago
Depends on: 1355893
William - I just saw this needinfo now (!!?) - it looks perfect. Quietly removing the needinfo, months later... wooops 
Flags: needinfo?(sole)
You need to log in before you can comment on or make changes to this bug.