- Revision
- 139313
- Author
- ale...@webkit.org
- Date
- 2013-01-10 05:46:39 -0800 (Thu, 10 Jan 2013)
Log Message
ASSERT_NOT_REACHED in StylePropertySet::fontValue when accessing font style property through JS after setting style font size.
https://bugs.webkit.org/show_bug.cgi?id=88866
Reviewed by Alexander Pavlov.
Source/WebCore:
StylePropertySet::fontValue always assumed that it was called using
style.font after a subsequent call which set the shorthand font. The
ASSERT_NOT_REACHED assumed that all longhands of the font shorthand not
set by the shorthand itself were set to initial. While it's true when
we set the font shorthand (i.e all longhands are set to implicit initial)
it is not true when you set the longhands individually. For example setting
font-size will not set other font properties to initial. It is the behavior of all
other shorthands in WebKit. When reconstructing the shorthand other
properties tests whether the value of each longhands is initial or not
(if not then we omit the value, as we should always construct the
shortest shorthand possible) or if the value is set or not (if set then
we include it in the shorthand if not then we omit it). The comment
removed was also talking about invalid font property potentially built
by fontValue(). So far appendFontLonghandValueIfExplicit will always
construct a valid value as it takes care of adding ' ' or '/' when
needed, so the return value is parsable and correct.
Test: fast/css/font-shorthand-from-longhands.html
* css/StylePropertySet.cpp:
(WebCore::StylePropertySet::appendFontLonghandValueIfExplicit):
(WebCore::StylePropertySet::fontValue):
* css/StylePropertySet.h:
LayoutTests:
Add tests to cover the bug.
* fast/css/font-shorthand-from-longhands-expected.txt: Added.
* fast/css/font-shorthand-from-longhands.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (139312 => 139313)
--- trunk/LayoutTests/ChangeLog 2013-01-10 13:37:27 UTC (rev 139312)
+++ trunk/LayoutTests/ChangeLog 2013-01-10 13:46:39 UTC (rev 139313)
@@ -1,3 +1,15 @@
+2013-01-10 Alexis Menard <ale...@webkit.org>
+
+ ASSERT_NOT_REACHED in StylePropertySet::fontValue when accessing font style property through JS after setting style font size.
+ https://bugs.webkit.org/show_bug.cgi?id=88866
+
+ Reviewed by Alexander Pavlov.
+
+ Add tests to cover the bug.
+
+ * fast/css/font-shorthand-from-longhands-expected.txt: Added.
+ * fast/css/font-shorthand-from-longhands.html: Added.
+
2013-01-10 Dominik Röttsches <dominik.rottsc...@intel.com>
[EFL] Unreviewed gardening.
Added: trunk/LayoutTests/fast/css/font-shorthand-from-longhands-expected.txt (0 => 139313)
--- trunk/LayoutTests/fast/css/font-shorthand-from-longhands-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/css/font-shorthand-from-longhands-expected.txt 2013-01-10 13:46:39 UTC (rev 139313)
@@ -0,0 +1,45 @@
+Test the return values for the font properties on the style object.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS style.font is ''
+PASS computedStyle.font is 'normal normal normal 20px/normal foobar'
+PASS computedStyle.fontSize is '20px'
+PASS checkFontStyleValue() is true
+PASS style.font is '20px sans-serif'
+PASS computedStyle.font is 'normal normal normal 20px/normal sans-serif'
+PASS computedStyle.fontFamily is 'sans-serif'
+PASS checkFontStyleValue() is true
+PASS style.font is 'italic 20px sans-serif'
+PASS computedStyle.font is 'italic normal normal 20px/normal sans-serif'
+PASS computedStyle.fontStyle is 'italic'
+PASS checkFontStyleValue() is true
+PASS style.font is 'italic small-caps 20px sans-serif'
+PASS computedStyle.font is 'italic small-caps normal 20px/normal sans-serif'
+PASS computedStyle.fontVariant is 'small-caps'
+PASS checkFontStyleValue() is true
+PASS style.font is 'italic small-caps bold 20px sans-serif'
+PASS computedStyle.font is 'italic small-caps bold 20px/normal sans-serif'
+PASS computedStyle.fontWeight is 'bold'
+PASS checkFontStyleValue() is true
+PASS style.font is 'italic small-caps bold 20px/40px sans-serif'
+PASS computedStyle.font is 'italic small-caps bold 20px/40px sans-serif'
+PASS computedStyle.lineHeight is '40px'
+PASS checkFontStyleValue() is true
+PASS style.font is ''
+PASS computedStyle.font is 'normal normal normal 16px/normal foobar'
+PASS checkFontStyleValue() is true
+PASS style.font is ''
+PASS computedStyle.font is 'normal normal bold 16px/normal foobar'
+PASS computedStyle.fontWeight is 'bold'
+PASS checkFontStyleValue() is true
+PASS style.font is 'bold 40px sans-serif'
+PASS computedStyle.font is 'normal normal bold 40px/normal sans-serif'
+PASS computedStyle.fontSize is '40px'
+PASS computedStyle.fontFamily is 'sans-serif'
+PASS checkFontStyleValue() is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/css/font-shorthand-from-longhands.html (0 => 139313)
--- trunk/LayoutTests/fast/css/font-shorthand-from-longhands.html (rev 0)
+++ trunk/LayoutTests/fast/css/font-shorthand-from-longhands.html 2013-01-10 13:46:39 UTC (rev 139313)
@@ -0,0 +1,99 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+@font-face {
+ font-family: "foobar";
+ src: local("foobar");
+}
+div {
+ font-family: "foobar";
+}
+</style>
+<script src=""
+</head>
+<body>
+<script>
+description("Test the return values for the font properties on the style object.")
+
+var testContainer = document.createElement("div");
+document.body.appendChild(testContainer);
+
+testContainer.innerHTML = '<div id="test">hello</div>';
+
+var e = document.getElementById('test');
+var style = e.style;
+var computedStyle = window.getComputedStyle(e, null);
+
+// This function checks the return value of style.font and verifies WebKit can parse it.
+function checkFontStyleValue() {
+ var before = e.style.getPropertyValue('font');
+ e.style.font = '';
+ e.style.font = before;
+ return (e.style.getPropertyValue('font') === before);
+}
+
+style.fontSize = "20px";
+// We need at least the font-family to build the shorthand.
+shouldBe("style.font", "''");
+shouldBe("computedStyle.font", "'normal normal normal 20px/normal foobar'");
+shouldBe("computedStyle.fontSize", "'20px'");
+shouldBe("checkFontStyleValue()", "true");
+
+style.fontSize = "20px";
+style.fontFamily = "sans-serif";
+shouldBe("style.font", "'20px sans-serif'");
+shouldBe("computedStyle.font", "'normal normal normal 20px/normal sans-serif'");
+shouldBe("computedStyle.fontFamily", "'sans-serif'");
+shouldBe("checkFontStyleValue()", "true");
+
+style.fontStyle = "italic";
+shouldBe("style.font", "'italic 20px sans-serif'");
+shouldBe("computedStyle.font", "'italic normal normal 20px/normal sans-serif'");
+shouldBe("computedStyle.fontStyle", "'italic'");
+shouldBe("checkFontStyleValue()", "true");
+
+style.fontVariant = "small-caps";
+shouldBe("style.font", "'italic small-caps 20px sans-serif'");
+shouldBe("computedStyle.font", "'italic small-caps normal 20px/normal sans-serif'");
+shouldBe("computedStyle.fontVariant", "'small-caps'");
+shouldBe("checkFontStyleValue()", "true");
+
+style.fontWeight = "bold";
+shouldBe("style.font", "'italic small-caps bold 20px sans-serif'");
+shouldBe("computedStyle.font", "'italic small-caps bold 20px/normal sans-serif'");
+shouldBe("computedStyle.fontWeight", "'bold'");
+shouldBe("checkFontStyleValue()", "true");
+
+style.lineHeight = "40px";
+shouldBe("style.font", "'italic small-caps bold 20px/40px sans-serif'");
+shouldBe("computedStyle.font", "'italic small-caps bold 20px/40px sans-serif'");
+shouldBe("computedStyle.lineHeight", "'40px'");
+shouldBe("checkFontStyleValue()", "true");
+
+style.font = "";
+shouldBe("style.font", "''");
+shouldBe("computedStyle.font", "'normal normal normal 16px/normal foobar'");
+shouldBe("checkFontStyleValue()", "true");
+
+style.fontWeight = "bold";
+// It is normal to return null as the font-size is mandatory to build the shorthand.
+shouldBe("style.font", "''");
+shouldBe("computedStyle.font", "'normal normal bold 16px/normal foobar'");
+shouldBe("computedStyle.fontWeight", "'bold'");
+shouldBe("checkFontStyleValue()", "true");
+
+style.fontSize = "40px";
+style.fontFamily = "sans-serif";
+style.fontWeight = "bold";
+shouldBe("style.font", "'bold 40px sans-serif'");
+shouldBe("computedStyle.font", "'normal normal bold 40px/normal sans-serif'");
+shouldBe("computedStyle.fontSize", "'40px'");
+shouldBe("computedStyle.fontFamily", "'sans-serif'");
+shouldBe("checkFontStyleValue()", "true");
+
+document.body.removeChild(testContainer);
+</script>
+<script src=""
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (139312 => 139313)
--- trunk/Source/WebCore/ChangeLog 2013-01-10 13:37:27 UTC (rev 139312)
+++ trunk/Source/WebCore/ChangeLog 2013-01-10 13:46:39 UTC (rev 139313)
@@ -1,3 +1,34 @@
+2013-01-10 Alexis Menard <ale...@webkit.org>
+
+ ASSERT_NOT_REACHED in StylePropertySet::fontValue when accessing font style property through JS after setting style font size.
+ https://bugs.webkit.org/show_bug.cgi?id=88866
+
+ Reviewed by Alexander Pavlov.
+
+ StylePropertySet::fontValue always assumed that it was called using
+ style.font after a subsequent call which set the shorthand font. The
+ ASSERT_NOT_REACHED assumed that all longhands of the font shorthand not
+ set by the shorthand itself were set to initial. While it's true when
+ we set the font shorthand (i.e all longhands are set to implicit initial)
+ it is not true when you set the longhands individually. For example setting
+ font-size will not set other font properties to initial. It is the behavior of all
+ other shorthands in WebKit. When reconstructing the shorthand other
+ properties tests whether the value of each longhands is initial or not
+ (if not then we omit the value, as we should always construct the
+ shortest shorthand possible) or if the value is set or not (if set then
+ we include it in the shorthand if not then we omit it). The comment
+ removed was also talking about invalid font property potentially built
+ by fontValue(). So far appendFontLonghandValueIfExplicit will always
+ construct a valid value as it takes care of adding ' ' or '/' when
+ needed, so the return value is parsable and correct.
+
+ Test: fast/css/font-shorthand-from-longhands.html
+
+ * css/StylePropertySet.cpp:
+ (WebCore::StylePropertySet::appendFontLonghandValueIfExplicit):
+ (WebCore::StylePropertySet::fontValue):
+ * css/StylePropertySet.h:
+
2013-01-10 Sheriff Bot <webkit.review....@gmail.com>
Unreviewed, rolling out r139306.
Modified: trunk/Source/WebCore/css/StylePropertySet.cpp (139312 => 139313)
--- trunk/Source/WebCore/css/StylePropertySet.cpp 2013-01-10 13:37:27 UTC (rev 139312)
+++ trunk/Source/WebCore/css/StylePropertySet.cpp 2013-01-10 13:46:39 UTC (rev 139313)
@@ -225,15 +225,15 @@
return horizontalValueCSSText + ' ' + verticalValueCSSText;
}
-bool StylePropertySet::appendFontLonghandValueIfExplicit(CSSPropertyID propertyID, StringBuilder& result, String& commonValue) const
+void StylePropertySet::appendFontLonghandValueIfExplicit(CSSPropertyID propertyID, StringBuilder& result, String& commonValue) const
{
int foundPropertyIndex = findPropertyIndex(propertyID);
if (foundPropertyIndex == -1)
- return false; // All longhands must have at least implicit values if "font" is specified.
+ return; // All longhands must have at least implicit values if "font" is specified.
if (propertyAt(foundPropertyIndex).isImplicit()) {
commonValue = String();
- return true;
+ return;
}
char prefix = '\0';
@@ -258,37 +258,32 @@
result.append(value);
if (!commonValue.isNull() && commonValue != value)
commonValue = String();
-
- return true;
}
String StylePropertySet::fontValue() const
{
- int foundPropertyIndex = findPropertyIndex(CSSPropertyFontSize);
- if (foundPropertyIndex == -1)
+ int fontSizePropertyIndex = findPropertyIndex(CSSPropertyFontSize);
+ int fontFamilyPropertyIndex = findPropertyIndex(CSSPropertyFontFamily);
+ if (fontSizePropertyIndex == -1 || fontFamilyPropertyIndex == -1)
return emptyString();
- PropertyReference fontSizeProperty = propertyAt(foundPropertyIndex);
- if (fontSizeProperty.isImplicit())
+ PropertyReference fontSizeProperty = propertyAt(fontSizePropertyIndex);
+ PropertyReference fontFamilyProperty = propertyAt(fontFamilyPropertyIndex);
+ if (fontSizeProperty.isImplicit() || fontFamilyProperty.isImplicit())
return emptyString();
String commonValue = fontSizeProperty.value()->cssText();
StringBuilder result;
- bool success = true;
- success &= appendFontLonghandValueIfExplicit(CSSPropertyFontStyle, result, commonValue);
- success &= appendFontLonghandValueIfExplicit(CSSPropertyFontVariant, result, commonValue);
- success &= appendFontLonghandValueIfExplicit(CSSPropertyFontWeight, result, commonValue);
+ appendFontLonghandValueIfExplicit(CSSPropertyFontStyle, result, commonValue);
+ appendFontLonghandValueIfExplicit(CSSPropertyFontVariant, result, commonValue);
+ appendFontLonghandValueIfExplicit(CSSPropertyFontWeight, result, commonValue);
if (!result.isEmpty())
result.append(' ');
result.append(fontSizeProperty.value()->cssText());
- success &= appendFontLonghandValueIfExplicit(CSSPropertyLineHeight, result, commonValue);
- success &= appendFontLonghandValueIfExplicit(CSSPropertyFontFamily, result, commonValue);
- if (!success) {
- // An invalid "font" value has been built (should never happen, as at least implicit values
- // for mandatory longhands are always found in the style), report empty value instead.
- ASSERT_NOT_REACHED();
- return emptyString();
- }
+ appendFontLonghandValueIfExplicit(CSSPropertyLineHeight, result, commonValue);
+ if (!result.isEmpty())
+ result.append(' ');
+ result.append(fontFamilyProperty.value()->cssText());
if (isInitialOrInherit(commonValue))
return commonValue;
return result.toString();
Modified: trunk/Source/WebCore/css/StylePropertySet.h (139312 => 139313)
--- trunk/Source/WebCore/css/StylePropertySet.h 2013-01-10 13:37:27 UTC (rev 139312)
+++ trunk/Source/WebCore/css/StylePropertySet.h 2013-01-10 13:46:39 UTC (rev 139313)
@@ -197,7 +197,7 @@
String get4Values(const StylePropertyShorthand&) const;
String borderSpacingValue(const StylePropertyShorthand&) const;
String fontValue() const;
- bool appendFontLonghandValueIfExplicit(CSSPropertyID, StringBuilder& result, String& value) const;
+ void appendFontLonghandValueIfExplicit(CSSPropertyID, StringBuilder& result, String& value) const;
bool removeShorthandProperty(CSSPropertyID);
bool propertyMatches(const PropertyReference&) const;
