Bug 74396

Summary: When <bdi> content is wrapped, all hell breaks loose
Product: WebKit Reporter: Aharon (Vladimir) Lanin <aharon>
Component: DOMAssignee: Ken Buchanan <kenrb>
Status: RESOLVED FIXED    
Severity: Normal CC: aharon, eric, kenrb, leviw, playmobil, rniwa, webkit.review.bot, xji
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 74311    
Bug Blocks: 50910    
Attachments:
Description Flags
test case (ref file coming as separate attachment)
none
ref file for the test case above
none
Screenshot of the test case above on my Chrome Canary
none
Screenshot after fixing the line break problem -- note the extra spaces
none
a test case that does not trigger the extra space bug. uses the same ref file as before.
none
a test case that does not trigger the extra space bug. uses the same ref file as before.
none
Patch none

Description Aharon (Vladimir) Lanin 2011-12-13 03:11:35 PST
<bdi> mostly works fine when all its content is displayed on one line. However, if the content is too long for the remainder of the line and gets wrapped, the results are rather spectacular, and I can't even adequately describe what happens. Basically, some content gets rendered more than once, and a lot is rendered in the wrong location. I am attaching a test case and a screenshot of how it gets rendered on my Chrome Canary.
Comment 1 Aharon (Vladimir) Lanin 2011-12-13 03:14:01 PST
Created attachment 118990 [details]
test case (ref file coming as separate attachment)

This is one of the tests being submitted to public-html-testsuite@w3.org for inclusion into the W3C's HTML5 test suite.
Comment 2 Aharon (Vladimir) Lanin 2011-12-13 03:15:31 PST
Created attachment 118991 [details]
ref file for the test case above
Comment 3 Aharon (Vladimir) Lanin 2011-12-13 03:17:10 PST
Created attachment 118993 [details]
Screenshot of the test case above on my Chrome Canary
Comment 4 Eric Seidel (no email) 2011-12-13 14:50:02 PST
Looks like this is going to be fixed by bug 74311.
Comment 5 Ken Buchanan 2011-12-13 20:22:40 PST
It looks like there are two bugs here: one is with the line breaks inside an isolate, and another apparently with the transition from LTR to RTL text.

The attached test renders very close to correctly when I fix the line break issue, because I had written a patch for bug 74311. However the result after fixing that is two extra spaces in the rendered text, apparently unrelated to the earlier problem. As a result I can't submit the ref test because it would break. I'll upload the patch I have on 74311 and leave this bug open for the spacing issue.

Also I'll put another screenshot on this bug to show the result after my patch is applied.
Comment 6 Ken Buchanan 2011-12-13 20:24:41 PST
Created attachment 119144 [details]
Screenshot after fixing the line break problem -- note the extra spaces
Comment 7 Aharon (Vladimir) Lanin 2011-12-13 22:58:41 PST
(In reply to comment #6)
> Created an attachment (id=119144) [details]
> Screenshot after fixing the line break problem -- note the extra spaces

1. I drool.

2. You are quite right about the extra space being a separate issue. It also reproduces on a simple opposite-direction <span dir=...> wrapped in the same manner. Should I file a bug on that?
Comment 8 Ryosuke Niwa 2011-12-13 23:11:50 PST
(In reply to comment #7)
> (In reply to comment #6)
> > Created an attachment (id=119144) [details] [details]
> > Screenshot after fixing the line break problem -- note the extra spaces
> 
> 1. I drool.
> 
> 2. You are quite right about the extra space being a separate issue. It also reproduces on a simple opposite-direction <span dir=...> wrapped in the same manner. Should I file a bug on that?

Please do.
Comment 9 Aharon (Vladimir) Lanin 2011-12-14 00:52:19 PST
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Created an attachment (id=119144) [details] [details] [details]
> > > Screenshot after fixing the line break problem -- note the extra spaces
> > 
> > 1. I drool.
> > 
> > 2. You are quite right about the extra space being a separate issue. It also reproduces on a simple opposite-direction <span dir=...> wrapped in the same manner. Should I file a bug on that?
> 
> Please do.

Filed as https://bugs.webkit.org/show_bug.cgi?id=74489
Comment 10 Ken Buchanan 2011-12-14 09:23:01 PST
rniwa: How do you want me to proceed with this now? This and 74311 are the same underlying bug, but I was going to submit the line break fix on this bug so it would be public, and keep the security-specific part on 74311. Unfortunately I can't add Aharon's test case to my patch because it is still broken. Should I upload the line break patch here with no test?
Comment 11 Ryosuke Niwa 2011-12-14 21:23:41 PST
(In reply to comment #10)
> rniwa: How do you want me to proceed with this now? This and 74311 are the same underlying bug, but I was going to submit the line break fix on this bug so it would be public, and keep the security-specific part on 74311. Unfortunately I can't add Aharon's test case to my patch because it is still broken. Should I upload the line break patch here with no test?

I think we should fix the bug 74489 first. Meanwhile maybe Aharon can come up with a test case that doesn't involve whitespace :) Just uploading a work-in-progress patch sounds great to me.
Comment 12 Aharon (Vladimir) Lanin 2011-12-14 23:57:30 PST
Created attachment 119386 [details]
a test case that does not trigger the extra space bug. uses the same ref file as before.
Comment 13 Aharon (Vladimir) Lanin 2011-12-15 00:00:24 PST
(In reply to comment #12)
> Created an attachment (id=119386) [details]
> a test case that does not trigger the extra space bug. uses the same ref file as before.

This should be able to serve as a test case for the fix you have, so you don't have to fix bug 74489 first.
Comment 14 Ken Buchanan 2011-12-15 07:54:49 PST
My patch landed last night, using bug 74311.

http://trac.webkit.org/changeset/102875

You could check that test against ToT and submit it if it passes. I think it wouldn't need review.
Comment 15 Ryosuke Niwa 2011-12-15 10:05:57 PST
(In reply to comment #14)
> My patch landed last night, using bug 74311.
> 
> http://trac.webkit.org/changeset/102875
> 
> You could check that test against ToT and submit it if it passes. I think it wouldn't need review.

Adding a test requires a formal review just like any other changes to WebKit.
Comment 16 Aharon (Vladimir) Lanin 2011-12-18 02:44:03 PST
(In reply to comment #15)
> (In reply to comment #14)
> > My patch landed last night, using bug 74311.
> > 
> > http://trac.webkit.org/changeset/102875
> > 
> > You could check that test against ToT and submit it if it passes. I think it wouldn't need review.
> 
> Adding a test requires a formal review just like any other changes to WebKit.

It would be great if someone could add my test case so we can close this bug. I am not set up to do WebKit development.
Comment 17 Ken Buchanan 2011-12-19 21:46:48 PST
This test doesn't seem to render quite right either, for a different reason than the first one. The &gt; symbols on the right side don't end up in the right place.

Strangely, this doesn't happen in the original test you uploaded (which just has the problem with spaces).

You can try this on Chrome Canary to see the result.
Comment 18 Aharon (Vladimir) Lanin 2011-12-20 01:09:05 PST
(In reply to comment #17)
> This test doesn't seem to render quite right either, for a different reason than the first one. The &gt; symbols on the right side don't end up in the right place.
> 
> Strangely, this doesn't happen in the original test you uploaded (which just has the problem with spaces).
> 
> You can try this on Chrome Canary to see the result.

Strange, it displayed fine in the Canary I used when I attached it, but doesn't now. It looks like a bug in -webkit-padding-end, and should be looked at in its own right.

Attaching yet another version that seems to display fine in Canary now.
Comment 19 Aharon (Vladimir) Lanin 2011-12-20 01:10:52 PST
Created attachment 119994 [details]
a test case that does not trigger the extra space bug. uses the same ref file as before.
Comment 20 Ken Buchanan 2011-12-20 06:38:14 PST
Created attachment 120022 [details]
Patch
Comment 21 Ken Buchanan 2011-12-20 06:41:52 PST
Ryosuke, are you able to have a look at this test case? It seems to render correctly now.
Comment 22 WebKit Review Bot 2011-12-20 13:11:50 PST
Comment on attachment 120022 [details]
Patch

Clearing flags on attachment: 120022

Committed r103345: <http://trac.webkit.org/changeset/103345>
Comment 23 WebKit Review Bot 2011-12-20 13:11:56 PST
All reviewed patches have been landed.  Closing bug.