Bug 45020 – Support "block-flow" and "writing-mode": interpret properties into RenderStyle

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Takumi Takano]

This is a subtask bug of <https://bugs.webkit.org/show_bug.cgi?id=45019>.

As the first step, add code that interprets "block-flow" property and "writing-mode" property settings into a RenderStyle object.

[Takumi Takano]

Created attachment 66174 [details]
Proposed patch file

[WebKit Review Bot]

Attachment 66174 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/style/RenderStyle.h:181:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/style/RenderStyle.h:210:  _block_flow is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adele Peterson]

Comment on attachment 66174 [details]
Proposed patch file

Its not clear to me whether the style warnings are important here.  I guess _block_flow should be _blockFlow, but the other warning goes against the existing style here, so I think we should leave it as is. This generally seems ok to me, but I'd like Dan or Hyatt to give it the once-over.

[Dave Hyatt]

Comment on attachment 66174 [details]
Proposed patch file

This patch is missing the "bt" value for the block-flow property.  I only see "tb", "rl" and "lr".

I'd prefer descriptive names for the block flow direction rather than the abbreviations.  Instead of BFLR, BFLR, BFTB, I'd prefer:

LeftToRightBlockFlow, RightToLeftBlockFlow, TopToBottomBlockFlow, BottomToTopBlockFlow

I would go ahead and add computed style support (see CSSComputedStyleDeclaration) for the new properties to this patch, since that will enable you to have actual tests that verify that the values are being parsed and inherited correctly.

[Takumi Takano]

Created attachment 66604 [details]
Modified patch. Incorporated feedback from Adele and Dave.

[WebKit Review Bot]

Attachment 66604 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/style/RenderStyle.h:181:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/style/RenderStyle.h:210:  _blockFlow is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

(In reply to comment #5)
> Created an attachment (id=66604) [details]
> Modified patch. Incorporated feedback from Adele and Dave.

It seems like this patch doesn't have any tests. Is that because this doesn't implement the feature but just adds support to the parser?  As hyatt suggested, I think you can still test the parser by calling set property value and getting computed style back.  Also, why are we only supporting -webkit-writing-mode and not writing-mode?  Is that a standard procedure when we add a support for new feature?

[Adele Peterson]

Its standard procedure if a property is in a draft specification and is likely to change in the future.

(In reply to comment #7)
> (In reply to comment #5)
> > Created an attachment (id=66604) [details] [details]
> > Modified patch. Incorporated feedback from Adele and Dave.
> 
> It seems like this patch doesn't have any tests. Is that because this doesn't implement the feature but just adds support to the parser?  As hyatt suggested, I think you can still test the parser by calling set property value and getting computed style back.  Also, why are we only supporting -webkit-writing-mode and not writing-mode?  Is that a standard procedure when we add a support for new feature?

[Dave Hyatt]

Comment on attachment 66604 [details]
Modified patch. Incorporated feedback from Adele and Dave.

(1) You need to move all of the value keywords for writing-mode and block-flow out of SVGCSSValueKeywords.in, so that your code will compile even when ENABLE(SVG) is false.  I'm not sure what some of them are doing in that file anyway.  Just move them all over to the main CSS keywords file.

(2) You need to patch the RenderStyle::diff() method to make sure changes in block-flow trigger a layout.

(3) I'd still like to see computed style support added (CSSComputedStyleDeclaration.cpp).

(4) Once you add computed style support in (3), you can add some layout tests to this patch as well.  I'd suggest fast/text/vertical as a good place for new tests.

[Adele Peterson]

You can read some general info about creating tests here: http://webkit.org/quality/testwriting.html

[Takumi Takano]

Created attachment 66881 [details]
Another modified patch

Incorporated Dave's feedback. Added a test to check if the parser correctly write/retrieve block-flow values to/from RenderStyle object.

[WebKit Review Bot]

Attachment 66881 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/style/RenderStyle.h:181:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/style/RenderStyle.h:210:  _blockFlow is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Hyatt]

Comment on attachment 66881 [details]
Another modified patch

r=me

[Dave Hyatt]

Keep in mind that all the computed style tests will start failing when you land this, so you'll need to fix those as well.  There are 3-4 of them, and they'll be really obvious if you just run-webkit-tests before landing.

[Dave Hyatt]

Comment on attachment 66881 [details]
Another modified patch

Changing this to a minus, since the patch needs the revised results for the various computed style tests before it can be landed via commit-queue.

[Takumi Takano]

Created attachment 67659 [details]
New patch file: incorporated Dave's feedback on computed style tests

Incorporated Dave's feedback on computed style tests. I revised expected result files at the top level of LayoutTests directory but haven't changed results in each platform. Is this okay?

[WebKit Review Bot]

Attachment 67659 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/rendering/style/RenderStyle.h:181:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/style/RenderStyle.h:210:  _blockFlow is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kent Tamura]

(In reply to comment #16)
> I revised expected result files at the top level of LayoutTests directory but haven't changed results in each platform. Is this okay?

It should be ok.  These test results are platform-neutral.

> Index: LayoutTests/fast/text/international/block-flow-parser-test.html
> ===================================================================
> --- LayoutTests/fast/text/international/block-flow-parser-test.html	(revision 0)
> +++ LayoutTests/fast/text/international/block-flow-parser-test.html	(revision 0)
> @@ -0,0 +1,39 @@
> +<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" 
> +"http://www.w3.org/TR/html4/loose.dtd">
> +<html>
> +<head>
> +<title>block-flow parser test</title>
> +<script>
> +function print(message)
> +{
> +    var paragraph = document.createElement("li");
> +    paragraph.appendChild(document.createTextNode(message));
> +    document.getElementById("console").appendChild(paragraph);
> +}

We have a lot of helper functions in LayoutTests/fast/js/resources/js-test-pre.js.  You may use it in this test.

[Dave Hyatt]

Comment on attachment 67659 [details]
New patch file: incorporated Dave's feedback on computed style tests

r=me

[Dave Hyatt]

Landed in r67667.

[WebKit Review Bot]

http://trac.webkit.org/changeset/67667 might have broken SnowLeopard Intel Release (Tests)

[Eric Seidel (no email)]

This resulted in a bunch of layout test failures:

http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r67673%20(17683)/results.html

[Takumi Takano]

Oops, this is a side effect of moving svg writing-mode values to standard css value definition. Working on...

(In reply to comment #22)
> This resulted in a bunch of layout test failures:
> 
> http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r67673%20(17683)/results.html

[Takumi Takano]

Created attachment 67881 [details]
Patch to fix the above SVG test regression

[Kent Tamura]

Comment on attachment 67881 [details]
Patch to fix the above SVG test regression

The patch looks good.  I'll confirm the patch fixes the tests and commit it.

[Kent Tamura]

Comment on attachment 67881 [details]
Patch to fix the above SVG test regression

Landed as r67695.

[John Gregg]

*** Bug 45947 has been marked as a duplicate of this bug. ***