Bug 109624 - Bidi-Isolate inlines break layout with collapsed whitespace
Summary: Bidi-Isolate inlines break layout with collapsed whitespace
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
: 124146 (view as bug list)
Depends on: 108137
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-12 15:32 PST by Levi Weintraub
Modified: 2016-02-22 14:56 PST (History)
15 users (show)

See Also:


Attachments
Potential ref test that shows the problem (1.22 KB, text/html)
2013-02-13 13:33 PST, Levi Weintraub
no flags Details
Port of Blink patch (19.38 KB, patch)
2015-07-09 15:44 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
No tests yet (19.85 KB, patch)
2015-07-09 18:38 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (23.64 KB, patch)
2015-07-09 20:32 PDT, Myles C. Maxfield
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Levi Weintraub 2013-02-12 15:32:20 PST
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.
Comment 1 Levi Weintraub 2013-02-13 13:33:06 PST
Created attachment 188169 [details]
Potential ref test that shows the problem
Comment 2 Eric Seidel (no email) 2013-02-13 13:34:47 PST
This is awesome btw.  Thank you for reducing this Levi.
Comment 3 Robert Hogan 2013-05-20 11:35:35 PDT
(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. :)
Comment 4 Levi Weintraub 2013-05-20 14:27:45 PDT
(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.
Comment 5 Myles C. Maxfield 2015-07-09 13:33:30 PDT
*** Bug 124146 has been marked as a duplicate of this bug. ***
Comment 6 Radar WebKit Bug Importer 2015-07-09 13:37:42 PDT
<rdar://problem/21752834>
Comment 7 Myles C. Maxfield 2015-07-09 14:20:29 PDT
https://codereview.chromium.org/26315003 is the relevant Blink patch which fixes the issue
Comment 8 Myles C. Maxfield 2015-07-09 15:44:40 PDT
Created attachment 256533 [details]
Port of Blink patch
Comment 9 Myles C. Maxfield 2015-07-09 15:55:07 PDT
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.
Comment 10 Myles C. Maxfield 2015-07-09 18:38:43 PDT
Created attachment 256554 [details]
No tests yet
Comment 11 Myles C. Maxfield 2015-07-09 20:32:43 PDT
Created attachment 256562 [details]
Patch
Comment 12 Myles C. Maxfield 2015-07-10 09:35:21 PDT
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
Comment 13 Dave Hyatt 2015-07-10 09:35:23 PDT
Comment on attachment 256562 [details]
Patch

r=me
Comment 14 Myles C. Maxfield 2015-07-10 09:36:36 PDT
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.
Comment 15 Myles C. Maxfield 2015-07-10 13:03:58 PDT
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.
Comment 16 Myles C. Maxfield 2015-07-10 13:40:23 PDT
Committed r186686: <http://trac.webkit.org/changeset/186686>
Comment 17 Derk-Jan Hartman 2016-02-22 14:56:20 PST
*** Bug 124146 has been marked as a duplicate of this bug. ***