Bug 181169

Summary: Implement "line-break: anywhere"
Product: WebKit Reporter: c_erok
Component: TextAssignee: Javier Fernandez <jfernandez>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, jfernandez, mmaxfield, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 11   
Hardware: All   
OS: macOS 10.13   
See Also: https://bugs.chromium.org/p/chromium/issues/detail?id=720205
https://bugs.webkit.org/show_bug.cgi?id=197876
Bug Depends on:    
Bug Blocks: 83934    
Attachments:
Description Flags
Sample of the line break.
none
Patch
none
Patch
none
Patch none

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.