Bug 74396

Summary:

When <bdi> content is wrapped, all hell breaks loose

Product:

WebKit

Reporter:

Aharon (Vladimir) Lanin <aharon>

Component:

DOM

Assignee:

Ken Buchanan <kenrb>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aharon, eric, kenrb, leviw, playmobil, rniwa, webkit.review.bot, xji

Priority:

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

Aharon (Vladimir) Lanin

Reported 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.

Attachments
test case (ref file coming as separate attachment) (2.03 KB, text/html) 2011-12-13 03:14 PST, Aharon (Vladimir) Lanin	no flags	Details
ref file for the test case above (1.35 KB, text/html) 2011-12-13 03:15 PST, Aharon (Vladimir) Lanin	no flags	Details
Screenshot of the test case above on my Chrome Canary (37.92 KB, image/png) 2011-12-13 03:17 PST, Aharon (Vladimir) Lanin	no flags	Details
Screenshot after fixing the line break problem -- note the extra spaces (31.23 KB, image/png) 2011-12-13 20:24 PST, Ken Buchanan	no flags	Details
a test case that does not trigger the extra space bug. uses the same ref file as before. (2.04 KB, text/html) 2011-12-14 23:57 PST, Aharon (Vladimir) Lanin	no flags	Details
a test case that does not trigger the extra space bug. uses the same ref file as before. (3.71 KB, text/html) 2011-12-20 01:10 PST, Aharon (Vladimir) Lanin	no flags	Details
Patch (6.64 KB, patch) 2011-12-20 06:38 PST, Ken Buchanan	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Aharon (Vladimir) Lanin

Comment 1 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.

Aharon (Vladimir) Lanin

Comment 2 2011-12-13 03:15:31 PST

Created attachment 118991 [details] ref file for the test case above

Aharon (Vladimir) Lanin

Comment 3 2011-12-13 03:17:10 PST

Created attachment 118993 [details] Screenshot of the test case above on my Chrome Canary

Eric Seidel (no email)

Comment 4 2011-12-13 14:50:02 PST

Looks like this is going to be fixed by bug 74311.

Ken Buchanan

Comment 5 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.

Ken Buchanan

Comment 6 2011-12-13 20:24:41 PST

Created attachment 119144 [details] Screenshot after fixing the line break problem -- note the extra spaces

Aharon (Vladimir) Lanin

Comment 7 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?

Ryosuke Niwa

Comment 8 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.

Aharon (Vladimir) Lanin

Comment 9 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

Ken Buchanan

Comment 10 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?

Ryosuke Niwa

Comment 11 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.

Aharon (Vladimir) Lanin

Comment 12 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.

Aharon (Vladimir) Lanin

Comment 13 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.

Ken Buchanan

Comment 14 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.

Ryosuke Niwa

Comment 15 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.

Aharon (Vladimir) Lanin

Comment 16 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.

Ken Buchanan

Comment 17 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 > 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.

Aharon (Vladimir) Lanin

Comment 18 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 > 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.

Aharon (Vladimir) Lanin

Comment 19 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.

Ken Buchanan

Comment 20 2011-12-20 06:38:14 PST

Created attachment 120022 [details] Patch

Ken Buchanan

Comment 21 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.

WebKit Review Bot

Comment 22 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>

WebKit Review Bot

Comment 23 2011-12-20 13:11:56 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.