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.)
Anyone looking at this?
It looks like HTMLElement::collectStyleForPresentationAttribute() is relevant.
Created attachment 264658 [details] Needs tests
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.
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.
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
Also see https://bugs.webkit.org/show_bug.cgi?id=149170
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
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 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
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 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
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
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
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?
(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?)
Created attachment 264891 [details] Screenshots
(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.
See also https://bugs.webkit.org/show_bug.cgi?id=63903
i reviewed the test results for Myles and sent comments directly to him.
Created attachment 400324 [details] Patch
Created attachment 400698 [details] Patch
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?
Created attachment 400785 [details] Patch for landing
(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.
Committed r262406: <https://trac.webkit.org/changeset/262406> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400785 [details].
<rdar://problem/63859940>