Bug 109624 – Bidi-Isolate inlines break layout with collapsed whitespace

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Levi Weintraub]

Because we don't update the midpointState while iterating across inline isolated content in the first-pass of layout, we don't properly handle collapsable whitespace inside and following the isolate.

[Levi Weintraub]

Created attachment 188169 [details]
Potential ref test that shows the problem

[Eric Seidel (no email)]

This is awesome btw.  Thank you for reducing this Levi.

[Robert Hogan]

(In reply to comment #1)
> Created an attachment (id=188169) [details]
> Potential ref test that shows the problem

So do we just need to introduce the same recursion in addFakeRunsIfNecessary() that we have in appendRunsForObject() for stopping and starting at midpoints?

Or are you saying that we don't even store the beginning and end of ignored spaces in lineMidpointState for isolates? 

This is RenderBlockLineLayout after all, it's hard to tell. :)

[Levi Weintraub]

(In reply to comment #3)
> (In reply to comment #1)
> > Created an attachment (id=188169) [details] [details]
> > Potential ref test that shows the problem
> 
> So do we just need to introduce the same recursion in addFakeRunsIfNecessary() that we have in appendRunsForObject() for stopping and starting at midpoints?
> 
> Or are you saying that we don't even store the beginning and end of ignored spaces in lineMidpointState for isolates? 
> 
> This is RenderBlockLineLayout after all, it's hard to tell. :)

Even if we store the beginning/end of ignored spaces for isolates, we'd get it wrong because we don't go through the normal path for the contents of the isolates in the first pass so the values would be wrong. We need to store the midpoint state at the start of the isolate, and then iterate across the contents to get the midpoint state at the end correct.

[Myles C. Maxfield]

*** Bug 124146 has been marked as a duplicate of this bug. ***

[Radar WebKit Bug Importer]

<rdar://problem/21752834>

[Myles C. Maxfield]

https://codereview.chromium.org/26315003 is the relevant Blink patch which fixes the issue

[Myles C. Maxfield]

Created attachment 256533 [details]
Port of Blink patch

[Myles C. Maxfield]

The Blink patch's approach has major problems. I strongly suggest that we do not land it. It solves the problem by keeping a HashMap of MidpointStates, each of which contains a vector of Iterators. However, the vector of iterators is the same for each of the MidpointStates in the HashMap.

(Also: I have other comments, like items are never removed from the HashMap, only added (though the lifetime of the HashMap is just the lifetime of laying out a single line). Also, I dislike that clearing the isolated runs moved into restoreIsolatedMidpointStates().)

It's probably a better idea to simply rerun through the same MidpointState vector each time constructBidiRunsForSegment() starts on a new isolate. RenderBlockFlow::appendRunsForObject() can just skip over the midpoints which are not relevant to the RenderObject it's passed. That is, assuming the added work of skipping midpoints doesn't incur a measurable perf hit.

[Myles C. Maxfield]

Created attachment 256554 [details]
No tests yet

[Myles C. Maxfield]

Created attachment 256562 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 256562 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockFlow.h:-540
> -    static void appendRunsForObject(BidiRunList<BidiRun>&, int start, int end, RenderObject*, InlineBidiResolver&);

Re-ordering these arguments is unnecessary

[Dave Hyatt]

Comment on attachment 256562 [details]
Patch

r=me

[Myles C. Maxfield]

Comment on attachment 256562 [details]
Patch

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

> Source/WebCore/ChangeLog:42
> +        Test: fast/text/bidi-isolate-whitespace-collapse.html

I should make the test more robust.

[Myles C. Maxfield]

Comment on attachment 256562 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1026
> +    for (unsigned i = 0; i < midpoints.numMidpoints(); ++i) {

Hrm. This search doesn't work if the entire isolate is inside a collapsed region.

The only way a search could work in this case is if we iterate through the entire line. However, it seems silly to do that because we just got finished iterating through the entire line to perform BiDi on it.

Seems like I've got to go the blink route and keep some state around from the first iteration. However, I can at least make this a map of integers rather than a map of vectors.

[Myles C. Maxfield]

Committed r186686: <http://trac.webkit.org/changeset/186686>

[Derk-Jan Hartman]

*** Bug 124146 has been marked as a duplicate of this bug. ***