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)
DevTools
Inspector
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.
Reporter | ||
Comment 1•10 years ago
|
||
Reporter | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
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
Assignee | ||
Comment 4•10 years ago
|
||
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
Assignee | ||
Comment 5•10 years ago
|
||
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)
Reporter | ||
Comment 6•10 years ago
|
||
> 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.
Assignee | ||
Comment 7•10 years ago
|
||
(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>
Assignee | ||
Comment 8•10 years ago
|
||
(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).
Reporter | ||
Comment 9•10 years ago
|
||
> 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">"{{ node_text_collapsed_50 }}"</span>
2.2. If whitespace_value is "pre" or "pre-wrap" or "pre-line", QUOTED_TEXT is:
<pre>"{{ node_text_raw_200 }}"</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.
Reporter | ||
Comment 10•10 years ago
|
||
(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>
Comment 11•10 years ago
|
||
(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 )
Assignee | ||
Comment 12•10 years ago
|
||
(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">"{{ node_text_collapsed_50 > }}"</span> > > 2.2. If whitespace_value is "pre" or "pre-wrap" or "pre-line", QUOTED_TEXT > is: > <pre>"{{ node_text_raw_200 }}"</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">"{{ node.textContent.replace(/\s+/g,' ') }}"</span> And: <span>"{{ node.textContent }}"</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.
Reporter | ||
Comment 13•10 years ago
|
||
(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">"{{ > node.textContent.replace(/\s+/g,' ') }}"</span> > > And: > > <span>"{{ node.textContent }}"</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.
Assignee | ||
Comment 14•10 years ago
|
||
(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">"{{ > > node.textContent.replace(/\s+/g,' ') }}"</span> > > > > And: > > > > <span>"{{ node.textContent }}"</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.
Assignee | ||
Comment 15•10 years ago
|
||
Just an idea of how a patch may look
Assignee | ||
Comment 16•10 years ago
|
||
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).
Comment 17•10 years ago
|
||
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.
Updated•9 years ago
|
Whiteboard: [devedition-40]
Updated•9 years ago
|
Whiteboard: [devedition-40] → [devedition-40][difficulty=medium]
Comment 19•9 years ago
|
||
Setting devedition-40 bugs to p3, filter on FB20EAC4-3FC3-48D9-B15F-0587C3987C25
Priority: -- → P3
Assignee | ||
Updated•9 years ago
|
Priority: P3 → P1
Assignee | ||
Comment 20•9 years ago
|
||
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
Assignee | ||
Comment 21•9 years ago
|
||
Requires the patch from Bug 1153022 to be applied
Assignee: nobody → bgrinstead
Attachment #8466359 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Assignee | ||
Comment 22•9 years ago
|
||
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 23•9 years ago
|
||
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+
Assignee | ||
Comment 24•9 years ago
|
||
(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)
Comment 25•9 years ago
|
||
(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)
Assignee | ||
Updated•9 years ago
|
Attachment #8598979 -
Flags: review?(pbrosset)
Comment 26•9 years ago
|
||
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+
Assignee | ||
Comment 27•9 years ago
|
||
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.
Assignee | ||
Comment 28•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 29•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/de4547be239c
Keywords: checkin-needed
Comment 30•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/de4547be239c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Assignee | ||
Updated•9 years ago
|
Attachment #8466362 -
Attachment is obsolete: true
Assignee | ||
Comment 31•9 years ago
|
||
Screenshot without the patch
Assignee | ||
Comment 32•9 years ago
|
||
Screenshot with the patch
Updated•9 years ago
|
Whiteboard: [devedition-40][difficulty=medium] → [polish-backlog][difficulty=medium]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•