Closed Bug 1568847 Opened 4 months ago Closed 2 months ago

Make whitespace text node in the markup view more explicit

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox71 fixed)

RESOLVED FIXED
Firefox 71
Tracking Status
firefox71 --- fixed

People

(Reporter: jdescottes, Assigned: thomas.jean.lynch)

References

(Depends on 1 open bug)

Details

(Keywords: good-first-bug)

User Story

Bug description: Improve the discoverability of one of our unique features: whitespace node badges! 

As seen on https://twitter.com/FirefoxDevTools/status/1153936100681015297

Changes needed:
- Change the badge text from • to whitespace
- Match styling of scroll badge
- Horizontally align badge with its peer nodes by removing padding from `.editor.text .whitespace` and removing margin from `.editor.text .whitespace::before`

Background from Julian:
>We help users visualize whitespace text nodes that impact the layout, but our UI for this is quite cryptic. This feature was added in Bug 1304685. Such nodes are currently displayed in the markup view as a badge-like element that just contains a circle. The circle was initially chosen because it was similar to how text-editors show whitespaces.


(Reading the other comments not necessary to fix this bug)

Attachments

(3 files)

We help users visualize whitespace text nodes that impact the layout, but our UI for this is quite cryptic. This feature was added in Bug 1304685. Such nodes are currently displayed in the markup view as a badge-like element that just contains a circle. The circle was initially chosen because it was similar to how text-editors show whitespaces.

See answers to https://twitter.com/FirefoxDevTools/status/1153936100681015297 , users seem to find the feature useful, but had no idea it was there.

The goal of this bug is to make the feature more discoverable.

A suggestion from Victoria was to use a badge that says [space].
This is similar to one of the designs we suggested initially, a badge saying [whitespace].

Attached image old-mockup.png

This is from Bug 1304685, hence the old inspector UI.

Regarding the tooltip: Currently, it shows the whitespace characters used, such as the return symbol for carriage returns and circles to indicate spaces. We can expand on that for more clarity by spelling it out like so (probably with some truncation after a certain point)

Whitespace-only text node: return, space, space, space, space, space ...

A more thorough solution would be to covert the title attr tooltip into an Inactive CSS-style tooltip, with a Learn More link to this MDN page. I did some quick editing today on this page, and could do more if needed. (E.g. Do we want the first section to focus more on the CSS/HTML layout bug implications?)

Badge

A badge of sorts that says "whitespace" could be useful. In practice in modern layouts with a lot of Flexbox I'm not seeing the "significant whitespace" nodes often (I had trouble finding examples to look at for this bug!), so when we do display it we can afford to make it bigger.

I hacked together a few options:

  • plain text or with a "badge" look (colors are not exact)
  • with the word "whitespace", or the more geeky \w, or a text representation of the actual content

The content representation could be the same we're using right now in the tooltip, or something a bit more code-like with \n for newlines, \t for tabs and either a blank space or a small bullet or middle-dot character for spaces; we could truncate it to the first 6-10 characters.

I'm not sure I like the badge look. One worry is that we're currently using clickable badges that are attached to the end of opening tags (<element attribute="foo"> [badge]), so users may read the badge look as meaning that the whitespace belongs to the previous node?

Tooltip

Spelling out the individual characters feels a bit strange. The current representation is alright, though it could be improved if we could style it (in a custom tooltip).

A more thorough solution would be to convert the title attr tooltip into an Inactive CSS-style tooltip, with a Learn More link to this MDN page

Love the idea.

Do we want the first section to focus more on the CSS/HTML layout bug implications?

Probably, this page feels strange when coming from the Inspector since it very quickly gives you a bunch of JS code that can be used for… er something, not sure what. Maybe that page needs an overhaul that is more HTML-and-CSS-friendly, and less "I'm writing DOM whitespace manipulation code".

The article from Patrick that is linked is actually very close to what would be useful in this use case. Maybe we should update it and publish it on MDN, and link to that?

Badge

Thanks for the mockups! There is indeed a bit of weirdness with using badges here — they haven't represented a specific bit of content before. I do love that it makes them more prominent in your bottom row examples.

I think I'd avoid displaying \w by itself because I suspect a lot of HTML/CSS-only devs won't be familiar with that. Representing the actual whitespace in the UI makes sense though, especially if we can do it more reliably! (Julian found one example in which that part shows in the tooltip as blank). Maybe something like this:

[whitespace: ••••\t\r]

Tooltip

If we're showing the whitespace representation from the tooltip right in the badge, then the tooltip could remain a simple title attr. It could even just say Learn More, with the badge potentially linking to the MDN page (though we'd want some kind of info icon if it's going to directly link anywhere).

MDN page

Yeah, the page used to not have anything on layout - I had just added those parts. I'll pull in the MDN folks on whether we could revamp the page or create a new page.

Hi Chris! We're wondering about this page, Whitespace in the DOM.

Before yesterday, the page was mainly based on the use case of writing DOM manipulation code. However, we recently got a lot of Twitter love for our whitespace node marker in DevTools, which is much more about HTML/CSS layout issues due to unexpected effects of whitespace. Thus, I started to add a bit of info on this to the page, including a link to this article by Patrick.

Because we want to start linking to it from inside DevTools as part of our new whitespace node design, we feel it would be best to have an MDN page that is more focused on the layout issues up front, with more of the info from Patrick. We also think the HTML/CSS use case is more common. Would it make sense to update the existing page for this, or create a new page about 'Whitespace in HTML/CSS Layouts'?

Flags: needinfo?(cmills)

Hi Victoria,

I think improving this page is a really good idea — it is already useful content, but I do agree that it should have some more practical stuff in there. Patrick's article is interesting and useful; the issue discussed at https://patrickbrosset.com/articles/2016-10-21-when-does-white-space-matter-in-HTML.html#ecde comes up all the time when you are trying to create a horizontal nav or rows of a photo gallery, for example, and it does really confuse beginners. Although it is less of a problem when you use modern responsive layout techiques like grid or flexbox.

I'd advise to keep it all in one article rather than do two separare ones, as it is all closely-related.

The only question then is where we put that article. It more logically feels like a CSS/layout thing than an API thing, so we could consider moving it under the CSS section of MDN, and perhaps renaming it slightly to something like "Solving layout problems due to excess whitespace"?

Flags: needinfo?(cmills)

Thanks for the info!

As far as whether this article should be in the CSS Layout category - I'm needinfoing Patrick for his opinion.

As far as refreshing the article to have more practical details about web layouts, such as information from Patrick's article, I wanted to check if anyone on your team is available to work on this :). I'd love to take this on, but I'm overloaded at the moment.

Flags: needinfo?(pbrosset)
Flags: needinfo?(cmills)

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #7)

The only question then is where we put that article. It more logically feels like a CSS/layout thing than an API thing, so we could consider moving it under the CSS section of MDN, and perhaps renaming it slightly to something like "Solving layout problems due to excess whitespace"?

I would definitely move this under CSS on MDN. This is all really about how CSS handles whitespace on the web, so it makes sense to move it to that category.

Flags: needinfo?(pbrosset)

As far as refreshing the article to have more practical details about web layouts, such as information from Patrick's article, I wanted to check if anyone on your team is available to work on this :). I'd love to take this on, but I'm overloaded at the moment.

OK. Can you file an issue at https://github.com/mdn/sprints/issues, explaining what should be done? Then we'll prioritise it as part of our sprint process.

Flags: needinfo?(cmills)

Should we define an initial goal for this bug as just changing the label of the icon to "whitespace"? Could be a good first bug :). We can experiment with different representations of whitespace later.

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #10)

OK. Can you file an issue at https://github.com/mdn/sprints/issues, explaining what should be done? Then we'll prioritise it as part of our sprint process.

Thanks, will do!

Priority: -- → P3

Clarifying next steps for this bug :)

  • Change badge text from to whitespace
  • Match styling of scroll badge
  • Horizontally align badge with its peer nodes by removing padding from .editor.text .whitespace and removing margin from .editor.text .whitespace::before
User Story: (updated)
User Story: (updated)
User Story: (updated)
User Story: (updated)
User Story: (updated)

Hey all, I'd like to grab this one (seems like a good bug to get familiar with the contribution workflow). I'm following the documentation at https://docs.firefox-dev.tools/ (just got a local Firefox build running); is there anything I should be looking at?

Hi Jean, thanks for wanting to work on this. I'll assign the bug to you now.
That website is the right one, great to hear you have a working local build! You're all set to work on this then.
The list of things to do is in comment 12. And the files you will be wanting to modify are:

I highly suggest using the Browser Toolbox to help you debug/inspect. This tool is like devtools for the entire Firefox. So you can inspect devtools with devtools, and therefore check if your style changes work correctly, and fine tune them without having to re-build.

Also, because you will only be changing static files, no need for a full build whenever you make a change. Once you've made the change, you can run ./mach build faster and then ./mach run.

Assignee: nobody → thomas.jean.lynch
Status: NEW → ASSIGNED

Change the displayed text of whitespace nodes in the inspector to "whitespace, and add a corresponding value to the inspector localized file. Also restyled the badge to match the scroll badge.

Awesome, thanks for the patch Jean! I see it was reviewed by Julian and Flod already. Are you ok taking it from there, addressing the comments and posting a new version? If you need any help, please do ask.
I'm looking forward to seeing this land!

Flags: needinfo?(thomas.jean.lynch)

(Jean updated the patch and I will now land it, so clearing the ni? here)

Flags: needinfo?(thomas.jean.lynch)
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6d44c6f765cb
Change whitespace node's text to "whitespace" and restyle the node. r=jdescottes,flod
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71

Thanks all for the great work!

I think this patch is still missing one part - fixing the alignment of the badge so that it's flush with other tags.

  • Horizontally align badge with its peer nodes by removing padding from .editor.text .whitespace and removing margin from .editor.text .whitespace::before

Should I reopen this bug or create a new one?

Flags: needinfo?(jdescottes)

(In reply to Victoria Wang [:victoria] from comment #20)

Thanks all for the great work!

I think this patch is still missing one part - fixing the alignment of the badge so that it's flush with other tags.

  • Horizontally align badge with its peer nodes by removing padding from .editor.text .whitespace and removing margin from .editor.text .whitespace::before

Should I reopen this bug or create a new one?

Please open another bug, blocking this one. Reopening bugs or using leave-open makes things very hard to track.

Flags: needinfo?(jdescottes)
Depends on: 1588922
You need to log in before you can comment on or make changes to this bug.