Skip to content

text wrap (lineWidth) #699

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 24, 2022
Merged

text wrap (lineWidth) #699

merged 3 commits into from
Jan 24, 2022

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Jan 22, 2022

Implements text wrapping using a lineWidth option on the text mark. Fixes #688. Tasks:

  • Don’t wrap by default (when lineWidth is Infinity).
  • Allow lineWidth to be specified in ems.
  • Allow lineWidth to be specified in pixels? (But how?) No.
  • Preserve newlines in input.
  • Optionally ignore newlines in input? No.
  • Optionally coalesce multiple spaces in input? No.
  • Support monospace text layout (a monospace option, say)?
  • Support tabular numerals? No.
  • Measure text metrics dynamically with Canvas, if fast enough? No.
  • Allow custom implementations for text metrics? No.
  • Implement Unicode line breaking algorithm. No, too hard.
  • Implement Unicode grapheme cluster breaking algorithm. No, too hard.
  • Implement Knuth–Plass line breaking algorithm. No, too hard.
  • Documentation
  • Tests

Even though this is very rudimentary, I think it will be super useful and can be improved in the future based on usage.

I think the biggest blocker is just whether it’s okay to have lineWidth in ems. That’s the easiest thing for us to implement based on the baked-in rudimentary text metrics, but I think people are likely to get confused because they are more likely to be thinking in pixels. The problem with pixels is that the fontSize isn’t necessarily in pixels, either #378 and so we’d almost certainly need to invoke the browser’s full text metrics in order to compute it. And that also means an element will need to be attached to the DOM, and it’s slow, etc. So, I think maybe lineWidth in ems is the best option under the “worse is better” philosophy. We could also expose a hook for computing text metrics yourself, but that feels super low-level, and also not necessary since you could just specify text with newlines if you want to do the wrapping yourself.

@mbostock mbostock requested a review from Fil January 22, 2022 20:00
@Fil
Copy link
Contributor

Fil commented Jan 24, 2022

  • lineWidth in ems make sense—maybe we could name the option lineChars to make that clearer? The monospace option is also very welcome.

  • Optionally coalesce multiple spaces in input: yes this is useful if we're using the date formatter that produces a space for the day's numbers: i've sometimes had to use d3.utcFormat("%b %e")(date).replace(/ +/, " ") and it's ugly.

  • Optionally ignore newlines in input? Doesn't seem necessary, it's easy enough to replace(\n, " ").

  • Would there be a way to support non-breaking spaces, which can be quite important when the text contains numbers (e.g. "1,000~units" should be preserved, if possible)?

@mbostock
Copy link
Member Author

maybe we could name the option lineChars to make that clearer

I think this requires a little prognostication. Here are some possible futures:

  1. We support lineWidth in px, em, and perhaps other CSS units; px is preferred.
  2. We support lineWidth in px, em, and perhaps other CSS units; em is preferred.
  3. We don’t support px; em is implied.

In future 1, we’d want lineWidth: "40em" for ems and lineWidth: 200 or lineWidth: "200px" for pixels.

In future 2, we’d want lineWidth: 40 or lineWidth: "40em" for ems and lineWidth: "200px" for pixels.

In future 3, we’d want lineWidth: 40 for ems… or possibly some name other than “lineWidth”, but “lineWidth” is symmetric with “lineHeight” in the sense that both are multiplied by the font size. And I would avoid “chars” because it implies counting characters, which is different from measuring ems (unless you’re using a monospaced font).

So if we think there’s a reasonably high probability that we want lineWidth: 200 to mean 200 pixels in the future (future 1), then it’s probably worth doing lineWidth: "40em" for now and throwing an error on other units. On the other hand if we’re not sure that we’ll implement lineWidth in pixels in the nearish future (future 3), or if we think that ems will be the preferred unit for specifying the length of lines (future 2), then we should leave the current behavior as-is.

I think em is probably preferred to px, given that Plot is mostly about positioning in abstract coordinates anyway (scaled data space). I vote we do nothing.

The monospace option is also very welcome

Yes. I think for due diligence it’s probably also worth benchmarking what the performance would be like if we measured the text with canvas rather than using a hard-coded character width map. That would also make it possible to support lineWidth in px, though as I implied above I don’t think that’s a priority.

Optionally coalesce multiple spaces in input: yes this is useful if we're using the date formatter that produces a space for the day's numbers: i've sometimes had to use d3.utcFormat("%b %e")(date).replace(/ +/, " ") and it's ugly.

You can use d3.utcFormat("%b %-e"). 🙂

Optionally ignore newlines in input? Doesn't seem necessary, it's easy enough to replace(\n, " ").

This seems conflicting with your previous input… I think I would only do these together or not at all, similar to CSS white-space, i.e., whiteSpace: "normal" to coalesce spaces and ignore newlines, whiteSpace: "pre-line" to coalesce spaces and preserve newlines, and whiteSpace: "pre-wrap" to preserve spaces and newlines (the current behavior). This would also allow whiteSpace: "pre" which would ignore the lineWidth option. But in any case, I also don’t think it’s hard to replace spaces in the input, and so don’t consider this pressing and feel we shouldn’t implement it now.

Would there be a way to support non-breaking spaces, which can be quite important when the text contains numbers (e.g. "1,000~units" should be preserved, if possible)?

Yes, this is already supported. You can type option-space for a non-breaking space, or use \xa0.

Copy link
Contributor

@Fil Fil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL about %-e

Thanks for the detailed explanations—it's really neat!

@mbostock
Copy link
Member Author

I did some exploration using context.measureText, which would allow several things:

  • We could specify lineWidth in pixels (and possibly any other CSS unit).
  • We could support any fontSize, fontFamily, and fontVariant accurately.

However, it was also about 10× slower than the current approach, and added some complexity (such as needing to create an invisible canvas element and attaching it to the DOM; you can’t set font-variant-numeric via the context.font attribute). So, I’m inclined to just keep it simple to start and revisit this in the future according to demand.

@Fil
Copy link
Contributor

Fil commented Jan 24, 2022

yes. I was thinking that in node we don't have canvas as a dependency (only as devDependency)

@mbostock
Copy link
Member Author

I fixed the definition of em. In CSS, 1em is not the width of a capital M, but instead an alias for the current fontSize. I had computed the default width map using 100px ui-system, but the layout is different at 10px ui-system (presumably because of hinting to align with pixel boundaries)? So, I recomputed the default width map at 10px ui-system, and then multiplied by 10 to avoid to produce integers. This will make the default metrics less accurate at larger font sizes, but I assume that most of the time text will be rendered with the default font size, and so we should prioritize that.

I’ve also implemented the monospace option. I’ll add a test or two and then merge.

@mbostock mbostock merged commit 6c29fd9 into main Jan 24, 2022
@mbostock mbostock deleted the mbostock/lineWidth branch January 24, 2022 23:26
@yurivish
Copy link
Contributor

yurivish commented Jun 3, 2022

Would the defaultWidth function implemented here also potentially work as a heuristic for providing default margin sizes based on tick label lengths (#383)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatic text wrapping
3 participants