LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html - Issue 17155007: [CSS3] Parsing the property, text-justify.

Side by Side Diff: LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify.html

Issue 17155007: [CSS3] Parsing the property, text-justify. (Closed) Base URL: https://chromium.googlesource.com/chromium/blink.git@parsingTextJustify

Patch Set: Created 7 years, 5 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« no previous file with comments | « no previous file | LayoutTests/fast/css3-text/css3-text-justify/getComputedStyle/getComputedStyle-text-justify-expected.txt » ('j') | Source/core/css/CSSParser-in.cpp » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <html>

	3 <head>

	4 <script src="../../../js/resources/js-test-pre.js"></script>

	5 </head>

	6 <body>

	7 <div id="test">hello world</div>

	8 <div id="ancestor"><div id="child">inherit test</div></div>

	9 <script>

	10 function testElementStyle(value)

	11 {

	12 shouldBe("e.style.textJustify", "'" + value + "'");

	13 shouldBe("e.style.getPropertyCSSValue('text-justify').cssText", "'" + valu e + "'");

	14 }

	15

	16 function testComputedStyle(value)

	17 {

	18 computedStyle = window.getComputedStyle(e, null);

	19 shouldBe("computedStyle.textJustify", "'" + value + "'");

	20 shouldBe("computedStyle.getPropertyCSSValue('text-justify').cssText", "'" + value + "'");

	21 }

	22

	23 function valueSettingTest(value)

	24 {

	25 debug("Value '" + value + "':");

	26 e.style.textJustify = value;

	27 testElementStyle(value);

	28 testComputedStyle(value);

	29 debug('');

	30 }

	31

	32 function invalidValueSettingTest(value, defaultValue)
	Julien - ping for review 2013/07/17 01:51:10 Do we really need \|defaultValue\| as it's always au Do we really need \|defaultValue\| as it's always auto? dw.im 2013/07/17 02:03:15 hmm... no. we don't. Show quoted text On 2013/07/17 01:51:10, Julien Chaffraix wrote: > Do we really need \|defaultValue\| as it's always auto? hmm... no. we don't.
	33 {

	34 debug("Invalid value test - '" + value + "':");

	35 e.style.textJustify = value;

	36 testElementStyle(defaultValue);

	37 testComputedStyle(defaultValue);

	38 debug('');

	39 }

	40

	41 function testElementStyle_inherit(value)

	42 {

	43 shouldBe("child.style.textJustify", "'" + value + "'");

	44 shouldBe("child.style.getPropertyCSSValue('text-justify').cssText", "'" + value + "'");

	45 }

	46

	47 function testComputedStyle_inherit(a_value, c_value)

	48 {

	49 shouldBe("window.getComputedStyle(ancestor).getPropertyCSSValue('text-just ify').cssText", "'" + a_value + "'");

	50 shouldBe("window.getComputedStyle(child).getPropertyCSSValue('text-justify ').cssText", "'" + c_value + "'");
	Julien - ping for review 2013/07/17 01:51:10 Any reason this doesn't check also test e.style? A Any reason this doesn't check also test e.style? As a rule you should be consistent across your tests.
	51 debug('');

	52 }

	53

	54 function ownValueTest(a_value, c_value)
	Julien - ping for review 2013/07/17 01:51:10 let's use WebKit style for variable instead of c_s let's use WebKit style for variable instead of c_style_variable_that_do_not_match_our_style. Also don't shorten the names, a_value is not very clear, ancestorValue is way better.
	55 {

	56 debug("Value of ancestor is '" + a_value + ", while child is '" + c_value + "':");

	57 ancestor.style.textJustify = a_value;

	58 child.style.textJustify = c_value;

	59 testComputedStyle_inherit(a_value, c_value);

	60 }

	61

	62 function inheritanceTest(a_value)

	63 {

	64 debug("Value of ancestor is '" + a_value + "':");

	65 ancestor.style.textJustify = a_value;

	66 testComputedStyle_inherit(a_value, a_value);
	Julien - ping for review 2013/07/17 01:51:10 Why 2 arguments when the function only takes one? Why 2 arguments when the function only takes one?
	67 }

	68

	69 function computedValueSettingTest(value, defaultValue)

	70 {

	71 debug("Computed value test - '" + value + "':");

	72 ancestor.style.textJustify = defaultValue;

	73 e.style.textJustify = value;

	74 testElementStyle(value);

	75 testComputedStyle(defaultValue);

	76 debug('');

	77 }

	78

	79 description("This test checks that text-justify parses properly the properties from CSS 3 Text.");

	80

	81 e = document.getElementById('test');

	82 ancestor = document.getElementById('ancestor');

	83 child = document.getElementById('child');

	84

	85 debug("Test the initial value:");

	86 testComputedStyle('auto');
	Julien - ping for review 2013/07/17 01:51:10 Why is this only testing getComputedStyle and not Why is this only testing getComputedStyle and not style? dw.im 2013/07/17 02:03:15 I will add one line of code to test it. Show quoted text On 2013/07/17 01:51:10, Julien Chaffraix wrote: > Why is this only testing getComputedStyle and not style? I will add one line of code to test it.
	87 debug('');

	88

	89 valueSettingTest('auto');
	Julien - ping for review 2013/07/17 01:51:10 This tests nothing as your default value is 'auto' This tests nothing as your default value is 'auto'. dw.im 2013/07/17 02:03:15 valueSettingTest is set & test, so, set as 'auto' Show quoted text On 2013/07/17 01:51:10, Julien Chaffraix wrote: > This tests nothing as your default value is 'auto'. valueSettingTest is set & test, so, set as 'auto', and check if the value is setted ad 'auto'. Julien - ping for review 2013/07/17 17:35:25 Right, this is what the code does but it is actual Show quoted text On 2013/07/17 02:03:15, dw.im wrote: > On 2013/07/17 01:51:10, Julien Chaffraix wrote: > > This tests nothing as your default value is 'auto'. > > valueSettingTest is set & test, > so, set as 'auto', and check if the value is setted ad 'auto'. Right, this is what the code does but it is actually not testing ANYTHING. You are missing the fact that the value is already set to 'auto' (from the previous test / initial value): - if we don't handle 'auto' properly during parsing, we will carry the original value of 'auto' - if we handle it properly, the output would be 'auto' Now because we have no way to know which of this case applies, this test doesn't give any meaningful information.
	90 valueSettingTest('none');

	91 valueSettingTest('inter-word');

	92 valueSettingTest('distribute');

	93

	94 defaultValue = 'auto'

	95 e.style.textJustify = defaultValue;

	96 invalidValueSettingTest('-webkit-left', defaultValue);

	97 invalidValueSettingTest('-webkit-right', defaultValue);
	Julien - ping for review 2013/07/17 01:51:10 Can we avoid undocumented, unstandardized and pref Can we avoid undocumented, unstandardized and prefixed values? dw.im 2013/07/17 02:03:15 oh! sure. I will use another one. I used these val Show quoted text On 2013/07/17 01:51:10, Julien Chaffraix wrote: > Can we avoid undocumented, unstandardized and prefixed values? oh! sure. I will use another one. I used these values because it make me feel these values are really invalid.
	98 invalidValueSettingTest('solid', defaultValue);

	99 invalidValueSettingTest('normal', defaultValue);

	100

	101 inheritanceTest("auto");

	102 inheritanceTest("none");

	103 inheritanceTest("inter-word");

	104 inheritanceTest("distribute");

	105

	106 ownValueTest("inter-word", "distribute");

	107 ownValueTest("none", "inter-word");

	108

	109 e = document.getElementById('child');

	110 computedValueSettingTest('initial', 'auto');
	Julien - ping for review 2013/07/17 01:51:10 This testing is very much based on the previous on This testing is very much based on the previous one passing which is a bit annoying. If we regress and return 'auto' from the previous tests, this will automatically pass. That's a general comment about re-using the test fixture for all the tests.
	111 computedValueSettingTest('inherit', 'none');

	112 computedValueSettingTest('inherit', 'distribute');

	113 </script>

	114 <script src="../../../js/resources/js-test-post.js"></script>

	115 </body>

	116

	117 </html>

OLD	NEW