Ignore unicode-bidi: isolate for visual order elements

Ignore unicode-bidi: isolate for visual order elements

This patch makes visual order elements to ignore bidi-isolate[1]. As a
result, such elements with unicode-bidi: isolate no longer bidi-reorder
their content.

The behavior of bidi-isolate for visual order (iso-8859-8) is not well
defined. It is reasonable to ignore bidi-isolate, however, given visual
order documents ignore dir attributes today.

Without this fix, when bidi-isolate is turned on by default in
crbug.com/391260, dir attributes start to effect, which is likely to
break web-compat.

[1] https://drafts.csswg.org/css-writing-modes-3/#bidi-isolate

BUG=559257
Committed: https://crrev.com/a92fdd6a08d474d8fb27f69970e541442a5ad0c4
Cr-Commit-Position: refs/heads/master@{#361283}

[kojii, 2015-11-20 20:04:54]

Description was changed from

==========
WIP: visual-isolate

BUG=559257
==========

to

==========
Ignore unicode-bidi: isolate for visual order elements

This patch fixes visual order elements to ignore bidi-isolate[1] and
prevent such elements with unicode-bidi: isolate to bidi-reorder.

The use of unicode-bidi: isolate to visual order documents such as
iso-8859-8 is unlikely, but crbug.com/391260 to make isolate by default
can hit this issue.

[1] https://drafts.csswg.org/css-writing-modes-3/#bidi-isolate

BUG=559257
==========

[kojii, 2015-11-21 06:37:22]

Description was changed from

==========
Ignore unicode-bidi: isolate for visual order elements

This patch fixes visual order elements to ignore bidi-isolate[1] and
prevent such elements with unicode-bidi: isolate to bidi-reorder.

The use of unicode-bidi: isolate to visual order documents such as
iso-8859-8 is unlikely, but crbug.com/391260 to make isolate by default
can hit this issue.

[1] https://drafts.csswg.org/css-writing-modes-3/#bidi-isolate

BUG=559257
==========

to

==========
Ignore unicode-bidi: isolate for visual order elements

This patch makes visual order elements to ignore bidi-isolate[1]. As a
result, such elements with unicode-bidi: isolate no longer bidi-reorder
their content.

The behavior of bidi-isolate for visual order (iso-8859-8) is not well
defined, and since visual order is legacy, it's unlikely to be defined.

However, the current behavior is blocking crbug.com/391260 to make
isolate by default, and it will encourage users to move away from
legacy when new features such as bidi-isolate require more conforming
encoding.

[1] https://drafts.csswg.org/css-writing-modes-3/#bidi-isolate

BUG=559257
==========

[kojii, 2015-11-22 10:23:48]

kojii@chromium.org changed reviewers:
+ eae@chromium.org, leviw@chromium.org

[kojii, 2015-11-22 10:23:48]

PTAL. Got offline confirmation from Aharon.

[kojii, 2015-11-22 10:26:50]

Description was changed from

==========
Ignore unicode-bidi: isolate for visual order elements

This patch makes visual order elements to ignore bidi-isolate[1]. As a
result, such elements with unicode-bidi: isolate no longer bidi-reorder
their content.

The behavior of bidi-isolate for visual order (iso-8859-8) is not well
defined, and since visual order is legacy, it's unlikely to be defined.

However, the current behavior is blocking crbug.com/391260 to make
isolate by default, and it will encourage users to move away from
legacy when new features such as bidi-isolate require more conforming
encoding.

[1] https://drafts.csswg.org/css-writing-modes-3/#bidi-isolate

BUG=559257
==========

to

==========
Ignore unicode-bidi: isolate for visual order elements

This patch makes visual order elements to ignore bidi-isolate[1]. As a
result, such elements with unicode-bidi: isolate no longer bidi-reorder
their content.

The behavior of bidi-isolate for visual order (iso-8859-8) is not well
defined, and since visual order is legacy, it's unlikely to be defined.
It is reasonable to ignore bidi-isolate, however, given visual order
documents ignore dir attributes today.

The current behavior is blocking crbug.com/391260 to make isolate by
default, and it will encourage users to move away from legacy when new
features such as bidi-isolate require more conforming encoding.

[1] https://drafts.csswg.org/css-writing-modes-3/#bidi-isolate

BUG=559257
==========

[kojii, 2015-11-22 10:32:37]

Description was changed from

==========
Ignore unicode-bidi: isolate for visual order elements

This patch makes visual order elements to ignore bidi-isolate[1]. As a
result, such elements with unicode-bidi: isolate no longer bidi-reorder
their content.

The behavior of bidi-isolate for visual order (iso-8859-8) is not well
defined, and since visual order is legacy, it's unlikely to be defined.
It is reasonable to ignore bidi-isolate, however, given visual order
documents ignore dir attributes today.

The current behavior is blocking crbug.com/391260 to make isolate by
default, and it will encourage users to move away from legacy when new
features such as bidi-isolate require more conforming encoding.

[1] https://drafts.csswg.org/css-writing-modes-3/#bidi-isolate

BUG=559257
==========

to

==========
Ignore unicode-bidi: isolate for visual order elements

This patch makes visual order elements to ignore bidi-isolate[1]. As a
result, such elements with unicode-bidi: isolate no longer bidi-reorder
their content.

The behavior of bidi-isolate for visual order (iso-8859-8) is not well
defined. It is reasonable to ignore bidi-isolate, however, given visual
order documents ignore dir attributes today.

Without this fix, when bidi-isolate is turned on by default in
crbug.com/391260, dir attributes start to effect, which is likely to
break web-compat.

[1] https://drafts.csswg.org/css-writing-modes-3/#bidi-isolate

BUG=559257
==========

[leviw_travelin_and_unemployed, 2015-11-23 19:27:53]

Small naming concern, but otherwise this lgtm

https://codereview.chromium.org/1463893002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/line/InlineIterator.h (right):

https://codereview.chromium.org/1463893002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/line/InlineIterator.h:133: static inline
bool isIsolated(const ComputedStyle& style)
How about a different name here to highlight the difference between this version
and the one in UnicodeBidi.h?

[kojii, 2015-11-24 00:53:01]

Thank you for your prompt review.

On 2015/11/23 at 19:27:53, leviw wrote:
> Small naming concern, but otherwise this lgtm
> 
>
https://codereview.chromium.org/1463893002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/line/InlineIterator.h (right):
> 
>
https://codereview.chromium.org/1463893002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/line/InlineIterator.h:133: static inline
bool isIsolated(const ComputedStyle& style)
> How about a different name here to highlight the difference between this
version and the one in UnicodeBidi.h?

hm, how about |isIsolatedStyle()|?

[leviw_travelin_and_unemployed, 2015-11-24 01:13:14]

On 2015/11/24 at 00:53:01, kojii wrote:
> Thank you for your prompt review.
> 
> On 2015/11/23 at 19:27:53, leviw wrote:
> > Small naming concern, but otherwise this lgtm
> > 
> >
https://codereview.chromium.org/1463893002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/line/InlineIterator.h (right):
> > 
> >
https://codereview.chromium.org/1463893002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/line/InlineIterator.h:133: static
inline bool isIsolated(const ComputedStyle& style)
> > How about a different name here to highlight the difference between this
version and the one in UnicodeBidi.h?
> 
> hm, how about |isIsolatedStyle()|?

That would work. I'd also suggest "treatAsIsolated". Pick one and go ahead and
land :)

[eae, 2015-11-24 01:31:39]

LGTM (once you've addressed levi's naming concern).

[kojii, 2015-11-24 05:12:51]

The CQ bit was checked by kojii@chromium.org

[kojii, 2015-11-24 05:12:52]

The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1463893002/#ps60001
(title: "Renamed to treatAsIsolated()")

[commit-bot: I haz the power, 2015-11-24 05:14:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463893002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463893002/60001

[kojii, 2015-11-24 05:15:01]

On 2015/11/24 01:13:14, leviw wrote:
> On 2015/11/24 at 00:53:01, kojii wrote:
> > Thank you for your prompt review.
> > 
> > On 2015/11/23 at 19:27:53, leviw wrote:
> > > Small naming concern, but otherwise this lgtm
> > > 
> > >
>
https://codereview.chromium.org/1463893002/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/layout/line/InlineIterator.h (right):
> > > 
> > >
>
https://codereview.chromium.org/1463893002/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/layout/line/InlineIterator.h:133: static
> inline bool isIsolated(const ComputedStyle& style)
> > > How about a different name here to highlight the difference between this
> version and the one in UnicodeBidi.h?
> > 
> > hm, how about |isIsolatedStyle()|?
> 
> That would work. I'd also suggest "treatAsIsolated". Pick one and go ahead and
> land :)

Picked "treatAsIsolated", thank you for the suggestion.

[leviw_travelin_and_unemployed, 2015-11-24 06:07:22]

> Picked "treatAsIsolated", thank you for the suggestion.

Awesome! Thanks for another good patch :)

[commit-bot: I haz the power, 2015-11-24 06:56:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-24 06:56:01]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kojii, 2015-11-24 07:44:36]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2015-11-24 07:46:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1463893002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1463893002/60001

[commit-bot: I haz the power, 2015-11-24 08:13:57]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-24 08:14:43]

Patchset 4 (id:??) landed as
https://crrev.com/a92fdd6a08d474d8fb27f69970e541442a5ad0c4
Cr-Commit-Position: refs/heads/master@{#361283}