Detect adjoining text fragments with no line break opportunity between them #19688

Manishearth · 2018-01-04T13:43:25Z

First attempt at #874

This change is

highfive · 2018-01-04T13:43:28Z

Heads up! This PR modifies the following files:

@emilio: components/layout/fragment.rs, components/layout/inline.rs

highfive · 2018-01-04T13:43:30Z

Warning

These commits modify layout code, but no tests are modified. Please consider adding a test!

Manishearth · 2018-01-04T13:45:20Z

I'm very unfamiliar with the code here, and I figured that instead of trying to get a million questions answered over a 12 hour time difference, it would make more sense to try a simple attempt at a solution and be told what's wrong with it 😄

feedback? @mbrubeck

mbrubeck · 2018-01-04T16:47:13Z

Testing for whitespace alone will not suffice, since we also want to break on characters like hyphens, in cases like foo-bar. Instead we need to use the UAX 14 line breaking algorithm to determine whether a breaking opportunity exists.

Currently we run UAX 14 in TextRun::break_and_shape, when constructing each TextRun. The text run is split on line-break opportunities, and the segments are stored as a Vec<GlyphRun> inside the TextRun. This is done by the TextRunScanner during flow construction. Since it runs on one TextRun at a time, it can't determine whether a line break exists between TextRuns.

(A text run is used in shaping and must be a contiguous slice of text with the same shaping options, so in the general case we can't simply combine all the text of a "word" into a single TextRun. The overview has some additional high-level description.)

At a conceptual level, we need to run UAX 14 on the entire inline flow as a single string of text. Maybe we could do that by moving line-break scanning out of TextRun construction, to some other place that can process all the fragments together. An alternative would be to keep the current code for finding intra-run breaks, and do a separate check to determine whether to allow breaks between runs (similar to the approach in this PR). This would be less intrusive, but I don't know how feasible or efficient it could be.

xi-unicode has a LineBreakLeafIter type looks useful for finding line breaks without having a contiguous str for the entire flow. It still requires starting from the beginning of the flow, though; it doesn't offer any way to test for a break at an arbitrary point in the middle of a string/rope.

mbrubeck · 2018-01-04T18:16:10Z

I guess we wouldn't need to scan the entire flow for line breaks all at once, as long as we can store and re-use the same LineBreakLeafIter for all the text runs in a flow.

Manishearth · 2018-01-05T16:58:27Z

So it seems like the problem is not actually stuff between textruns? For the testcase at hand the entire thing is a single textrun. I can make it into two text runs if I do  (which still does not break in Firefox), and I can fix that case (for some reason LineBreakLeafIter doesn't seem to help distinguish between boundary breaks and I need to figure out why that's happening first)

mbrubeck · 2018-01-05T17:35:05Z

Ah. So, in the original test case there is a single TextRun, but it is split into multiple Fragments, each pointing to a separate range of the TextRun. We use TextRun::natural_word_slices_in_range to find breaking opportunities within the range for each fragment, but we don't check whether the start of a new fragment is at a breaking opportunity or not.

For a fragment that starts in the middle of a TextRun we can check whether it starts within a GlyphRun, or between GlyphRuns. For the first fragment in a TextRun, we hit the case that I mentioned previously, and we'll need to use LineBreakLeafIter during TextRun construction to record whether a break opportunity exists between this TextRun and the previous one.

Manishearth · 2018-01-10T12:36:14Z

For a fragment that starts in the middle of a TextRun we can check whether it starts within a GlyphRun, or between GlyphRuns.

I'm not clear on where we'd do this. During fragment construction? Or in scan_for_lines? It feels like the text run info isn't accessible from the latter, but for the former I feel like we'd have unnecessary effects.

mbrubeck · 2018-01-10T15:53:58Z

I think we'll want to add a "can break before" flag to ScannedTextFragmentInfo. We could determine this by doing a linear or binary search of the GlyphRuns in its TextRun, to find where the fragment boundary falls (or by looking at a similar flag stored on the TextRun, for the first fragment in a run), but it's probably more efficient to track this during text scanning and record it as each scanned text fragment is constructed. (There might still be cases where we need to search the GlyphRuns to initialize the flag when splitting a fragment...)

Manishearth · 2018-01-10T16:06:16Z

I think we'll want to add a "can break before" flag to ScannedTextFragmentInfo.

Right, that's the solution for the foobar case (which I've almost got working), but not the foobar case, right?

Maybe I've misunderstood your point.

Manishearth · 2018-01-10T16:15:46Z

Oh, wait, i see your point. Yeah, we can use the same flag for both.

Manishearth · 2018-01-10T16:16:42Z

Though, last I checked the creation of the fragment break in foobar was not in flush_clump_to_list, where I'd expect it to be. I'll recheck, but are there other places where we split such fragments?

Manishearth · 2018-01-12T07:31:27Z

Updated code. r? @mbrubeck

Going to try some edge cases with mixed scripts, including Arabic ZWJ-ing (e.g. الـ iPhone, though I need to bidi-override it to work correctly)

mbrubeck · 2018-01-12T18:10:30Z

@bors-servo try

bors-servo · 2018-01-12T18:10:35Z

⌛ Trying commit 3743be1 with merge 5dc0a96...


        Auto merge of #19688 - Manishearth:linebreak, r=<try>

Detect adjoining text fragments with no whitespace between them and avoid linebreaking there First attempt at #874  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19688)

mbrubeck · 2018-01-12T18:15:58Z

components/gfx/text/text_run.rs

+                finished = true;
+            }
+            if idx == 0 {
+                break_at_zero = false;


Should this be true?

bors-servo · 2018-01-12T18:43:55Z

💔 Test failed - linux-rel-css

Manishearth · 2018-01-13T04:19:59Z

@bors-servo retry try

bors-servo · 2018-01-13T04:20:04Z

⌛ Trying commit 1026c8a with merge ef5e71d...


        Auto merge of #19688 - Manishearth:linebreak, r=<try>

Detect adjoining text fragments with no whitespace between them and avoid linebreaking there First attempt at #874  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19688)

Manishearth · 2018-01-13T04:45:37Z

Hm, even after fixing that it doesn't seem to want to line break anymore


        Auto merge of #19688 - Manishearth:linebreak, r=mbrubeck

Detect adjoining text fragments with no line break opportunity between them First attempt at #874  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19688)

bors-servo · 2018-01-25T00:33:12Z

💔 Test failed - linux-rel-css

Manishearth · 2018-01-25T03:03:51Z

Nothing changed between this and the try, so it's an intermittent. Had a quick look and doesn't seem related, but might investigate further if it keeps cropping up.

@bors-servo retry

bors-servo · 2018-01-25T03:03:56Z

⌛ Testing commit fc97f0b with merge e53663e...


        Auto merge of #19688 - Manishearth:linebreak, r=mbrubeck

Detect adjoining text fragments with no line break opportunity between them First attempt at #874  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19688)

bors-servo · 2018-01-25T03:20:20Z

💔 Test failed - arm64

Manishearth · 2018-01-25T03:23:37Z

@bors-servo retry

infra, "unable to read response"

bors-servo · 2018-01-25T03:36:17Z

⚡ Previous build results for android, arm32, linux-dev, linux-rel-css, mac-dev-unit are reusable. Rebuilding only arm64, linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev...

bors-servo · 2018-01-25T03:52:10Z

💔 Test failed - arm64

Manishearth · 2018-01-25T04:04:12Z

arm64 is permafail on

error: failed to execute compile
caused by: error reading compile response from server
caused by: Failed to read response header
caused by: failed to fill whole buffer
error: Could not compile `script`.

for both this PR and another. Trying to ssh and fix

Manishearth · 2018-01-25T04:09:32Z

@bors-servo retry try-

(rehighstated the builder, cleaned up processes)

bors-servo · 2018-01-25T04:43:01Z

⌛ Testing commit fc97f0b with merge 9c13073...


        Auto merge of #19688 - Manishearth:linebreak, r=mbrubeck

Detect adjoining text fragments with no line break opportunity between them First attempt at #874  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19688)

bors-servo · 2018-01-25T05:57:35Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: mbrubeck
Pushing 9c13073 to master...

Manishearth · 2018-01-25T05:58:38Z

Yay! Will Mark the intermittent and file follow-ups when I get back

…

Manishearth · 2018-01-25T11:02:11Z

followups : #19862 , #19863

highfive assigned pcwalton Jan 4, 2018

highfive added the S-awaiting-review label Jan 4, 2018

pcwalton assigned mbrubeck and unassigned pcwalton Jan 5, 2018

Manishearth force-pushed the Manishearth:linebreak branch from 90a757e to 3743be1 Jan 12, 2018

mbrubeck requested changes Jan 12, 2018

View changes

highfive added the S-tests-failed label Jan 12, 2018

Manishearth force-pushed the Manishearth:linebreak branch from 3743be1 to 1026c8a Jan 13, 2018

highfive removed the S-tests-failed label Jan 13, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Jan 25, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Jan 25, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Jan 25, 2018

highfive added the S-tests-failed label Jan 25, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Jan 25, 2018

bors-servo merged commit fc97f0b into servo:master Jan 25, 2018
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jan 25, 2018

Manishearth deleted the Manishearth:linebreak branch Jan 25, 2018

This was referenced Jan 25, 2018

Intermittent PASS expected FAIL on /css/css-ui/text-overflow-023.html #19861

Open

WOFF webfonts broken on Linux #19862

Open

servo / servo

Detect adjoining text fragments with no line break opportunity between them #19688

Detect adjoining text fragments with no line break opportunity between them #19688

Manishearth commented Jan 4, 2018 •

edited

highfive commented Jan 4, 2018

highfive commented Jan 4, 2018

Manishearth commented Jan 4, 2018

mbrubeck commented Jan 4, 2018 •

edited

mbrubeck commented Jan 4, 2018

Manishearth commented Jan 5, 2018

mbrubeck commented Jan 5, 2018 •

edited

Manishearth commented Jan 10, 2018 •

edited

mbrubeck commented Jan 10, 2018

Manishearth commented Jan 10, 2018

Manishearth commented Jan 10, 2018

Manishearth commented Jan 10, 2018

Manishearth commented Jan 12, 2018 •

edited

mbrubeck commented Jan 12, 2018

bors-servo commented Jan 12, 2018

This comment has been minimized.

This comment has been minimized.

bors-servo commented Jan 12, 2018

Manishearth commented Jan 13, 2018

bors-servo commented Jan 13, 2018

Manishearth commented Jan 13, 2018

bors-servo commented Jan 25, 2018

Manishearth commented Jan 25, 2018

bors-servo commented Jan 25, 2018

bors-servo commented Jan 25, 2018

Manishearth commented Jan 25, 2018

bors-servo commented Jan 25, 2018

bors-servo commented Jan 25, 2018

Manishearth commented Jan 25, 2018

Manishearth commented Jan 25, 2018

bors-servo commented Jan 25, 2018

bors-servo commented Jan 25, 2018

Manishearth commented Jan 25, 2018

Manishearth commented Jan 25, 2018

servo / servo

Join GitHub today

Detect adjoining text fragments with no line break opportunity between them #19688

Detect adjoining text fragments with no line break opportunity between them #19688

Conversation

Manishearth commented Jan 4, 2018 • edited

highfive commented Jan 4, 2018

highfive commented Jan 4, 2018

Manishearth commented Jan 4, 2018

mbrubeck commented Jan 4, 2018 • edited

mbrubeck commented Jan 4, 2018

Manishearth commented Jan 5, 2018

mbrubeck commented Jan 5, 2018 • edited

Manishearth commented Jan 10, 2018 • edited

mbrubeck commented Jan 10, 2018

Manishearth commented Jan 10, 2018

Manishearth commented Jan 10, 2018

Manishearth commented Jan 10, 2018

Manishearth commented Jan 12, 2018 • edited

mbrubeck commented Jan 12, 2018

bors-servo commented Jan 12, 2018

This comment has been minimized.

mbrubeck Jan 12, 2018

This comment has been minimized.

Manishearth Jan 13, 2018

bors-servo commented Jan 12, 2018

Manishearth commented Jan 13, 2018

bors-servo commented Jan 13, 2018

Manishearth commented Jan 13, 2018

bors-servo commented Jan 25, 2018

Manishearth commented Jan 25, 2018

bors-servo commented Jan 25, 2018

bors-servo commented Jan 25, 2018

Manishearth commented Jan 25, 2018

bors-servo commented Jan 25, 2018

bors-servo commented Jan 25, 2018

Manishearth commented Jan 25, 2018

Manishearth commented Jan 25, 2018

bors-servo commented Jan 25, 2018

bors-servo commented Jan 25, 2018

Manishearth commented Jan 25, 2018

Manishearth commented Jan 25, 2018

Manishearth commented Jan 4, 2018 •

edited

mbrubeck commented Jan 4, 2018 •

edited

mbrubeck commented Jan 5, 2018 •

edited

Manishearth commented Jan 10, 2018 •

edited

Manishearth commented Jan 12, 2018 •

edited