Bug 134630 - Make unicode-bidi:isolate the default for an element with a dir attribute (instead of unicode-bidi:embed)
Summary: Make unicode-bidi:isolate the default for an element with a dir attribute (in...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Major
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar, WebExposed
Depends on:
Blocks:
 
Reported: 2014-07-04 04:22 PDT by r12a
Modified: 2020-06-01 22:50 PDT (History)
16 users (show)

See Also:


Attachments
Needs tests (1.69 KB, patch)
2015-11-02 20:31 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (1.16 MB, application/zip)
2015-11-02 21:16 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (1.22 MB, application/zip)
2015-11-02 21:20 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (1.28 MB, application/zip)
2015-11-02 21:22 PST, Build Bot
no flags Details
Screenshots (1.78 MB, application/zip)
2015-11-05 15:01 PST, Myles C. Maxfield
no flags Details
Patch (4.86 KB, patch)
2020-05-27 05:16 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (23.48 KB, patch)
2020-05-31 04:24 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch for landing (23.28 KB, patch)
2020-06-01 22:24 PDT, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description r12a 2014-07-04 04:22:48 PDT
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.)
Comment 1 r12a 2014-07-18 04:49:22 PDT
Anyone looking at this?
Comment 2 Myles C. Maxfield 2015-11-02 19:27:14 PST
It looks like HTMLElement::collectStyleForPresentationAttribute() is relevant.
Comment 3 Myles C. Maxfield 2015-11-02 20:31:30 PST
Created attachment 264658 [details]
Needs tests
Comment 4 WebKit Commit Bot 2015-11-02 20:34:36 PST
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.
Comment 5 r12a 2015-11-02 20:41:50 PST
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.
Comment 7 Myles C. Maxfield 2015-11-02 20:45:07 PST
Also see https://bugs.webkit.org/show_bug.cgi?id=149170
Comment 8 Build Bot 2015-11-02 21:16:47 PST
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
Comment 9 Build Bot 2015-11-02 21:16:50 PST
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
Comment 10 Build Bot 2015-11-02 21:20:48 PST
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
Comment 11 Build Bot 2015-11-02 21:20:52 PST
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
Comment 12 Build Bot 2015-11-02 21:22:17 PST
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
Comment 13 Build Bot 2015-11-02 21:22:21 PST
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
Comment 14 Myles C. Maxfield 2015-11-03 13:43:17 PST
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
Comment 15 r12a 2015-11-04 04:38:16 PST
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?
Comment 16 Myles C. Maxfield 2015-11-05 11:05:52 PST
(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?)
Comment 17 Myles C. Maxfield 2015-11-05 15:01:08 PST
Created attachment 264891 [details]
Screenshots
Comment 18 Myles C. Maxfield 2015-11-05 15:04:27 PST
(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.
Comment 19 Myles C. Maxfield 2015-11-06 17:26:42 PST
See also https://bugs.webkit.org/show_bug.cgi?id=63903
Comment 20 r12a 2015-11-18 10:03:25 PST
i reviewed the test results for Myles and sent comments directly to him.
Comment 21 Noam Rosenthal 2020-05-27 05:16:21 PDT
Created attachment 400324 [details]
Patch
Comment 22 Noam Rosenthal 2020-05-31 04:24:05 PDT
Created attachment 400698 [details]
Patch
Comment 23 Darin Adler 2020-06-01 13:01:32 PDT
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?
Comment 24 Noam Rosenthal 2020-06-01 22:24:16 PDT
Created attachment 400785 [details]
Patch for landing
Comment 25 Noam Rosenthal 2020-06-01 22:24:52 PDT
(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.
Comment 26 EWS 2020-06-01 22:49:51 PDT
Committed r262406: <https://trac.webkit.org/changeset/262406>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400785 [details].
Comment 27 Radar WebKit Bug Importer 2020-06-01 22:50:19 PDT
<rdar://problem/63859940>