Merge ~lgp171188/launchpad:diff-viewer-fix-text-overflow into launchpad:master

[Guruprasad]

Wrap the line when the diff text is too long

[Guruprasad (lgp171188)]

Fixes the text overflow issue in the diff viewer seen in https://code.launchpad.net/~lgp171188/launchpad/+git/launchpad/+merge/423964.

[Andrey Fedoseev (andrey-fedoseev)]

Consider this instead:

```
table.diff {
  ...
  overflow-wrap: break-word;
}
```

[Guruprasad (lgp171188)]

Andrey,

> ```
> table.diff {
> ...
> overflow-wrap: break-word;
> }
> ```

I am not a CSS expert. So can you tell me why and how this is better than having a horizontal scrollbar?

[Andrey Fedoseev (andrey-fedoseev)]

We already have text wrapping there, which causes the long lines to wrap without using horizontal scrollbars.

Be default, it breaks the lines at word delimiters (whitespace, dash, etc.). By having `overflow-wrap: break-word;` we allow wrapping by breaking longs words, which is the case in your example.

You can find some information on disadvantages of the horizontal scrollbars here: https://www.nngroup.com/articles/horizontal-scrolling/

In general, they offer bad user experience.

[Guruprasad (lgp171188)]

Andrey, `overflow: break-word` is a good choice for inserting line breaks between words in long sentences. However in the referenced MP, there are no words and `overflow: break-word` breaks on the hyphens in the values of the some svg attribute. I am not sure if that is such a good idea.

Colin, comments/thoughts?

[Colin Watson (cjwatson)]

I tried these out interactively in Firefox. I'm not sure I completely like either of these, but on balance I prefer Andrey's suggestion. I agree that it's a _bit_ weird to break on hyphens in raw SVG files, but it's not like it's really human-readable anyway. We have two basic cases here:

 * long lines in files that aren't reviewable as text (in this case it doesn't matter very much, and we might as well make it just not be terribly ugly)
 * long lines in files that are reviewable as text (in this case, `overflow-wrap: break-word` is likely to be reasonable)

The `table-layout: auto` change looks worse to my eye: I can see why you would have needed it with `overflow: scroll`, but it makes the line-number column uncomfortably narrow, and it's definitely worse overall with `overflow-wrap: break-word`.

[Guruprasad (lgp171188)]

Andrey, Colin, I have updated this MP to just specify `overflow-wrap: break-word`.

> I'm not sure I completely like either of these, but on balance I prefer Andrey's suggestion.
> ..
> The `table-layout: auto` change looks worse to my eye: I can see why you would have needed it with `overflow: scroll`, but it makes the line-number column uncomfortably narrow, and it's definitely worse overall with `overflow-wrap: break-word`.

I understood the `it's definitely worse overall with 'overflow-wrap: break-word'` as `it's definitely worse overall vs 'overflow-wrap: break-word'` based on your comment above that you prefer Andrey's suggestion.

[Colin Watson (cjwatson)]

I meant something like "`table-layout: auto` has some benefits and some drawbacks in conjunction with `overflow: scroll`; `table-layout: auto` is definitely worse in combination with `overflow-wrap: break-word`".

I'm fine with this change now.

review: Approve

diff --git a/lib/canonical/launchpad/icing/style.css b/lib/canonical/launchpad/icing/style.css
index 92f2d6f..1db5bd9 100644
--- a/lib/canonical/launchpad/icing/style.css
+++ b/lib/canonical/launchpad/icing/style.css
@@ -634,6 +634,7 @@ table.diff {
    width: 100%;
    background-color: white;
    margin-bottom: 0.5em;
+   overflow-wrap: break-word;
 }
 table.unidiff {
    table-layout: fixed;