Closed Bug 1304685 Opened 8 years ago Closed 8 years ago

Help debug spacing issues caused by whitespace text nodes

Categories

(DevTools :: Inspector, enhancement, P2)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(firefox52 fixed)

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

People

(Reporter: pbro, Assigned: pbro)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(7 files, 3 obsolete files)

whitespace-spacing-issues.html
445 bytes, text/html
Details
whitespace-textnodes-with-patch-applied.PNG
92.94 KB, image/png
Details
whitespace-text-nodes.png
18.78 KB, image/png
hholmes
: feedback+
Details
whitespaces.gif
834.17 KB, image/gif
Details
Bug 1304685 - Show empty text nodes in markupview if they impact layout;
58 bytes, text/x-review-board-request
jdescottes
: review+
Details
centered-whitespace-symbol.png
105.40 KB, image/png
Details
whitespace-issues-continued.html
197 bytes, text/html
Details 
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
Attached patch show_empty_text_nodes.diff (obsolete) — — Splinter Review 
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.
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)
Attached image whitespace-char-markup-view.png (obsolete) — 
(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+
Inspector bug triage (filter on CLIMBING SHOES).
Severity: normal → enhancement
Priority: -- → P2
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
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)
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+
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)
Attached image whitespace_nodes_proposals.png (obsolete) — 
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)
Attached image 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! 
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
Blocks: 1309014
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
As mentioned in the review. Top: padding:0 .5em; Bottom: padding-inline-end: 1em;
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.
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
(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.
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
(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.
https://hg.mozilla.org/mozilla-central/rev/fe1d7c28668f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
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)
Depends on: 1337944
Depends on: 1341258
Depends on: 1345529
Depends on: 1347934
No longer depends on: 1347934
Depends on: 1355893
William - I just saw this needinfo now (!!?) - it looks perfect. Quietly removing the needinfo, months later... wooops
Flags: needinfo?(sole)
Product: Firefox → DevTools
See Also: → 1568847
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: