Bug 181169 - Implement "line-break: anywhere"
Summary: Implement "line-break: anywhere"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: Safari 11
Hardware: All macOS 10.13
: P2 Normal
Assignee: Javier Fernandez
URL:
Keywords: InRadar
Depends on:
Blocks: 83934
  Show dependency treegraph
 
Reported: 2017-12-26 19:39 PST by c_erok
Modified: 2019-05-14 06:21 PDT (History)
4 users (show)

See Also:


Attachments
Sample of the line break. (68.77 KB, image/png)
2017-12-26 19:39 PST, c_erok
no flags Details
Patch (73.61 KB, patch)
2019-04-30 14:07 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (72.62 KB, patch)
2019-04-30 15:34 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (72.87 KB, patch)
2019-05-14 03:21 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description c_erok 2017-12-26 19:39:52 PST
Created attachment 330206 [details]
Sample of the line break.

In the situation of a left quotation appearing in the last of a line, the left quotation will wrap to a new line with one character before it. But in Chinese text, a left quotation appearing in the front of a line is allowed. Same as above, a right quotation should be able to appear in the last of a line.
Comment 1 Myles C. Maxfield 2017-12-26 21:42:54 PST
This behavior is compliant with the spec and interoperable (to some degree) with all the major browsers on the Mac (I haven't tested Edge to know if it behaves the same way, but I would guess that it would). In addition, this is consistent with the system editing behavior for native apps (like TextEdit).

You probably want to style your content with "line-break: anywhere" which isn't implemented in WebKit yet. As soon as we implement that (or if it gets implemented), adding that style to your content will cause it to have the behavior you desire.

Until then, you can work around this bug by adding "word-break: break-all" which currently, erroneously, has the behavior you desire. However, we want to fix "line-break: break-all" to have your undesirable behavior (so that it is spec-compliant).

Because "line-break: anywhere" is at risk[1] in the CSS Text spec, it is possible that the spec may change in the future, so it probably wouldn't be a good idea to blindly add it to your content right now.

[1] https://drafts.csswg.org/css-text-3/
Comment 2 Javier Fernandez 2019-03-01 04:15:04 PST
I started to work on this feature for Chrome; if nobody does it before, I may eventually implement it for WebKit, so I'd appreciate feedback on the interest of webkit on the feature.
Comment 3 Radar WebKit Bug Importer 2019-03-01 04:17:31 PST
<rdar://problem/48507088>
Comment 4 Myles C. Maxfield 2019-03-01 13:34:00 PST
(In reply to Javier Fernandez from comment #2)
> I started to work on this feature for Chrome; if nobody does it before, I
> may eventually implement it for WebKit, so I'd appreciate feedback on the
> interest of webkit on the feature.

We are definitely interested! :)
Comment 5 Javier Fernandez 2019-04-30 14:07:17 PDT
Created attachment 368610 [details]
Patch
Comment 6 Javier Fernandez 2019-04-30 15:34:07 PDT
Created attachment 368620 [details]
Patch

Patch rebased
Comment 7 Myles C. Maxfield 2019-05-06 17:49:16 PDT
Comment on attachment 368620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368620&action=review

> Source/WebCore/rendering/BreakLines.h:194
> +    NonSharedCharacterBreakIterator iterator(stringView);

Is this really the correct type of iterator here?

There's likely a significant performance opportunity here, we should probably file a new bug about using a shared iterator and add a FIXME here.

> Source/WebCore/rendering/BreakLines.h:206
> +    if (breakAnywhere) {
> +        nextBreakable = nextBreakablePositionBreakCharacter(lazyBreakIterator, startPosition);
> +    } else if (keepAllWords) {

Didn't the style checker catch the unnecessary { } ?

Do we need to have two variants for the NBSP behavior?
Comment 8 Javier Fernandez 2019-05-07 03:22:32 PDT
Comment on attachment 368620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368620&action=review

>> Source/WebCore/rendering/BreakLines.h:194
>> +    NonSharedCharacterBreakIterator iterator(stringView);
> 
> Is this really the correct type of iterator here?
> 
> There's likely a significant performance opportunity here, we should probably file a new bug about using a shared iterator and add a FIXME here.

This is the approach Chrome is following now, although it implemented some optimizations to avoid iterating through the whole text line's length every time the function is called. 

I could definitively evaluating the Shared iterator approach, but I think I'd need some guidance.

>> Source/WebCore/rendering/BreakLines.h:206
>> +    } else if (keepAllWords) {
> 
> Didn't the style checker catch the unnecessary { } ?
> 
> Do we need to have two variants for the NBSP behavior?

Ups, I'm not sure, but I'll fix it before landing.

breakAnywhere should be include a wider range of breaking opportunities than just NBSP; do you mean that I should refactor the breakNBSP case and this one ?
Comment 9 Myles C. Maxfield 2019-05-08 13:06:32 PDT
> breakAnywhere should be include a wider range of breaking opportunities than
> just NBSP; do you mean that I should refactor the breakNBSP case and this
> one ?

I just mean the code directly below has a bunch of "if (breakNBSP)" blocks. Do we need the same thing here?
Comment 10 Javier Fernandez 2019-05-14 02:38:07 PDT
(In reply to Myles C. Maxfield from comment #9)
> > breakAnywhere should be include a wider range of breaking opportunities than
> > just NBSP; do you mean that I should refactor the breakNBSP case and this
> > one ?
> 
> I just mean the code directly below has a bunch of "if (breakNBSP)" blocks.
> Do we need the same thing here?

My understanding of current code and what the breakNBSP condition would imply is that breakAnywhere assume always that condition as true. 

The line-break:anywhere feature indeed allows to break "non-breakable spaces"; but as I commented before, it allows breaking on other usually unbreakable characters as well. Hence, I decided to implement this logic in a new function, which doesen't depend on this breakNBSP condition.
Comment 11 Javier Fernandez 2019-05-14 03:21:30 PDT
Created attachment 369832 [details]
Patch

Patch rebased
Comment 12 WebKit Commit Bot 2019-05-14 06:21:00 PDT
Comment on attachment 369832 [details]
Patch

Clearing flags on attachment: 369832

Committed r245275: <https://trac.webkit.org/changeset/245275>
Comment 13 WebKit Commit Bot 2019-05-14 06:21:01 PDT
All reviewed patches have been landed.  Closing bug.