Bug 108137 – Bidi-Isolated inlines can cause subsequent content to not be rendered

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

[Levi Weintraub]

Created attachment 185112 [details]
Test case

Text following an isolated span fails to be rendered. For easy debugging, RenderBlock::appendRunsForObject returns early for the run following the Isolate.

The test case should read 123 456 789

[Eric Seidel (no email)]

Yes, that is definitely wrong. I wonder if this is a recent regression?

[Levi Weintraub]

(In reply to comment #1)
> Yes, that is definitely wrong. I wonder if this is a recent regression?

Dunno. You can probably get a bisection if you ask on the Chromium bug. It may be awhile before I get a chance to take a look if you want to in the meantime, otherwise it's on my todo list.

[Eric Seidel (no email)]

Yeah, the question is just if builds right after we added isolate were broken.  Which is from like summer/fall of '11.

[Levi Weintraub]

It actually appears to be a combination issue of isolates and midpoints, as <div>123<span style="unicode-bidi: -webkit-isolate">456</span>789</div> doesn't reproduce the problem. You need a newline and leading space following 123.

Repros:
<div>123
 <span style="unicode-bidi: -webkit-isolate">456</span>789</div>

[Eric Seidel (no email)]

Oh.  Then I probably just broke this when writing isolate and it wasn't sufficiently tested.  PRobably an easy fix, if we were to stare at the code for a bit.  We can do that together when I'm back in the office if you like.

[Levi Weintraub]

(In reply to comment #5)
> Oh.  Then I probably just broke this when writing isolate and it wasn't sufficiently tested.  PRobably an easy fix, if we were to stare at the code for a bit.  We can do that together when I'm back in the office if you like.

Yeah, I'd be surprised if this is a regression given that fact. I'd be happy to take a look with you.

[Levi Weintraub]

(In reply to comment #6)
> Yeah, I'd be surprised if this is a regression given that fact. I'd be happy to take a look with you.

Or I may have figured it out ;)

If we're in the middle of a midpoint that terminates in an isolate, we failed to clear it when adding the fake run.

[Levi Weintraub]

(In reply to comment #7)
> If we're in the middle of a midpoint that terminates in an isolate, we failed to clear it when adding the fake run.

Sorry, "in the middle of a midpoint" doesn't make sense. If we're *between* midpoints and the next ends in the isolate.

[Levi Weintraub]

So the fix to get us to not improperly truncate content following an isolate is easy and I've got a patch that does so. Sadly, though, the current implementation makes it very difficult to properly coalesce spacing around an isolate the way we do with non-isolated content due to the two-pass approach we take. I believe to fix that, we'd have to actually parse the Isolated content to properly set up the MidpointState. I'll talk with eseidel when he's in the office.

[Levi Weintraub]

Created attachment 186193 [details]
Incorrect collapsed whitespace inside Isolates

More bad news. Inline Isolates do *not* properly handle ignored whitespace, instead it's all rendered, but of course not considered in Layout. You can see this in the attached test case. "4 5 6" should have only one space between characters and be contained in the green background.

With more testing, I believe my quick fix to be an acceptable first-run solution. I'll upload it next week. The right fix for all these issues will take more time :-/

[Levi Weintraub]

Changing the title of this bug to track the issue I plan on immediately addressing and filing a new bug to track the remaining issue.

[Levi Weintraub]

Created attachment 187956 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 187956 [details]
Patch

Is it possible to do this as a dumpAsText test and still get what you want?

[Levi Weintraub]

(In reply to comment #13)
> (From update of attachment 187956 [details])
> Is it possible to do this as a dumpAsText test and still get what you want?

Updating the patch to use a test utilizing dumpAsText. The original test is more comprehensive, but is meant to be a ref test, but will only pass once we fix bug 109624.

[Levi Weintraub]

Created attachment 188127 [details]
Patch

[WebKit Review Bot]

Comment on attachment 188127 [details]
Patch

Clearing flags on attachment: 188127

Committed r142793: <http://trac.webkit.org/changeset/142793>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.