Bug 43989

Summary:

There is no test for unicode-bidi / direction support in ApplyStyleCommand::applyInlineStyle

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

HTML Editing

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Major

CC:

aharon, darin, enrica, eric, evan, mitz, ojan, tkent, tony

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

44371

Bug Blocks:

44359, 44458

Attachments:

Description	Flags
adds a script test	none
added nested case and expected results	none
fixed per Tony's comment	none
removed chinese characters	none
added back Chinese chracters	tony: review+

Ryosuke Niwa

Reported 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.

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2010-08-19 17:18:45 PDT

Created attachment 64916 [details] adds a script test

Ryosuke Niwa

Comment 2 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.

Evan Martin

Comment 3 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>

Ryosuke Niwa

Comment 4 2010-08-19 17:53:05 PDT

Created attachment 64919 [details] added nested case and expected results

Tony Chang

Comment 5 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 "hello world webkit rocks" yielded hello world webkit rocks but expected hello world webkit rocks > +FAIL LeftToRight on third word of "hello world webkit rocks" yielded hello world webkit rocks but expected hello world webkit rocks 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 +

Ryosuke Niwa

Comment 6 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 "hello world webkit rocks" yielded hello world webkit rocks but expected hello world webkit rocks > > +FAIL LeftToRight on third word of "hello world webkit rocks" yielded hello world webkit rocks but expected hello world webkit rocks > > 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.

Ryosuke Niwa

Comment 7 2010-08-19 19:08:55 PDT

Created attachment 64923 [details] fixed per Tony's comment

Tony Chang

Comment 8 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.

Ryosuke Niwa

Comment 9 2010-08-20 15:14:49 PDT

Committed r65757: <http://trac.webkit.org/changeset/65757>

Ryosuke Niwa

Comment 10 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.

Ryosuke Niwa

Comment 11 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.

Tony Chang

Comment 12 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?

Ryosuke Niwa

Comment 13 2010-08-23 12:35:35 PDT

Created attachment 65151 [details] added back Chinese chracters

Ryosuke Niwa

Comment 14 2010-08-23 13:20:35 PDT

Committed r65828: <http://trac.webkit.org/changeset/65828>

Note You need to log in before you can comment on or make changes to this bug.