While I was cleaning up the code in ApplyStyleCommand::applyInlineStyle, I found out that unicode-bidi was never present in all of our editing test. getIdentifierValue(style, CSSPropertyUnicodeBidi) always returns 0. We definitely need some testing here.
Created attachment 64916 [details] adds a script test
(In reply to comment #1) > Created an attachment (id=64916) [details] > adds a script test The attachment is in UTF-8 so please manually select the encoding if arabic / chinese characters didn't show up correctly. Also please let me know if any of the expectations are wrong.
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> +<html> +<head> +<meta http-equiv="Content-Type" content="text/html; charset=utf-8"> FYI, HTML5 allows: <!DOCTYPE html> and <meta charset=utf-8>
Created attachment 64919 [details] added nested case and expected results
Comment on attachment 64919 [details] added nested case and expected results > Index: LayoutTests/editing/style/make-text-writing-direction-inline-expected.txt > +FAIL Natural on third word of "<span dir="rtl">hello <span dir="ltr">world webkit rocks</span></span>" yielded <span dir="rtl">hello <span dir="ltr">world</span></span><span><span> webkit</span></span><span dir="rtl"><span dir="ltr"> rocks</span></span> but expected <span dir="rtl">hello <span dir="ltr">world</span></span> webkit<span dir="rtl"><span dir="ltr"> rocks</span></span> > +FAIL LeftToRight on third word of "<span dir="rtl">hello <span dir="ltr">world webkit rocks</span></span>" yielded <span dir="rtl">hello <span dir="ltr">world</span></span><span><span><span style="unicode-bidi: embed;"> webkit</span></span></span><span dir="rtl"><span dir="ltr"> rocks</span></span> but expected <span dir="rtl">hello <span dir="ltr">world</span></span><span style="unicode-bidi: embed;"> webkit</span><span dir="rtl"><span dir="ltr"> rocks</span></span> These tests are expected to fail? Can you include a link to a bug for these failing tests? > Index: LayoutTests/editing/style/script-tests/make-text-writing-direction-inline.js > +function modifyWritingDirection(content, selector, command, expected) > +{ > + if (!window.layoutTestController) > + return; Is this redundant with the earlier check (line 3)? > + var selected = selector(testContainer); > + window.layoutTestController.execCommand('MakeTextWritingDirection'+command); Nit: Spaces around the +
(In reply to comment #5) > (From update of attachment 64919 [details]) > > Index: LayoutTests/editing/style/make-text-writing-direction-inline-expected.txt > > +FAIL Natural on third word of "<span dir="rtl">hello <span dir="ltr">world webkit rocks</span></span>" yielded <span dir="rtl">hello <span dir="ltr">world</span></span><span><span> webkit</span></span><span dir="rtl"><span dir="ltr"> rocks</span></span> but expected <span dir="rtl">hello <span dir="ltr">world</span></span> webkit<span dir="rtl"><span dir="ltr"> rocks</span></span> > > +FAIL LeftToRight on third word of "<span dir="rtl">hello <span dir="ltr">world webkit rocks</span></span>" yielded <span dir="rtl">hello <span dir="ltr">world</span></span><span><span><span style="unicode-bidi: embed;"> webkit</span></span></span><span dir="rtl"><span dir="ltr"> rocks</span></span> but expected <span dir="rtl">hello <span dir="ltr">world</span></span><span style="unicode-bidi: embed;"> webkit</span><span dir="rtl"><span dir="ltr"> rocks</span></span> > > These tests are expected to fail? Can you include a link to a bug for these failing tests? There's no bug filed right now but there are extra spans in the actual result, which we should be removing. > > Index: LayoutTests/editing/style/script-tests/make-text-writing-direction-inline.js > > +function modifyWritingDirection(content, selector, command, expected) > > +{ > > + if (!window.layoutTestController) > > + return; > > Is this redundant with the earlier check (line 3)? No. If I didn't exit there, it'll cause error when I call execCommand on layoutTestController. Notice that testFailed does not exit the test. > > + var selected = selector(testContainer); > > + window.layoutTestController.execCommand('MakeTextWritingDirection'+command); > > Nit: Spaces around the + Will fix.
Created attachment 64923 [details] fixed per Tony's comment
Comment on attachment 64923 [details] fixed per Tony's comment (In reply to comment #6) > (In reply to comment #5) > > These tests are expected to fail? Can you include a link to a bug for these failing tests? > > There's no bug filed right now but there are extra spans in the actual result, which we should be removing. Can you file a bug about these failures and add a comment to the test that says that these are expected to fail because of <link to new bug>? r=me with that change.
Committed r65757: <http://trac.webkit.org/changeset/65757>
Because of https://bugs.webkit.org/show_bug.cgi?id=43989, rolled out the patch in http://trac.webkit.org/changeset/65762. We need to remove Chinese characters in order to commit.
Created attachment 65032 [details] removed chinese characters It seems like Chromium Windows or possibly entire Windows platform have a different behavior on selection.modfy on Chinese characters. Testing on ltr and rtl texts should suffice ensuring the behavior of MakeTextWritingDirection. If the behaviral difference on Chinese chracters is not what we expect, then we should file a separate bug and add a test but that's beyond the scope of this bug.
(In reply to comment #10) > Because of https://bugs.webkit.org/show_bug.cgi?id=43989, rolled out the patch in http://trac.webkit.org/changeset/65762. Did you mean to link to this bug? I'm not sure I understand why the tests failed on Windows. > We need to remove Chinese characters in order to commit. If the behavioral differences are expected, can we just add back the tests and add chromium-win specific results?
Created attachment 65151 [details] added back Chinese chracters
Committed r65828: <http://trac.webkit.org/changeset/65828>