| 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> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | eric, gyuyoung.kim, hyatt, leviw, mitz, ojan.autocc, playmobil, rakuco, rniwa, webkit.review.bot | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| URL: | https://code.google.com/p/chromium/issues/detail?id=171942 | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 109624 | ||||||||||||
| Attachments: |
|
||||||||||||
Yes, that is definitely wrong. I wonder if this is a recent regression? (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. Yeah, the question is just if builds right after we added isolate were broken. Which is from like summer/fall of '11. 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> 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. (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. (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. (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. 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. 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 :-/
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. Created attachment 187956 [details]
Patch
Comment on attachment 187956 [details]
Patch
Is it possible to do this as a dumpAsText test and still get what you want?
(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. Created attachment 188127 [details]
Patch
Comment on attachment 188127 [details] Patch Clearing flags on attachment: 188127 Committed r142793: <http://trac.webkit.org/changeset/142793> All reviewed patches have been landed. Closing bug. |
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