Closed Bug 1046803 Opened 10 years ago Closed 9 years ago

Markup view: too much whitespace in text node preview

Categories

(DevTools :: Inspector, defect, P1)

defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: fvsch, Assigned: bgrins)

References

Details

(Whiteboard: [polish-backlog][difficulty=medium])

Attachments

(8 files, 2 obsolete files)

SUMMARY: text node previews (not the corresponding editing <textarea>s) shouldn’t be rendered using <pre>. They should be rendered using white-space:normal, to avoid surprising renderings that make it harder to browse the document tree.

See attachment whitespace-example1.png for an example of the current result and of the desired result.


RATIONALE

The inspector’s text node preview displays the first 50 characters or so of the text node in a <pre> element. When the text has a lot of whitespace, especially line breaks, this can take up a lot of space for little value, and make the value hard to parse visually.

This common markup:

<h2>
  My title
</h2>
<p>
  Published
  <time>two days ago</time>
  by
  <span>Author</span>
</p>

is visually rendered with 5 blank lines in the markup view.
This result:
1. takes up more space than is necessary for a useful result;
2. looks quite different from the source code anyway.

Web pages are full of more extreme cases, such as:
- multiple blank lines between tags and the non-whitespace content;
- deep indenting being displayed in the preview (for instance, 6 tab characters push the text preview more than 300px to the right).

Especially with templating engines, PHP, etc., source code may have a lot of extraneous whitespace that is generally not useful to display.


PROPOSED SOLUTION, with remarks

I suggest moving to just using white-space:normal. This will give a useful rendering that is not jarring most of the time.

-> What about the editing mode?
It probably shouldn’t change. This means that when double-clicking to go from preview to edit mode, the text can move around quite a bit. I think that’s an acceptable trade-off.
Separately from this issue, the editing mode could have a few improvements such as removal of extraneous indentation (then the text wouldn’t jump around much).

-> But seeing whitespace can explain rendering issues!
It’s true that the presence or absence of whitespace at the beginning and end of a text node can alter the layout (when the text node is preceded or followed by a display:inline|inline-block|inline-table element). But as of now the use of <pre> doesn’t show the presence or absence of such whitespace efficiently anyway:
- The final line break (if any) is not displayed.
- Whitespace at the end won't be displayed if the string is longer than 50 characters.
- If the whitespace at the beginning is just one or two spaces, it will be visible but hard to identify.
If it is indeed useful for layout debugging, "Highlight whitespace at the beginning|end of a text node when that text node is preceded|followed by a display:inline[-*] element" should be a separate feature.

-> What about <pre> elements?
No special treatment for <pre> elements or any element with white-space other than normal. There’s probably little value in showing a "faithful" preview of a <pre>’s text content in the text node preview, since it’s just 50 characters long or so.

-> Should this be a devtools option?
No idea. If it is implemented as an option, I suggest making white-space:normal the default for text node previews.
The trimmed whitespace indeed looks much better.  What do you think about editing these text nodes?  I would assume that all actual whitespace should be restored once double clicking to edit
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hardware: x86 → All
Semi related to this is Bug 892935, where we should be showing the contents of a node immediately if it is only text.
See Also: → 892935
Comparison of https://bug1046803.bugzilla.mozilla.org/attachment.cgi?id=8465485 between chrome/firebug/fx devtools.  Of the 3, Firebug seems most like what you are suggesting.

Main differences to note:
1) On the <h1> which just has text but with whitespace around it, Chrome is adding it on a new line, Firebug is on the same line, but with spacing.  We are showing it with all of the whitespace
2) On text nodes that have element siblings, Chrome is showing the whitespace as-is, using quotes to show when it begins and ends.  Firebug is showing it as you suggest (white-space:normal), and we are showing it as-is but with no delimeters.

It seems like it comes down to tidiness in the tree vs accuracy of representing the content as-is.  We are never going to be 100% accurate with the page source, so in general I think I'd be OK with using the more compact layout.  Though I'm guessing that the most likely place that could cause confusion (as you mention in the description) would be with an element that is using pre whitespace and causing layout issues on the page or something.

Clearly we need to also improve the tree output in a couple of ways:

1) auto-expanding the <time> element - to be done in Bug 892935
2) moving single text children inline with the tag name (like the "Author" span in the screenshot)
> What do you think about editing these text nodes? I would assume that all actual whitespace should be restored once double clicking to edit

Yes, showing all the whitespace. (No need to "restore" it in a technical sense, since it never was changed if we use `white-space:normal` or swap the <pre> for a <span>.)

> Though I'm guessing that the most likely place that could cause confusion (as you mention in the description) would be with an element that is using pre whitespace and causing layout issues on the page or something.

Actually I don’t think that would be the main source of confusion. Not many content on typical web pages and applications use `white-space: pre|pre-wrap|pre-line;`, and when they do the element is often displayed on the page so it’s easy to see the whitespace: you mostly look at the text rendered on the page.

The main source of confusion, which only Chrome seems to prevent in your comparison, is this scenario:

<p>
  "
     AAA
  "
  <span>BBB</span>
  " CCC"
</p>

… would be displayed as "AAA BBB CCC", given the common scenario that all elements are `white-space:normal`.
While this:

<p>
  "        AAA"
  <span>BBB</span>
  "               CCC   "
</p>

… would be displayed as "AAABBB CCC".

There are cases where we want one rendered space character between a text node and the next inline element. And others where we don't want any space between those. So the information "there is some white space" is relevant, though showing all the spaces and tabulations doesn't help much and mostly creates empty lines and text that looks out of place in the tree.

One solution would be to use a structure such as:

<!-- Editing: -->
<textarea>{{ node.textContent }}</textarea>
<!-- Displaying: -->
<span style="white-space:pre-wrap">"{{ node.textContent.replace(/\s+/g,' ') }}"</span>

The result is very similar to just using `white-space:normal`, with the difference that we’re showing a space character at any end where there were whitespace characters.

The two examples above then become:

<p>
  " AAA "
  <span>BBB</span>
  " CCC"
</p>

<p>
  " AAA"
  <span>BBB</span>
  " CCC "
</p>

And voilà, you get a hint of which text nodes have initial/trailing whitespace that you don’t want, or alternatively lack initial/trailing whitespace that you need.
This only works if we display quote marks though. For a style without quote marks, we would need a initial/trailing whitespace symbol.
(In reply to Florent Verschelde from comment #6)
> This only works if we display quote marks though. For a style without quote
> marks, we would need a initial/trailing whitespace symbol.

Really a sidenote and not part of this bug, but I also just thought about whitespace around inline-block elements being a really common and hard to diagnose problem.

Given this markup it may actually be useful to highlight the whitespace between the <li> tags using some kind of symbol:

<style>
nav a {
  display: inline-block;
  padding: 5px;
  background: red;
}
</style>

<nav>
  <a href="#">One</a>
  <a href="#">Two</a>
  <a href="#">Three</a>
</nav>
(In reply to Florent Verschelde from comment #6)
> One solution would be to use a structure such as:
> 
> <!-- Editing: -->
> <textarea>{{ node.textContent }}</textarea>
> <!-- Displaying: -->
> <span style="white-space:pre-wrap">"{{ node.textContent.replace(/\s+/g,' ')
> }}"</span>
> 
> The result is very similar to just using `white-space:normal`, with the
> difference that we’re showing a space character at any end where there were
> whitespace characters.
> 
> The two examples above then become:
> 
> <p>
>   " AAA "
>   <span>BBB</span>
>   " CCC"
> </p>
> 
> <p>
>   " AAA"
>   <span>BBB</span>
>   " CCC "
> </p>
> 
> And voilà, you get a hint of which text nodes have initial/trailing
> whitespace that you don’t want, or alternatively lack initial/trailing
> whitespace that you need.
> This only works if we display quote marks though. For a style without quote
> marks, we would need a initial/trailing whitespace symbol.

I like that idea.  The quote marks / whitespace symbols would be necessary if it has sibling element nodes, but I think we could get away without them if the only content in an element is text nodes (see the <h1> in Firebug in https://bugzilla.mozilla.org/attachment.cgi?id=8465699, for example).
> I like that idea.  The quote marks / whitespace symbols would be necessary if it has sibling element nodes, but I think we could get away without them if the only content in an element is text nodes

Agreed. The Firebug way looks good.

There might even be a way to display `white-space:pre[-*]` content in a somewhat faithful fashion.

Let's say that:
- node_text_raw_200 is the node’s raw textContent, truncated to 200 characters.
- node_text_collapsed_50 is the textContent with whitespace sequences collapsed as a single space (NOT trimming the start and end spaces, if any), and truncated to 50 characters. Truncation should NOT remove the last space if any ("  Shorten this  " -> " Shorte… " and not " Shorte…").
- whitespace_value is the parent’s computed value for `white-space`.

1. If the text node is an only child AND the whitespace_value is "normal" or "nowrap" AND node_text_collapsed is short enough, display in the tree as:
<parent>UNQUOTED_TEXT</parent> (no quotes)
… where UNQUOTED_TEXT is:
<span style="white-space:pre-wrap">{{ node_text_collapsed_50 }}</span>

In all other cases:

2. Display in the tree as:
<parent>
  [previous siblings if any]
  QUOTED_TEXT
  [next siblings if any]
</parent>

2.1. If the whitespace_value is "normal" or "nowrap", QUOTED_TEXT is:
<span style="white-space:pre-wrap">&quot;{{ node_text_collapsed_50 }}&quot;</span>

2.2. If whitespace_value is "pre" or "pre-wrap" or "pre-line", QUOTED_TEXT is:
<pre>&quot;{{ node_text_raw_200 }}&quot;</pre>

That way (2.2), we can show a more informative preview of `white-space:pre[-*]` content, with `white-space:pre` formatting and more characters in the preview if that makes sense. It might very well be overkill.
(In reply to Brian Grinstead [:bgrins] from comment #7)
> Really a sidenote and not part of this bug, but I also just thought about
> whitespace around inline-block elements being a really common and hard to
> diagnose problem.

Since we're brainstorming. :)

Taking this HTML code:

<nav>
  <a href="/1/">
    <strong>One</strong>
  </a>
  <a href="/2/">
    Two
  </a>
  <a href="/3/">Three</a>
  Four
</nav>

A corresponding tree when opening the <nav> element could be:

<nav>
▸ <a href="/1/"></a>
  " "
  <a href="/2/"> Two </a>
  " "
  <a href="/3/">Three</a>
  " Four "
</nav>

The collapsed “whitespace” text nodes would only be shown if both their previous and next sibling are element nodes, with a `display` value such as "inline", "inline-block", and "inline-table".

Another option would be to put those indicative “whitespace” text nodes after the closing tag of the previous sibling. Also, let’s say that "·" (MIDDLE DOT) is our symbol for collapsed whitespace at the beginning or end of a text node, which allows us to ditch the quote marks. We could have:

<nav>
▸ <a href="/1/"></a> [·]
  <a href="/2/">·Two·</a> [·]
  <a href="/3/">Three</a>
</nav>

(the [square brackets] are only there to indicate a formatting of some kind, such as borders, a background, a visible color…).

And when opening the first <a> element:

<nav>
▾ <a href="/1/">
    <strong>One</strong>
  </a> [·]
  <a href="/2/">·Two·</a> [·]
  <a href="/3/">Three</a>
  ·Four·
</nav>

Or putting the whitespace-only nodes before the next node:

<nav>
▸ <a href="/1/"></a>
  [·] <a href="/2/">·Two·</a>
  [·] <a href="/3/">Three</a>
  ·Four·
</nav>
(In reply to Brian Grinstead [:bgrins] from comment #7)
> (In reply to Florent Verschelde from comment #6)
> > This only works if we display quote marks though. For a style without quote
> > marks, we would need a initial/trailing whitespace symbol.
> 
> Really a sidenote and not part of this bug, but I also just thought about
> whitespace around inline-block elements being a really common and hard to
> diagnose problem.
> 
> Given this markup it may actually be useful to highlight the whitespace
> between the <li> tags using some kind of symbol:
...

Some prior art for this is Komodo / Scintilla's 'show whitespace' and 'show EOL marker' features, see this screenshot:

http://note.io/1nXRUGc

You would never turn this on by default, but it's really handy every once in a while ( one reason I occasionally open Komodo Edit these days )
(In reply to Florent Verschelde from comment #9)
> > I like that idea.  The quote marks / whitespace symbols would be necessary if it has sibling element nodes, but I think we could get away without them if the only content in an element is text nodes
> 
> Agreed. The Firebug way looks good.
> 
> There might even be a way to display `white-space:pre[-*]` content in a
> somewhat faithful fashion.
> 
> Let's say that:
> - node_text_raw_200 is the node’s raw textContent, truncated to 200
> characters.
> - node_text_collapsed_50 is the textContent with whitespace sequences
> collapsed as a single space (NOT trimming the start and end spaces, if any),
> and truncated to 50 characters. Truncation should NOT remove the last space
> if any ("  Shorten this  " -> " Shorte… " and not " Shorte…").
> - whitespace_value is the parent’s computed value for `white-space`.
> 
> 1. If the text node is an only child AND the whitespace_value is "normal" or
> "nowrap" AND node_text_collapsed is short enough, display in the tree as:
> <parent>UNQUOTED_TEXT</parent> (no quotes)
> … where UNQUOTED_TEXT is:
> <span style="white-space:pre-wrap">{{ node_text_collapsed_50 }}</span>
> 
> In all other cases:
> 
> 2. Display in the tree as:
> <parent>
>   [previous siblings if any]
>   QUOTED_TEXT
>   [next siblings if any]
> </parent>
> 
> 2.1. If the whitespace_value is "normal" or "nowrap", QUOTED_TEXT is:
> <span style="white-space:pre-wrap">&quot;{{ node_text_collapsed_50
> }}&quot;</span>
> 
> 2.2. If whitespace_value is "pre" or "pre-wrap" or "pre-line", QUOTED_TEXT
> is:
> <pre>&quot;{{ node_text_raw_200 }}&quot;</pre>
> 
> That way (2.2), we can show a more informative preview of
> `white-space:pre[-*]` content, with `white-space:pre` formatting and more
> characters in the preview if that makes sense. It might very well be
> overkill.

FWIW, we were planning on turning up the default minimum length from 50 characters to 1000 characters or so to prevent ellipses in most cases, though the point of (1) still stands (that any trailing space should be added after the ellipses).  We may want to tweak this value to something that fits reasonably well into a few lines in the tree if we go with the inline display <parent>UNQUOTED_TEXT</parent>.

With regards to 2 we could special case any pre whitespace but as you say, it is probably not the primary use case.  Still, it seems like it wouldn't be that hard of a case to add.

Regarding the ability to show whitespace around the edges of the string, is there any practical difference between:

  <span style="white-space:pre-wrap">&quot;{{ node.textContent.replace(/\s+/g,' ') }}&quot;</span>

And:

  <span>&quot;{{ node.textContent }}&quot;</span>

Seems like just using a normal element would achieve the same "trim multiple spaces" effect that we would get by using pre-wrap with a regex.
(In reply to Brian Grinstead [:bgrins] from comment #12)
> Regarding the ability to show whitespace around the edges of the string, is
> there any practical difference between:
> 
>   <span style="white-space:pre-wrap">&quot;{{
> node.textContent.replace(/\s+/g,' ') }}&quot;</span>
> 
> And:
> 
>   <span>&quot;{{ node.textContent }}&quot;</span>
> 
> Seems like just using a normal element would achieve the same "trim multiple
> spaces" effect that we would get by using pre-wrap with a regex.

Yes. If the text node is, say, '\n\t\t\tTest':
- The 1st one will display: " Test"
- The 2nd one will display: "Test"

With the first one you get the information that there is some whitespace that *may* impact the layout. With the second one you lose this information, and can only get it by clicking through to edit mode.

This is probably why the Chrome devtools are showing all the (uncollapsed) whitespace for text nodes, because that information is valuable for debugging layout issues, especially with display:inline[-*] elements.

I’m suggesting collapsing the whitespace (in order to make the tree more readable) but still leaving an indication that there is whitespace at the beginning and/or end of the text node. A middle ground solution. Of course there are several possible implementations, the regex+pre-wrap might not be the best one.
(In reply to Florent Verschelde from comment #13)
> (In reply to Brian Grinstead [:bgrins] from comment #12)
> > Regarding the ability to show whitespace around the edges of the string, is
> > there any practical difference between:
> > 
> >   <span style="white-space:pre-wrap">&quot;{{
> > node.textContent.replace(/\s+/g,' ') }}&quot;</span>
> > 
> > And:
> > 
> >   <span>&quot;{{ node.textContent }}&quot;</span>
> > 
> > Seems like just using a normal element would achieve the same "trim multiple
> > spaces" effect that we would get by using pre-wrap with a regex.
> 
> Yes. If the text node is, say, '\n\t\t\tTest':
> - The 1st one will display: " Test"
> - The 2nd one will display: "Test"

Right, I was thinking for some reason the second would also show " Test" in this case, but I see that it won't now.  My suggestion would be that we proceed with a patch that follows the outline, without worrying about case 1 (where the value is displayed inline).  That can be handled separately and would add some extra complication to the patch.

> With the first one you get the information that there is some whitespace
> that *may* impact the layout. With the second one you lose this information,
> and can only get it by clicking through to edit mode.
> This is probably why the Chrome devtools are showing all the (uncollapsed)
> whitespace for text nodes, because that information is valuable for
> debugging layout issues, especially with display:inline[-*] elements.
> 
> I’m suggesting collapsing the whitespace (in order to make the tree more
> readable) but still leaving an indication that there is whitespace at the
> beginning and/or end of the text node. A middle ground solution. Of course
> there are several possible implementations, the regex+pre-wrap might not be
> the best one.

Bug 777674 has a bunch of changes to the templating for text nodes / comments so it would be best to wait to land anything until after that to avoid conflicts, but that shouldn't block from building a simple patch to test / get feedback on.
Attached patch markupview-whitespace-WIP.patch (obsolete) — Splinter Review
Just an idea of how a patch may look
Attached image with-wip-patch-applied.png (obsolete) —
I'm not really sure if we should be quoting the text under the html tag in this case (though I think if we collapsed close tag onto same line this one could look like the Firebug case and get the point across without quotes).
I just stumbled upon a problem that is closely related to this bug, so here is a video to understand better.
Basically, if a textnode has a long value, only a substring is shown at first, and the full value is shown when the node is selected, including white spaces.
So in some cases, this may make the actual markup container increase its size suddenly, which makes it really hard when you want to double-click to edit the value.
Blocks: 1090373
Whiteboard: [devedition-40]
Whiteboard: [devedition-40] → [devedition-40][difficulty=medium]
Setting devedition-40 bugs to p3, filter on FB20EAC4-3FC3-48D9-B15F-0587C3987C25
Priority: -- → P3
Priority: P3 → P1
After playing with this in the frontend some more, I think `white-space: normal;` will be the easiest and most consistent way to handle this
See Also: → 1153022
Depends on: 1153022
See Also: 1153022
Requires the patch from Bug 1153022 to be applied
Assignee: nobody → bgrinstead
Attachment #8466359 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Comment on attachment 8598979 [details] [diff] [review]
markup-view-whitespace-WIP.patch

Patrick, what do you think about this approach vs the last one?  It's definitely a lot simpler than that and it seems fine based on some simple usage (and all existing tests still pass).  This requires the patch from Bug 1153022 to be applied.
Attachment #8598979 - Flags: feedback?(pbrosset)
Comment on attachment 8598979 [details] [diff] [review]
markup-view-whitespace-WIP.patch

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

Yes! Love it!
Attachment #8598979 - Flags: feedback?(pbrosset) → feedback+
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #23)
> Comment on attachment 8598979 [details] [diff] [review]
> markup-view-whitespace-WIP.patch
> 
> Review of attachment 8598979 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yes! Love it!

What do you think we should do as far as tests?  It's basically a CSS change..
Flags: needinfo?(pbrosset)
(In reply to Brian Grinstead [:bgrins] from comment #24)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #23)
> > Comment on attachment 8598979 [details] [diff] [review]
> > markup-view-whitespace-WIP.patch
> > 
> > Review of attachment 8598979 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Yes! Love it!
> 
> What do you think we should do as far as tests?  It's basically a CSS
> change..
I don't see any good way to test this apart from using a reftest. I don't think we have any of these in devtools, and I have never created one myself before. I'd be ok to r+ this without a test.
Flags: needinfo?(pbrosset)
Attachment #8598979 - Flags: review?(pbrosset)
Comment on attachment 8598979 [details] [diff] [review]
markup-view-whitespace-WIP.patch

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

Yes! Love it!

::: browser/devtools/markupview/markup-view.xhtml
@@ +84,5 @@
>     --></span><!--
>   --></span>
>  
>      <span id="template-text" save="${elt}" class="editor text">
> +      <pre save="${value}" style="display:inline-block; white-space: normal;" tabindex="0"></pre>

Any reason these styles are inline rather than in markupview.css?
I think they should be moved there.
Attachment #8598979 - Flags: review?(pbrosset) → review+
Comment on attachment 8598979 [details] [diff] [review]
markup-view-whitespace-WIP.patch

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

::: browser/devtools/markupview/markup-view.xhtml
@@ +84,5 @@
>     --></span><!--
>   --></span>
>  
>      <span id="template-text" save="${elt}" class="editor text">
> +      <pre save="${value}" style="display:inline-block; white-space: normal;" tabindex="0"></pre>

I don't know why, but if I move these into the css file targeted on .editor.text pre {}, then the textarea isn't sized properly when editing the <p> tag on https://bgrins.github.io/devtools-demos/ but it does work when it's an inline style.
Try push looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cad590d62dba.  Going to check in and file a follow up to move the inline styles out of that file.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/de4547be239c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Attachment #8466362 - Attachment is obsolete: true
Attached image whitespace-before.png
Screenshot without the patch
Attached image whitespace-after.png
Screenshot with the patch
Blocks: 1166506
Whiteboard: [devedition-40][difficulty=medium] → [polish-backlog][difficulty=medium]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.