Bug 43989 - There is no test for unicode-bidi / direction support in ApplyStyleCommand::applyInlineStyle
Summary: There is no test for unicode-bidi / direction support in ApplyStyleCommand::a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on: 44371
Blocks: 44359 44458
  Show dependency treegraph
 
Reported: 2010-08-13 13:03 PDT by Ryosuke Niwa
Modified: 2010-08-23 15:01 PDT (History)
9 users (show)

See Also:


Attachments
adds a script test (8.02 KB, patch)
2010-08-19 17:18 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
added nested case and expected results (13.77 KB, patch)
2010-08-19 17:53 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
fixed per Tony's comment (13.77 KB, patch)
2010-08-19 19:08 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
removed chinese characters (14.20 KB, patch)
2010-08-21 12:47 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
added back Chinese chracters (15.35 KB, patch)
2010-08-23 12:35 PDT, Ryosuke Niwa
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2010-08-13 13:03:01 PDT
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.
Comment 1 Ryosuke Niwa 2010-08-19 17:18:45 PDT
Created attachment 64916 [details]
adds a script test
Comment 2 Ryosuke Niwa 2010-08-19 17:22:49 PDT
(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.
Comment 3 Evan Martin 2010-08-19 17:35:30 PDT
+<!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>
Comment 4 Ryosuke Niwa 2010-08-19 17:53:05 PDT
Created attachment 64919 [details]
added nested case and expected results
Comment 5 Tony Chang 2010-08-19 18:47:33 PDT
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 +
Comment 6 Ryosuke Niwa 2010-08-19 19:06:09 PDT
(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.
Comment 7 Ryosuke Niwa 2010-08-19 19:08:55 PDT
Created attachment 64923 [details]
fixed per Tony's comment
Comment 8 Tony Chang 2010-08-20 08:36:38 PDT
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.
Comment 9 Ryosuke Niwa 2010-08-20 15:14:49 PDT
Committed r65757: <http://trac.webkit.org/changeset/65757>
Comment 10 Ryosuke Niwa 2010-08-20 16:51:46 PDT
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.
Comment 11 Ryosuke Niwa 2010-08-21 12:47:24 PDT
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.
Comment 12 Tony Chang 2010-08-23 09:14:11 PDT
(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?
Comment 13 Ryosuke Niwa 2010-08-23 12:35:35 PDT
Created attachment 65151 [details]
added back Chinese chracters
Comment 14 Ryosuke Niwa 2010-08-23 13:20:35 PDT
Committed r65828: <http://trac.webkit.org/changeset/65828>