Bug 134630 – Make unicode-bidi:isolate the default for an element with a dir attribute (instead of unicode-bidi:embed)

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[r12a]

The HTML5 spec changes the semantics of the dir attribute to use unicode-bidi:isolate instead of unicode-bidi:embed (on all elements except BDO), and unicode-bidi: isolate-override instead of unicode-bidi:bidi-override on BDO.

Here is a link to the relevant section of the HTML5 spec and the CSS involved:
http://www.w3.org/TR/html5/rendering.html#bidi-rendering

Similarly, the definition of the dir attribute in the HTML5 spec speaks of it making the element's content "directionally isolated"; see http://www.w3.org/TR/html5/dom.html#the-dir-attribute

This is a very important change that should make it easier to author bidi pages. It was proposed and extensively discussed by the W3C i18n WG (http://www.w3.org/International/wiki/Html-bidi-isolation), and then by the WHATWG and HTML5 WG (https://www.w3.org/Bugs/Public/show_bug.cgi?id=23260).

It is also a very easily implemented change - just a change in the default CSS.

The change is not backward compatible, but it is very close to the behavior of IE 8 through IE 11. The bodies mentioned above were well aware of the lack of backward compatibility, but approved the change nonetheless because of its benefits going forward.

It is now the standard.

(Chrome's implementation of unicode-bidi:isolate (prefixed as -webkit-isolate) has undergone a number of bug fixes over the past year and a half and is currently stable and, as far as Aharon Lanin knows, bug-free. For the past three months, all Google Web Search and Google Maps pages have already included a piece of CSS (very similar to that in http://www.w3.org/TR/html5/rendering.html#bidi-rendering) that implements this behavior, i.e. applies isolation to all elements with the dir attribute. It fixed a number of problems in those pages, and did not cause any problems. In a couple of cases, it caused a visible change in the display that was considered to be neither good nor bad, just different.)

It is time to implement the HTML5 spec and make this Webkit's default behavior for all pages.

BTW, this also requires a fix for bug 124146, for those cases affected. (This has already been fixed in Chrome, and Aharon Lanin can provide help if needed.)

[r12a]

Anyone looking at this?

[Myles C. Maxfield]

It looks like HTMLElement::collectStyleForPresentationAttribute() is relevant.

[Myles C. Maxfield]

Created attachment 264658 [details]
Needs tests

[WebKit Commit Bot]

Attachment 264658 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[r12a]

There are some tests available at http://www.w3.org/International/tests/repo/results/the-dir-attribute-isolation

The section "With CSS shim" shows results for the same tests using the CSS described at http://www.w3.org/International/articles/inline-bidi-markup/Overview#cssshim

It's some time since i updated the results, but i doubt much, if anything, has changed. I'll take a look.

[Myles C. Maxfield]

Related: http://www.w3.org/International/questions/qa-bidi-controls http://www.w3.org/International/tests/ http://www.w3.org/International/questions/qa-bidi-controls

[Myles C. Maxfield]

Also see https://bugs.webkit.org/show_bug.cgi?id=149170

[Build Bot]

Comment on attachment 264658 [details]
Needs tests

Attachment 264658 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/375132

New failing tests:
fast/text/international/bidi-LDB-2-HTML.html
fast/text/bidi-embedding-pop-and-push-same.html
editing/style/make-text-writing-direction-inline-mac.html
fast/css/absolute-inline-alignment-2.html
fast/css/default-bidi-css-rules.html
fast/text/international/iso-8859-8.html
editing/selection/move-by-word-visually-single-space-inline-element.html
editing/selection/move-by-word-visually-mac.html
fast/text/international/bidi-ignored-for-first-child-inline.html
editing/style/make-text-writing-direction-inline-win.html
fast/text/bidi-reverse-runs-crash.html

[Build Bot]

Created attachment 264663 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 264658 [details]
Needs tests

Attachment 264658 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/375133

New failing tests:
fast/text/international/bidi-LDB-2-HTML.html
fast/text/bidi-embedding-pop-and-push-same.html
fast/text/international/bidi-ignored-for-first-child-inline.html
fast/css/absolute-inline-alignment-2.html
fast/css/default-bidi-css-rules.html
fast/text/international/iso-8859-8.html
editing/selection/move-by-word-visually-single-space-inline-element.html
editing/selection/move-by-word-visually-mac.html
editing/style/make-text-writing-direction-inline-mac.html
editing/style/make-text-writing-direction-inline-win.html
fast/text/bidi-reverse-runs-crash.html

[Build Bot]

Created attachment 264664 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 264658 [details]
Needs tests

Attachment 264658 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/375131

New failing tests:
fast/text/international/bidi-LDB-2-HTML.html
fast/text/bidi-embedding-pop-and-push-same.html
editing/style/make-text-writing-direction-inline-mac.html
fast/css/absolute-inline-alignment-2.html
fast/css/default-bidi-css-rules.html
fast/text/international/iso-8859-8.html
editing/selection/move-by-word-visually-single-space-inline-element.html
editing/selection/move-by-word-visually-mac.html
fast/text/international/bidi-ignored-for-first-child-inline.html
editing/style/make-text-writing-direction-inline-win.html
fast/text/bidi-reverse-runs-crash.html

[Build Bot]

Created attachment 264665 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Myles C. Maxfield]

Richard,

I'm actually not super sure if these tests are indicative of real failure, or if the results should simply be rebaselined. Do you think we could go through them together to see if this is the right change?

Thanks,
Myles

[r12a]

Myles, i'd be happy to help if i can.  I assume you're talking about the tests in comment 12?  Unfortunately, i'm not familiar with the test format, etc. Would you like to discuss offline?

[Myles C. Maxfield]

(In reply to comment #15)
> Myles, i'd be happy to help if i can.  I assume you're talking about the
> tests in comment 12?  Unfortunately, i'm not familiar with the test format,
> etc. Would you like to discuss offline?

Thanks! Do you think we should discuss this on IRC? When would you be available? (Also, which server / channel would be best for you?)

[Myles C. Maxfield]

Created attachment 264891 [details]
Screenshots

[Myles C. Maxfield]

(In reply to comment #16)
> (In reply to comment #15)
> > Myles, i'd be happy to help if i can.  I assume you're talking about the
> > tests in comment 12?  Unfortunately, i'm not familiar with the test format,
> > etc. Would you like to discuss offline?
> 
> Thanks! Do you think we should discuss this on IRC? When would you be
> available? (Also, which server / channel would be best for you?)

I've just attached a zip archive of a collection of screenshots; half with the patch, and half without (so we can compare them). The raw HTML sources can be found under http://trac.webkit.org/browser/trunk/LayoutTests . Hopefully we can go through these screenshots together to determine if the differences are expected (and a progression, rather than a regression).

There are some other tests which are more difficult to do this sort of analysis on; I figure that once we get through this set we can tackle the remaining tests.

[Myles C. Maxfield]

See also https://bugs.webkit.org/show_bug.cgi?id=63903

[r12a]

i reviewed the test results for Myles and sent comments directly to him.

[Noam Rosenthal]

Created attachment 400324 [details]
Patch

[Noam Rosenthal]

Created attachment 400698 [details]
Patch

[Darin Adler]

Comment on attachment 400698 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400698&action=review

> Source/WebCore/html/HTMLElement.cpp:218
> -            if (isLTROrRTLIgnoringCase(value))
> +            auto directionOverridingTag = hasTagName(bdiTag) || hasTagName(bdoTag) || hasTagName(outputTag);
> +
> +            if (isLTROrRTLIgnoringCase(value)) {
>                  addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection, value);
> -            if (!hasTagName(bdiTag) && !hasTagName(bdoTag) && !hasTagName(outputTag))
> +                if (!directionOverridingTag)
> +                    addPropertyToPresentationAttributeStyle(style, CSSPropertyUnicodeBidi, CSSValueIsolate);
> +            } else if (!directionOverridingTag)
>                  addPropertyToPresentationAttributeStyle(style, CSSPropertyUnicodeBidi, CSSValueEmbed);

I’d like this better with a local variable for the default. Something like this:

    auto unicodeBidiValue = CSSValueEmbed;
    if (isLTROrRTLIgnoringCase(value)) {
        addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection, value);
        unicodeBidiValue = CSSValueIsolate;
    }
    if (!hasTagName(bdiTag) && !hasTagName(bdoTag) && !hasTagName(outputTag))
        addPropertyToPresentationAttributeStyle(style, CSSPropertyUnicodeBidi, unicodeBidiValue);

Something about this seems less repetitive to me.

> LayoutTests/ChangeLog:28
> +                Modified them to include that old default explicity in the test, as they test something else.

explicitly

> LayoutTests/fast/css/absolute-inline-alignment-2.html:1
> -<!DOCTYPE html>
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">

Why this change?

[Noam Rosenthal]

Created attachment 400785 [details]
Patch for landing

[Noam Rosenthal]

(In reply to Darin Adler from comment #23)
> Comment on attachment 400698 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400698&action=review
> 
> > Source/WebCore/html/HTMLElement.cpp:218
> > -            if (isLTROrRTLIgnoringCase(value))
> > +            auto directionOverridingTag = hasTagName(bdiTag) || hasTagName(bdoTag) || hasTagName(outputTag);
> > +
> > +            if (isLTROrRTLIgnoringCase(value)) {
> >                  addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection, value);
> > -            if (!hasTagName(bdiTag) && !hasTagName(bdoTag) && !hasTagName(outputTag))
> > +                if (!directionOverridingTag)
> > +                    addPropertyToPresentationAttributeStyle(style, CSSPropertyUnicodeBidi, CSSValueIsolate);
> > +            } else if (!directionOverridingTag)
> >                  addPropertyToPresentationAttributeStyle(style, CSSPropertyUnicodeBidi, CSSValueEmbed);
> 
> I’d like this better with a local variable for the default. Something like
> this:
> 
>     auto unicodeBidiValue = CSSValueEmbed;
>     if (isLTROrRTLIgnoringCase(value)) {
>         addPropertyToPresentationAttributeStyle(style, CSSPropertyDirection,
> value);
>         unicodeBidiValue = CSSValueIsolate;
>     }
>     if (!hasTagName(bdiTag) && !hasTagName(bdoTag) && !hasTagName(outputTag))
>         addPropertyToPresentationAttributeStyle(style,
> CSSPropertyUnicodeBidi, unicodeBidiValue);
> 
> Something about this seems less repetitive to me.
Yea, that's better... applied

> 
> > LayoutTests/ChangeLog:28
> > +                Modified them to include that old default explicity in the test, as they test something else.
> 
> explicitly
Fixed

> 
> > LayoutTests/fast/css/absolute-inline-alignment-2.html:1
> > -<!DOCTYPE html>
> > +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">
> 
> Why this change?
No reason, removed.

[EWS]

Committed r262406: <https://trac.webkit.org/changeset/262406>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400785 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63859940>