unicode-bidi: plaintext is treating embedded unicode-bidi: isolate incorrectly.

unicode-bidi: plaintext is treating embedded unicode-bidi: isolate incorrectly.

In the CSS Writing Modes specification:
for the purpose of bidi resolution in its containing bidi paragraph,
the sequence is treated as if it were a single Object Replacement Character (U+FFFC).

However Blink does not treat the isolated sequence as if it were a U+UFFFC.
This patch adds a new method called bidiFirstSkippingIsolated that is used to get the
first nested bidi not isolated.

BUG=276642
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156517

[igoroliveira, 2013-08-20 22:51:02]

Added new OWNERS.

[eseidel, 2013-08-20 23:01:37]

I'm OK with this.  I'd like to hear your thoughts on the design tradeoff of
splitting out bidiFirstSkippingIsolate like this.  Also ccing a bidi expert.

https://codereview.chromium.org/23264018/diff/1/Source/core/rendering/RenderB...
File Source/core/rendering/RenderBlockLineLayout.cpp (right):

https://codereview.chromium.org/23264018/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBlockLineLayout.cpp:347: static inline RenderObject*
bidiFirstSkippingIsolated(RenderObject* root, RenderObject* current = 0)
Bleh.  All these bidiFirst* functions are ugly.  Is there no nice way to share
code here?  I know that I split these because bidiFirst used to have a bunch of
differnet modes which didn't make sense to share code together.  But adding
another copy of this function smells bad.

[igoroliveira, 2013-08-21 01:06:31]

On 2013/08/20 23:01:37, eseidel wrote:
> I'm OK with this.  I'd like to hear your thoughts on the design tradeoff of
> splitting out bidiFirstSkippingIsolate like this.  Also ccing a bidi expert.
> 
>
https://codereview.chromium.org/23264018/diff/1/Source/core/rendering/RenderB...
> File Source/core/rendering/RenderBlockLineLayout.cpp (right):
> 
>
https://codereview.chromium.org/23264018/diff/1/Source/core/rendering/RenderB...
> Source/core/rendering/RenderBlockLineLayout.cpp:347: static inline
RenderObject*
> bidiFirstSkippingIsolated(RenderObject* root, RenderObject* current = 0)
> Bleh.  All these bidiFirst* functions are ugly.  Is there no nice way to share
> code here?  I know that I split these because bidiFirst used to have a bunch
of
> differnet modes which didn't make sense to share code together.  But adding
> another copy of this function smells bad.

In my first attempt to fix the bug i tried to unify bidiFirstSkippingIsolated
logic and bidiFirst*Inlines, however the code was a mess.
Currently, I am planing to open a bug to refactor bidiFirst* since we have fixes
around everywhere in InlineIterator and unify all the bidiFirst*
implementations.

[leviw_travelin_and_unemployed, 2013-08-21 02:04:37]

https://codereview.chromium.org/23264018/diff/1/Source/core/rendering/RenderB...
File Source/core/rendering/RenderBlockLineLayout.cpp (right):

https://codereview.chromium.org/23264018/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBlockLineLayout.cpp:347: static inline RenderObject*
bidiFirstSkippingIsolated(RenderObject* root, RenderObject* current = 0)
On 2013/08/20 23:01:37, eseidel wrote:
> Bleh.  All these bidiFirst* functions are ugly.  Is there no nice way to share
> code here?  I know that I split these because bidiFirst used to have a bunch
of
> differnet modes which didn't make sense to share code together.  But adding
> another copy of this function smells bad.

I just talked to Eric about this, and this seems like a different problem being
solved than bidiFirst.

It seems to me that determineDirectionality should be smart enough (and perhaps
renamed to indicate this intelligence) to do this work. Unless you feel there's
a case where it shouldn't behave this way, in which case I'm happy to listen to
that argument.

[igoroliveira, 2013-08-21 02:41:14]

On 2013/08/21 02:04:37, Levi wrote:
>
https://codereview.chromium.org/23264018/diff/1/Source/core/rendering/RenderB...
> File Source/core/rendering/RenderBlockLineLayout.cpp (right):
> 
>
https://codereview.chromium.org/23264018/diff/1/Source/core/rendering/RenderB...
> Source/core/rendering/RenderBlockLineLayout.cpp:347: static inline
RenderObject*
> bidiFirstSkippingIsolated(RenderObject* root, RenderObject* current = 0)
> On 2013/08/20 23:01:37, eseidel wrote:
> > Bleh.  All these bidiFirst* functions are ugly.  Is there no nice way to
share
> > code here?  I know that I split these because bidiFirst used to have a bunch
> of
> > differnet modes which didn't make sense to share code together.  But adding
> > another copy of this function smells bad.
> 
> I just talked to Eric about this, and this seems like a different problem
being
> solved than bidiFirst.
> 
> It seems to me that determineDirectionality should be smart enough (and
perhaps
> renamed to indicate this intelligence) to do this work. Unless you feel
there's
> a case where it shouldn't behave this way, in which case I'm happy to listen
to
> that argument.

Makes sense. In fact, IMO, determineDirectionality should be moved to
BidiResolver since it is a subset of UBA.
What do you guys think about in this issue we stick with
determineDirectionality(or rename it to determinePlaintextDirectionality)
with the "bidiFirstSkippingIsolated" inside determinePlaintextDirectionality
since BidiResolver does not have knowledge of render tree.
And i open an issue about moving the determinePlaintextDirectionality to
BidiResolver.
I do not want to move right now because i do not want to mix bug fixing with
refactoring in the same issue.
any thoughts?

[aharon, 2013-08-21 06:48:35]

I think that Eric was asking for my two cents, although I am not 100% sure
regarding what, specifically. Since I am not familiar with Blink's codebase, the
only thing I can easily say is that the bug report is correct;
unicode-bidi:plaintext is indeed supposed to skip over the content in any
embedded unicode-bidi:isolate or unicode-bidi:plaintext elements when
determining the directionality of the paragraph. This is made explicit in
Unicode 6.3's version of P2
(http://www.unicode.org/reports/tr9/proposed.html#P2), i.e. "In each paragraph,
find the first character of type L, AL, or R while skipping over any characters
between an isolate initiator and its matching PDI or, if it has no matching PDI,
the end of the paragraph.". To see how this is related, note that:
 * unicode-bidi:plaintext is supposed to be following P2: "the base
directionality of each of the element's bidi paragraphs (if a block container)
or isolated sequences (if an inline) is determined by following the heuristic in
rules P2 and P3 of the Unicode bidirectional algorithm";
 * unicode-bidi:isolate is supposed to correspond to "LRI (U+2066), for
direction: ltr, or RLI (U+2067), for direction: rtl, at the start of the
element, and a PDI (U+2069) at the end of the element";
 * unicode-bidi:plaintext is supposed to correspond to "an FSI (U+2068) at the
start of the element, and a PDI (U+2069) at the end of the element"

In any case, if you want my opinion about something else, let me know.

[eseidel, 2013-08-21 20:50:46]

On 2013/08/21 06:48:35, aharon wrote:
> I think that Eric was asking for my two cents, although I am not 100% sure
> regarding what, specifically. Since I am not familiar with Blink's codebase,
the
> only thing I can easily say is that the bug report is correct;

Sorry.  I should have been more clear.  I just wanted you to know about this
change and confirm that you believed it was correct.  You have much more
experiance with Bidi than I try to lean on you for a "does this behave right"
check.

> unicode-bidi:plaintext is indeed supposed to skip over the content in any
> embedded unicode-bidi:isolate or unicode-bidi:plaintext elements when
> determining the directionality of the paragraph. This is made explicit in
> Unicode 6.3's version of P2
> (http://www.unicode.org/reports/tr9/proposed.html#P2), i.e. "In each
paragraph,
> find the first character of type L, AL, or R while skipping over any
characters
> between an isolate initiator and its matching PDI or, if it has no matching
PDI,
> the end of the paragraph.". To see how this is related, note that:
>  * unicode-bidi:plaintext is supposed to be following P2: "the base
> directionality of each of the element's bidi paragraphs (if a block container)
> or isolated sequences (if an inline) is determined by following the heuristic
in
> rules P2 and P3 of the Unicode bidirectional algorithm";
>  * unicode-bidi:isolate is supposed to correspond to "LRI (U+2066), for
> direction: ltr, or RLI (U+2067), for direction: rtl, at the start of the
> element, and a PDI (U+2069) at the end of the element";
>  * unicode-bidi:plaintext is supposed to correspond to "an FSI (U+2068) at the
> start of the element, and a PDI (U+2069) at the end of the element"

thanks for confirming.

[eseidel, 2013-08-21 20:52:29]

https://codereview.chromium.org/23264018/diff/15001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlockLineLayout.cpp (right):

https://codereview.chromium.org/23264018/diff/15001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlockLineLayout.cpp:349: RenderObject* next =
current;
OK, so Levi's suggestion was good, but I still think this can be it's own
separate function, it just doens't need to have anything to do with bidiFirst*.

This is more firstRenderObjectForDirectionaityDetermination()?

I'm not exactly sure what the naming should be.

[eseidel, 2013-08-21 20:55:11]

lgtm with it broken back out with a descriptive name.  "bidiFirst" is a
meaningless prefix we inherited from khtml and just have never gone back and
renamed those functions.  You just stepped on some old code and thus got some
wrinkled noses from levi and I.  not because your code was ugly, but because it
made refernece to a very ugly part of our system.

[commit-bot: I haz the power, 2013-08-21 22:30:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/igor.o@sisa.samsung.com/23264018/22001

[commit-bot: I haz the power, 2013-08-22 00:17:45]

Change committed as 156517