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

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Aharon (Vladimir) Lanin]

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

[Aharon (Vladimir) Lanin]

Created attachment 118990 [details]
test case (ref file coming as separate attachment)

This is one of the tests being submitted to [e-mail] for inclusion into the W3C's HTML5 test suite.

[Aharon (Vladimir) Lanin]

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

[Aharon (Vladimir) Lanin]

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

[Eric Seidel (no email)]

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

[Ken Buchanan]

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]

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

[Aharon (Vladimir) Lanin]

(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]

(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]

(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]

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]

(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]

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]

(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]

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]

(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]

(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]

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]

(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]

Created attachment 119994 [details]
a test case that does not trigger the extra space bug. uses the same ref file as before.

[Ken Buchanan]

Created attachment 120022 [details]
Patch

[Ken Buchanan]

Ryosuke, are you able to have a look at this test case? It seems to render correctly now.

[WebKit Review Bot]

Comment on attachment 120022 [details]
Patch

Clearing flags on attachment: 120022

Committed r103345: <http://trac.webkit.org/changeset/103345>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.