WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
134630
Make unicode-bidi:isolate the default for an element with a dir attribute (instead of unicode-bidi:embed)
https://bugs.webkit.org/show_bug.cgi?id=134630
Summary
Make unicode-bidi:isolate the default for an element with a dir attribute (in...
r12a
Reported
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.)
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
r12a
Comment 1
2014-07-18 04:49:22 PDT
Anyone looking at this?
Myles C. Maxfield
Comment 2
2015-11-02 19:27:14 PST
It looks like HTMLElement::collectStyleForPresentationAttribute() is relevant.
Myles C. Maxfield
Comment 3
2015-11-02 20:31:30 PST
Created
attachment 264658
[details]
Needs tests
WebKit Commit Bot
Comment 4
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.
r12a
Comment 5
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.
Myles C. Maxfield
Comment 6
2015-11-02 20:43:08 PST
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
Comment 7
2015-11-02 20:45:07 PST
Also see
https://bugs.webkit.org/show_bug.cgi?id=149170
Build Bot
Comment 8
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
Build Bot
Comment 9
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
Build Bot
Comment 10
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
Build Bot
Comment 11
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
Build Bot
Comment 12
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
Build Bot
Comment 13
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
Myles C. Maxfield
Comment 14
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
r12a
Comment 15
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?
Myles C. Maxfield
Comment 16
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?)
Myles C. Maxfield
Comment 17
2015-11-05 15:01:08 PST
Created
attachment 264891
[details]
Screenshots
Myles C. Maxfield
Comment 18
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.
Myles C. Maxfield
Comment 19
2015-11-06 17:26:42 PST
See also
https://bugs.webkit.org/show_bug.cgi?id=63903
r12a
Comment 20
2015-11-18 10:03:25 PST
i reviewed the test results for Myles and sent comments directly to him.
Noam Rosenthal
Comment 21
2020-05-27 05:16:21 PDT
Created
attachment 400324
[details]
Patch
Noam Rosenthal
Comment 22
2020-05-31 04:24:05 PDT
Created
attachment 400698
[details]
Patch
Darin Adler
Comment 23
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?
Noam Rosenthal
Comment 24
2020-06-01 22:24:16 PDT
Created
attachment 400785
[details]
Patch for landing
Noam Rosenthal
Comment 25
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.
EWS
Comment 26
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]
.
Radar WebKit Bug Importer
Comment 27
2020-06-01 22:50:19 PDT
<
rdar://problem/63859940
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug