|Summary:||Bidi-Isolated inlines can cause subsequent content to not be rendered|
|Product:||WebKit||Reporter:||Levi Weintraub <leviw>|
|Component:||Layout and Rendering||Assignee:||Levi Weintraub <leviw>|
|Severity:||Normal||CC:||eric, gyuyoung.kim, hyatt, leviw, mitz, ojan.autocc, playmobil, rakuco, rniwa, webkit.review.bot|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
Description Levi Weintraub 2013-01-28 17:08:10 PST
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
Comment 1 Eric Seidel (no email) 2013-01-28 17:12:16 PST
Yes, that is definitely wrong. I wonder if this is a recent regression?
Comment 2 Levi Weintraub 2013-01-29 01:52:00 PST
(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.
Comment 3 Eric Seidel (no email) 2013-01-29 14:50:37 PST
Yeah, the question is just if builds right after we added isolate were broken. Which is from like summer/fall of '11.
Comment 4 Levi Weintraub 2013-01-29 14:53:23 PST
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>
Comment 5 Eric Seidel (no email) 2013-01-29 14:57:10 PST
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.
Comment 6 Levi Weintraub 2013-01-29 14:59:01 PST
(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.
Comment 7 Levi Weintraub 2013-01-29 15:29:13 PST
(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.
Comment 8 Levi Weintraub 2013-01-29 15:29:58 PST
(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.
Comment 9 Levi Weintraub 2013-01-31 15:36:06 PST
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.
Comment 10 Levi Weintraub 2013-02-01 17:28:09 PST
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 :-/
Comment 11 Levi Weintraub 2013-02-12 15:29:26 PST
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.
Comment 13 Eric Seidel (no email) 2013-02-12 16:55:37 PST
Comment on attachment 187956 [details] Patch Is it possible to do this as a dumpAsText test and still get what you want?
Comment 14 Levi Weintraub 2013-02-13 11:25:04 PST
(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.
Comment 16 WebKit Review Bot 2013-02-13 13:57:03 PST
Comment on attachment 188127 [details] Patch Clearing flags on attachment: 188127 Committed r142793: <http://trac.webkit.org/changeset/142793>
Comment 17 WebKit Review Bot 2013-02-13 13:57:08 PST
All reviewed patches have been landed. Closing bug.