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.
Created attachment 66174 [details] Proposed patch file
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.
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.
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.
Created attachment 66604 [details] Modified patch. Incorporated feedback from Adele and Dave.
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.
(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?
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?
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.
You can read some general info about creating tests here: http://webkit.org/quality/testwriting.html
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.
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.
Comment on attachment 66881 [details] Another modified patch r=me
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.
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.
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?
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.
(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.
Comment on attachment 67659 [details] New patch file: incorporated Dave's feedback on computed style tests r=me
Landed in r67667.
http://trac.webkit.org/changeset/67667 might have broken SnowLeopard Intel Release (Tests)
This resulted in a bunch of layout test failures: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r67673%20(17683)/results.html
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
Created attachment 67881 [details] Patch to fix the above SVG test regression
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.
Comment on attachment 67881 [details] Patch to fix the above SVG test regression Landed as r67695.
*** Bug 45947 has been marked as a duplicate of this bug. ***