Make unicode-bidi:isolate the default for elements with dir attributes

Make unicode-bidi:isolate the default for elements with dir attributes

This patch changes the unicode-bidi value for elements with dir
attributes to "isolate" as defined by HTML5 spec[1][2], except for bdo
and other elements defined in the spec.

Tests to apply the correct unicode-bidi value were imported from W3C
I18N WG. These tests were submitted to the web-platform-tests, and will
be removed when merged.

Tests for unicode-bidi:isolate are in CSS Writing Modes Level 3 test
suites and are imported to imported/csswg-test/css-writing-modes-3.

Following 3 tests were changed to apply "embed" since the reviews by
experts concluded that these tests are specifically for "embed", and
having these tests helps to find our possible regressions until authors
moved out from using "embed".
- fast/text/bidi-embedding-pop-and-push-same.html
- fast/text/international/bidi-LDB-2-HTML.html
- fast/text/international/bidi-ignored-for-first-child-inline.html

To align with the change, the execCommand("MakeTextWritingDirection")
is also changed to apply "isolate".

[1] http://www.w3.org/TR/html5/rendering.html#bidi-rendering
[2] https://html.spec.whatwg.org/multipage/rendering.html#bidi-rendering

BUG=391260
Committed: https://crrev.com/549f017c96ad0e70bfce3b6a2248cbf747dde26c
Cr-Commit-Position: refs/heads/master@{#362888}

[kojii, 2015-11-19 18:43:41]

Description was changed from

==========
WIP: dir-isolate

BUG=391260
==========

to

==========
Make unicode-bidi:isolate the default for an element with a dir attribute

BUG=391260
==========

[kojii, 2015-11-27 13:23:28]

Patchset #7 (id:120001) has been deleted

[kojii, 2015-12-01 18:13:12]

Description was changed from

==========
Make unicode-bidi:isolate the default for an element with a dir attribute

BUG=391260
==========

to

==========
Make unicode-bidi:isolate the default for elements with dir attributes

This patch changes the unicode-bidi value for elements with dir
attributes to "isolate" as defined by HTML5 spec[1][2], except for bdo
and other elements defined in the spec.

Tests to apply the correct value were imported from W3C I18N WG. These
tests were submitted to the web-platform-tests, and will be removed
when merged.

Tests for unicode-bidi:isolate are in CSS Writing Modes Level 3 test
suites and are imported to imported/csswg-test/css-writing-modes-3.

Following 3 tests were changed to apply "embed" as the reviews by
experts concluded that these tests are specifically for "embed", and
having these tests helps to find our possible regressions until authors
moved out from using "embed".
- fast/text/bidi-embedding-pop-and-push-same.html
- fast/text/international/bidi-LDB-2-HTML.html
- fast/text/international/bidi-ignored-for-first-child-inline.html

To align with the change, the execCommand("MakeTextWritingDirection")
is also changed to apply "isolate".

[1] http://www.w3.org/TR/html5/rendering.html#bidi-rendering
[2] https://html.spec.whatwg.org/multipage/rendering.html#bidi-rendering

BUG=391260
==========

[kojii, 2015-12-01 18:13:55]

Description was changed from

==========
Make unicode-bidi:isolate the default for elements with dir attributes

This patch changes the unicode-bidi value for elements with dir
attributes to "isolate" as defined by HTML5 spec[1][2], except for bdo
and other elements defined in the spec.

Tests to apply the correct value were imported from W3C I18N WG. These
tests were submitted to the web-platform-tests, and will be removed
when merged.

Tests for unicode-bidi:isolate are in CSS Writing Modes Level 3 test
suites and are imported to imported/csswg-test/css-writing-modes-3.

Following 3 tests were changed to apply "embed" as the reviews by
experts concluded that these tests are specifically for "embed", and
having these tests helps to find our possible regressions until authors
moved out from using "embed".
- fast/text/bidi-embedding-pop-and-push-same.html
- fast/text/international/bidi-LDB-2-HTML.html
- fast/text/international/bidi-ignored-for-first-child-inline.html

To align with the change, the execCommand("MakeTextWritingDirection")
is also changed to apply "isolate".

[1] http://www.w3.org/TR/html5/rendering.html#bidi-rendering
[2] https://html.spec.whatwg.org/multipage/rendering.html#bidi-rendering

BUG=391260
==========

to

==========
Make unicode-bidi:isolate the default for elements with dir attributes

This patch changes the unicode-bidi value for elements with dir
attributes to "isolate" as defined by HTML5 spec[1][2], except for bdo
and other elements defined in the spec.

Tests to apply the correct value were imported from W3C I18N WG. These
tests were submitted to the web-platform-tests, and will be removed
when merged.

Tests for unicode-bidi:isolate are in CSS Writing Modes Level 3 test
suites and are imported to imported/csswg-test/css-writing-modes-3.

Following 3 tests were changed to apply "embed" since the reviews by
experts concluded that these tests are specifically for "embed", and
having these tests helps to find our possible regressions until authors
moved out from using "embed".
- fast/text/bidi-embedding-pop-and-push-same.html
- fast/text/international/bidi-LDB-2-HTML.html
- fast/text/international/bidi-ignored-for-first-child-inline.html

To align with the change, the execCommand("MakeTextWritingDirection")
is also changed to apply "isolate".

[1] http://www.w3.org/TR/html5/rendering.html#bidi-rendering
[2] https://html.spec.whatwg.org/multipage/rendering.html#bidi-rendering

BUG=391260
==========

[kojii, 2015-12-01 18:14:53]

Description was changed from

==========
Make unicode-bidi:isolate the default for elements with dir attributes

This patch changes the unicode-bidi value for elements with dir
attributes to "isolate" as defined by HTML5 spec[1][2], except for bdo
and other elements defined in the spec.

Tests to apply the correct value were imported from W3C I18N WG. These
tests were submitted to the web-platform-tests, and will be removed
when merged.

Tests for unicode-bidi:isolate are in CSS Writing Modes Level 3 test
suites and are imported to imported/csswg-test/css-writing-modes-3.

Following 3 tests were changed to apply "embed" since the reviews by
experts concluded that these tests are specifically for "embed", and
having these tests helps to find our possible regressions until authors
moved out from using "embed".
- fast/text/bidi-embedding-pop-and-push-same.html
- fast/text/international/bidi-LDB-2-HTML.html
- fast/text/international/bidi-ignored-for-first-child-inline.html

To align with the change, the execCommand("MakeTextWritingDirection")
is also changed to apply "isolate".

[1] http://www.w3.org/TR/html5/rendering.html#bidi-rendering
[2] https://html.spec.whatwg.org/multipage/rendering.html#bidi-rendering

BUG=391260
==========

to

==========
Make unicode-bidi:isolate the default for elements with dir attributes

This patch changes the unicode-bidi value for elements with dir
attributes to "isolate" as defined by HTML5 spec[1][2], except for bdo
and other elements defined in the spec.

Tests to apply the correct unicode-bidi value were imported from W3C
I18N WG. These tests were submitted to the web-platform-tests, and will
be removed when merged.

Tests for unicode-bidi:isolate are in CSS Writing Modes Level 3 test
suites and are imported to imported/csswg-test/css-writing-modes-3.

Following 3 tests were changed to apply "embed" since the reviews by
experts concluded that these tests are specifically for "embed", and
having these tests helps to find our possible regressions until authors
moved out from using "embed".
- fast/text/bidi-embedding-pop-and-push-same.html
- fast/text/international/bidi-LDB-2-HTML.html
- fast/text/international/bidi-ignored-for-first-child-inline.html

To align with the change, the execCommand("MakeTextWritingDirection")
is also changed to apply "isolate".

[1] http://www.w3.org/TR/html5/rendering.html#bidi-rendering
[2] https://html.spec.whatwg.org/multipage/rendering.html#bidi-rendering

BUG=391260
==========

[kojii, 2015-12-02 00:47:02]

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

[kojii, 2015-12-02 00:47:03]

PTAL.

[kojii, 2015-12-02 00:48:10]

The document we used to discuss/review is here, JFYI:
https://docs.google.com/document/d/1Ojb95CQJ49aqmJ6SKVOMd_rNTGx8e0LEqjMFJBM1d...

[leviw_travelin_and_unemployed, 2015-12-02 18:25:38]

This patch is *super* exciting! Great work, Koji!

Are you fairly confident that this won't have serious compatibility issues on
real-world content? I suppose it's no too hard to revert worst-case scenario.

LGTM.

https://codereview.chromium.org/1463473002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1463473002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:525: # Ref tests that pass but
causes 1px diff on images.
:(

https://codereview.chromium.org/1463473002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/1463473002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/EditingStyle.cpp:1378: if
(unicodeBidiValue != CSSValueNormal && unicodeBidiValue != CSSValueInitial
Nit: I can understand why you did it like this, but it does make it more
difficult to understand what values are expected here.

[kojii, 2015-12-02 18:39:35]

Thank you for the review.

On 2015/12/02 at 18:25:38, leviw wrote:
> This patch is *super* exciting! Great work, Koji!
> 
> Are you fairly confident that this won't have serious compatibility issues on
real-world content?

For that part, I have to rely on Aharon and Richard@w3c. fantasai also supported
their opinions. There might be some, I'm actually not surprised if we get a few
bug reports asking to revert. But I do support that isolate is much better way
to go in a long run, and I'm hoping those a few can be convinced to add
"span[dir] { unicode-bidi:embed; }".

Isolate is much simpler than embed in terms of algorithm, so I think it can
improve our implementation quality too.

> I suppose it's no too hard to revert worst-case scenario.

Yes, just the code part in this CL. Two CLs I did before are not needed to
revert.

> LGTM.
> 
>
https://codereview.chromium.org/1463473002/diff/200001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/1463473002/diff/200001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/TestExpectations:525: # Ref tests that pass but
causes 1px diff on images.
> :(

Yeah, I spent some time to fix the tests, but it was hard. I'm guessing
fractional width snaps to logical-start side, and if so, it's unfixable.

>
https://codereview.chromium.org/1463473002/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/editing/EditingStyle.cpp (right):
> 
>
https://codereview.chromium.org/1463473002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/editing/EditingStyle.cpp:1378: if
(unicodeBidiValue != CSSValueNormal && unicodeBidiValue != CSSValueInitial
> Nit: I can understand why you did it like this, but it does make it more
difficult to understand what values are expected here.

There are 5 values and 3 prefixed values, do you prefer to list them here? I'm
ok either way. Not sure which is more likely; new values to unicode-bidi or new
special values to CSS Values.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

The other option is extract to a function (e.g., isNonInitialUnicodeBidiValue)
-- do you think that might help to understand?

[leviw_travelin_and_unemployed, 2015-12-02 18:49:55]

On 2015/12/02 at 18:39:35, kojii wrote:
> Thank you for the review.
> 
> On 2015/12/02 at 18:25:38, leviw wrote:
> > This patch is *super* exciting! Great work, Koji!
> > 
> > Are you fairly confident that this won't have serious compatibility issues
on real-world content?
> 
> For that part, I have to rely on Aharon and Richard@w3c. fantasai also
supported their opinions. There might be some, I'm actually not surprised if we
get a few bug reports asking to revert. But I do support that isolate is much
better way to go in a long run, and I'm hoping those a few can be convinced to
add "span[dir] { unicode-bidi:embed; }".
> 
> Isolate is much simpler than embed in terms of algorithm, so I think it can
improve our implementation quality too.

Your statements echo my thoughts. Thanks for the summary :)

> 
> > I suppose it's no too hard to revert worst-case scenario.
> 
> Yes, just the code part in this CL. Two CLs I did before are not needed to
revert.
> 
> > LGTM.
> > 
> >
https://codereview.chromium.org/1463473002/diff/200001/third_party/WebKit/Lay...
> > File third_party/WebKit/LayoutTests/TestExpectations (right):
> > 
> >
https://codereview.chromium.org/1463473002/diff/200001/third_party/WebKit/Lay...
> > third_party/WebKit/LayoutTests/TestExpectations:525: # Ref tests that pass
but causes 1px diff on images.
> > :(
> 
> Yeah, I spent some time to fix the tests, but it was hard. I'm guessing
fractional width snaps to logical-start side, and if so, it's unfixable.

I figured as much. Sadly, with these tests, you can't use Ahem to fix 'em,
either :(

> 
> >
https://codereview.chromium.org/1463473002/diff/200001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/editing/EditingStyle.cpp (right):
> > 
> >
https://codereview.chromium.org/1463473002/diff/200001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/editing/EditingStyle.cpp:1378: if
(unicodeBidiValue != CSSValueNormal && unicodeBidiValue != CSSValueInitial
> > Nit: I can understand why you did it like this, but it does make it more
difficult to understand what values are expected here.
> 
> There are 5 values and 3 prefixed values, do you prefer to list them here? I'm
ok either way. Not sure which is more likely; new values to unicode-bidi or new
special values to CSS Values.

I'd lean towards the specific ones expected.

>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> The other option is extract to a function (e.g., isNonInitialUnicodeBidiValue)
-- do you think that might help to understand?

[kojii, 2015-12-03 02:04:42]

On 2015/12/02 18:49:55, leviw wrote:
> > 
> > There are 5 values and 3 prefixed values, do you prefer to list them here?
I'm
> ok either way. Not sure which is more likely; new values to unicode-bidi or
new
> special values to CSS Values.
> 
> I'd lean towards the specific ones expected.

Done, thank you for your advice!

[kojii, 2015-12-03 02:12:57]

The CQ bit was checked by kojii@chromium.org

[kojii, 2015-12-03 02:13:05]

The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org
Link to the patchset: https://codereview.chromium.org/1463473002/#ps240001
(title: "leviw review")

[commit-bot: I haz the power, 2015-12-03 02:13:32]

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

[eae, 2015-12-03 03:08:17]

LGTM

[commit-bot: I haz the power, 2015-12-03 03:27:31]

Description was changed from

==========
Make unicode-bidi:isolate the default for elements with dir attributes

This patch changes the unicode-bidi value for elements with dir
attributes to "isolate" as defined by HTML5 spec[1][2], except for bdo
and other elements defined in the spec.

Tests to apply the correct unicode-bidi value were imported from W3C
I18N WG. These tests were submitted to the web-platform-tests, and will
be removed when merged.

Tests for unicode-bidi:isolate are in CSS Writing Modes Level 3 test
suites and are imported to imported/csswg-test/css-writing-modes-3.

Following 3 tests were changed to apply "embed" since the reviews by
experts concluded that these tests are specifically for "embed", and
having these tests helps to find our possible regressions until authors
moved out from using "embed".
- fast/text/bidi-embedding-pop-and-push-same.html
- fast/text/international/bidi-LDB-2-HTML.html
- fast/text/international/bidi-ignored-for-first-child-inline.html

To align with the change, the execCommand("MakeTextWritingDirection")
is also changed to apply "isolate".

[1] http://www.w3.org/TR/html5/rendering.html#bidi-rendering
[2] https://html.spec.whatwg.org/multipage/rendering.html#bidi-rendering

BUG=391260
==========

to

==========
Make unicode-bidi:isolate the default for elements with dir attributes

This patch changes the unicode-bidi value for elements with dir
attributes to "isolate" as defined by HTML5 spec[1][2], except for bdo
and other elements defined in the spec.

Tests to apply the correct unicode-bidi value were imported from W3C
I18N WG. These tests were submitted to the web-platform-tests, and will
be removed when merged.

Tests for unicode-bidi:isolate are in CSS Writing Modes Level 3 test
suites and are imported to imported/csswg-test/css-writing-modes-3.

Following 3 tests were changed to apply "embed" since the reviews by
experts concluded that these tests are specifically for "embed", and
having these tests helps to find our possible regressions until authors
moved out from using "embed".
- fast/text/bidi-embedding-pop-and-push-same.html
- fast/text/international/bidi-LDB-2-HTML.html
- fast/text/international/bidi-ignored-for-first-child-inline.html

To align with the change, the execCommand("MakeTextWritingDirection")
is also changed to apply "isolate".

[1] http://www.w3.org/TR/html5/rendering.html#bidi-rendering
[2] https://html.spec.whatwg.org/multipage/rendering.html#bidi-rendering

BUG=391260
==========

[commit-bot: I haz the power, 2015-12-03 03:27:32]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2015-12-03 03:28:26]

Description was changed from

==========
Make unicode-bidi:isolate the default for elements with dir attributes

This patch changes the unicode-bidi value for elements with dir
attributes to "isolate" as defined by HTML5 spec[1][2], except for bdo
and other elements defined in the spec.

Tests to apply the correct unicode-bidi value were imported from W3C
I18N WG. These tests were submitted to the web-platform-tests, and will
be removed when merged.

Tests for unicode-bidi:isolate are in CSS Writing Modes Level 3 test
suites and are imported to imported/csswg-test/css-writing-modes-3.

Following 3 tests were changed to apply "embed" since the reviews by
experts concluded that these tests are specifically for "embed", and
having these tests helps to find our possible regressions until authors
moved out from using "embed".
- fast/text/bidi-embedding-pop-and-push-same.html
- fast/text/international/bidi-LDB-2-HTML.html
- fast/text/international/bidi-ignored-for-first-child-inline.html

To align with the change, the execCommand("MakeTextWritingDirection")
is also changed to apply "isolate".

[1] http://www.w3.org/TR/html5/rendering.html#bidi-rendering
[2] https://html.spec.whatwg.org/multipage/rendering.html#bidi-rendering

BUG=391260
==========

to

==========
Make unicode-bidi:isolate the default for elements with dir attributes

This patch changes the unicode-bidi value for elements with dir
attributes to "isolate" as defined by HTML5 spec[1][2], except for bdo
and other elements defined in the spec.

Tests to apply the correct unicode-bidi value were imported from W3C
I18N WG. These tests were submitted to the web-platform-tests, and will
be removed when merged.

Tests for unicode-bidi:isolate are in CSS Writing Modes Level 3 test
suites and are imported to imported/csswg-test/css-writing-modes-3.

Following 3 tests were changed to apply "embed" since the reviews by
experts concluded that these tests are specifically for "embed", and
having these tests helps to find our possible regressions until authors
moved out from using "embed".
- fast/text/bidi-embedding-pop-and-push-same.html
- fast/text/international/bidi-LDB-2-HTML.html
- fast/text/international/bidi-ignored-for-first-child-inline.html

To align with the change, the execCommand("MakeTextWritingDirection")
is also changed to apply "isolate".

[1] http://www.w3.org/TR/html5/rendering.html#bidi-rendering
[2] https://html.spec.whatwg.org/multipage/rendering.html#bidi-rendering

BUG=391260

Committed: https://crrev.com/549f017c96ad0e70bfce3b6a2248cbf747dde26c
Cr-Commit-Position: refs/heads/master@{#362888}
==========

[commit-bot: I haz the power, 2015-12-03 03:28:27]

Patchset 12 (id:??) landed as
https://crrev.com/549f017c96ad0e70bfce3b6a2248cbf747dde26c
Cr-Commit-Position: refs/heads/master@{#362888}