Issue 330974: Text duplication on <bdi>

Available on

data:text/html;charset=utf-8,<bdi>%D8%B3%D8%B3</bdi>%D8%A7

data:text/html;charset=utf-8,<span style="unicode-bidi: -webkit-isolate;">(5%D8%B3%D8%B4 </span>%DB%8C

Interesting, Midori (Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-us) AppleWebKit/537+ (KHTML, like Gecko) Version/5.0 Safari/537.6+ Midori/0.4) is OK

CHANGELOG URL:
  http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/trunk/src&range=223577%3A223608

This is a showstopper bug!!!

Google Web Search has just started using unicode-bidi:-webkit-isolate. Some other Google products have been using it (and relying on it) for months.

Reproduces on Version 31.0.1650.63 on Linux.
Reproduces on Version 32.0.1700.41 m Aura on Win7.
Reproduces on Version 34.0.1766.0 canary on Win7.

I am pretty sure that this is a regression, but cannot easily check earlier builds.

To reproduce, it seems that both of the following conditions have to hold:

1. The unicode-bidi:-webkit-isolate element contains more than one non-whitespace character. The directionality (or neutrality) of these characters does not matter.

2. The unicode-bidi:-webkit-isolate element is *immediately* followed by a strong character of the directionality opposite to the parent element's directionality, or, if the parent element is RTL, by a digit (makes no difference if EN or AN).

IMHO from http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/trunk/src&range=223577%3A223608 and http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog_blink.html?url=/trunk&range=157864:157879&mode=html this is most related change to http://src.chromium.org/viewvc/blink?revision=157874&view=revision so it would be nice if someone able CC igor.o

I am on it.

Thanks, Igor!

The fix was committed: https://src.chromium.org/viewvc/blink?revision=164396&view=revision

The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=164396

------------------------------------------------------------------------
r164396 | igor.o@sisa.samsung.com | 2013-12-31T09:12:45.743076Z

Changed paths:
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/text/international/unicode-bidi-isolate-duplicated-text-expected.html?r1=164396&r2=164395&pathrev=164396
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/text/international/unicode-bidi-isolate-duplicated-text.html?r1=164396&r2=164395&pathrev=164396
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/InlineIterator.h?r1=164396&r2=164395&pathrev=164396

When an isolated run is appended, m_eor(End Of Run) is wrongly incremented. 
Instead of moving to the next renderer, the iterator(m_eor) is just moving to the
next position in the same renderer.

BUG= 330974 

Review URL: https://codereview.chromium.org/122553005
------------------------------------------------------------------------

Thanks, Igor!

A whole spate of problems was discovered in this feature recently (many of them apparently dating back years to when it was originally developed). In your opinion, does the code look solid now, or should we expect more surprises?

Issue 331959 has been merged into this issue.

Can/should we merge this fix to M32 or M33? Adding M32 ReleaseBlock-Stable.

Aharon,

Yeah I believe we are already in a good shape. Now we can focus in features  
instead of bug fixing!

Yes, please, if at all possible, merge into M32.

I believe the m32 ship has long sailed, but we can add Merge-Requested to have the merge-master tell us that officially.  I suspect this will have to ship in 33.

we've cut stable 1 aleady so it's too late but depending on how this does, we can consider it for stable 2.

Tested the issue on Windows 8 and MacBook Pro 10.9 - canary version 34.0.1777.0 (Official Build 243882). Observed a deviation in the output when compared Chrome with Firefox browser.

Please find the attached screenshot for reference. Please confirm us on the fix.

Deleted: Firefox.png 30.8 KB

	Firefox.png 30.8 KB View Download

Deleted: Canary.png 19.7 KB

	Canary.png 19.7 KB View Download

I confirm the original bug is fixed. This behavior however should be checked with standards likes http://www.unicode.org/reports/tr9/ . From what I know this is OK and Firefox should be corrected. IMO the whole point of <bdi> is making the content inside isolated like an object so in the below testcase b should on right.

data:text/html,<div dir="rtl"><img height="11" width="11"></img>b</div><div dir="rtl"><bdi>A</bdi>b</div>

* On left, on both lines

Another testcase that both line should be same IMHO:
data:text/html,<div dir="rtl"><span style="display: inline-block;">A</span>b</div><div dir="rtl"><bdi>A</bdi>b</div>
OK on Canary but differs from Webkit and Firefox and I guess http://src.chromium.org/viewvc/blink?revision=157874&view=revision is its bisect.

Re comments 23 - 26:
- Canary's behavior is correct.
- FF behavior is a known bug (soon to be fixed in FF by going to the latest ICU implementation of UBA and implementation of unicode-bidi:isolate based on Unicode isolate formatting characters): https://bugzilla.mozilla.org/show_bug.cgi?id=717811
- Don't have a Mac handy right now, so unable to test WebKit.

aharon@, Thanks so much for you confirmation.

Adding 'TE-Verified-34.0.1777.0' label as per comments# 23&27.

Thank you.

let's merge to M33 first.

Merge approved for M33 (1750)

Sorry I was so slow to deal with this.

Committed revision 167218.

The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=167218

------------------------------------------------------------------------
r167218 | eseidel@chromium.org | 2014-02-14T19:35:35.635121Z

Changed paths:
   A http://src.chromium.org/viewvc/blink/branches/chromium/1750/LayoutTests/fast/text/international/unicode-bidi-isolate-duplicated-text-expected.html?r1=167218&r2=167217&pathrev=167218
   A http://src.chromium.org/viewvc/blink/branches/chromium/1750/LayoutTests/fast/text/international/unicode-bidi-isolate-duplicated-text.html?r1=167218&r2=167217&pathrev=167218
   M http://src.chromium.org/viewvc/blink/branches/chromium/1750/Source/core/rendering/InlineIterator.h?r1=167218&r2=167217&pathrev=167218

Merge 164396 "When an isolated run is appended, m_eor(End Of Run..."

> When an isolated run is appended, m_eor(End Of Run) is wrongly incremented. 
> Instead of moving to the next renderer, the iterator(m_eor) is just moving to the
> next position in the same renderer.
> 
> BUG= 330974 
> 
> Review URL: https://codereview.chromium.org/122553005

TBR=igor.o@sisa.samsung.com

Review URL: https://codereview.chromium.org/167513002
------------------------------------------------------------------------

In comment 22, kareng@google.com wrote:

> we've cut stable 1 aleady so it's too late but depending on how this does,
> we can consider it for stable 2.

Will there be a 32 stable 2, and can we please merge this into it?

Just FWIW there is a somehow similar issue on  Issue 454099  with this testcase: data:text/html;charset=utf8,<span dir="auto"><span dir="rtl">%E0%A4%B7%E0%A5%8D (%D8%B3)</span></span>