Bug 126674 – CSS word-spacing property does not obey percentages

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Myles C. Maxfield]

According to the CSS3 spec [1]: "<percentage>: Specifies the additional spacing as a percentage of the affected character's advance measure."

[1] http://www.w3.org/TR/css3-text/#word-spacing

[Myles C. Maxfield]

Using Lengths for word-spacing and letter-spacing is a good idea.

[Myles C. Maxfield]

Created attachment 220874 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css3-text/css3-word-spacing-percentage/word-spacing-change-font-expected.html', u'LayoutTests/fast/css3-text/css3-word-spacing-percentage/word-spacing-change-font.html', u'LayoutTests/fast/css3-text/css3-word-spacing-percentage/word-spacing-percentage-expected.html', u'LayoutTests/fast/css3-text/css3-word-spacing-percentage/word-spacing-percentage-parse-expected.txt', u'LayoutTests/fast/css3-text/css3-word-spacing-percentage/word-spacing-percentage-parse.html', u'LayoutTests/fast/css3-text/css3-word-spacing-percentage/word-spacing-percentage.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/DeprecatedStyleBuilder.cpp', u'Source/WebCore/page/animation/CSSPropertyAnimation.cpp', u'Source/WebCore/rendering/RenderBlockLineLayout.cpp', u'Source/WebCore/rendering/RenderText.cpp', u'Source/WebCore/rendering/SimpleLineLayout.cpp', u'Source/WebCore/rendering/line/BreakingContextInlineHeaders.h', u'Source/WebCore/rendering/style/RenderStyle.cpp', u'Source/WebCore/rendering/style/RenderStyle.h', u'Source/WebCore/rendering/style/StyleRareInheritedData.cpp', u'Source/WebCore/rendering/style/StyleRareInheritedData.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/css/CSSParser.cpp:2151:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 20 files


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

[Myles C. Maxfield]

The comments (style error) are used throughout CSSParser. I am being consistent.

[Myles C. Maxfield]

Created attachment 220878 [details]
Patch

[Myles C. Maxfield]

This new style error looks like it's caused by the binary (.png) file.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css1/text_properties/word_spacing.html', u'LayoutTests/css2.1/20110323/c541-word-sp-001-expected.html', u'LayoutTests/css2.1/20110323/c541-word-sp-001.htm', u'LayoutTests/fast/css3-text/css3-word-spacing-percentage/word-spacing-change-font-expected.html', u'LayoutTests/fast/css3-text/css3-word-spacing-percentage/word-spacing-change-font.html', u'LayoutTests/fast/css3-text/css3-word-spacing-percentage/word-spacing-percentage-expected.html', u'LayoutTests/fast/css3-text/css3-word-spacing-percentage/word-spacing-percentage-parse-expected.txt', u'LayoutTests/fast/css3-text/css3-word-spacing-percentage/word-spacing-percentage-parse.html', u'LayoutTests/fast/css3-text/css3-word-spacing-percentage/word-spacing-percentage.html', u'LayoutTests/platform/mac/css1/text_properties/word_spacing-expected.png', u'LayoutTests/platform/mac/css1/text_properties/word_spacing-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/DeprecatedStyleBuilder.cpp', u'Source/WebCore/page/animation/CSSPropertyAnimation.cpp', u'Source/WebCore/rendering/RenderBlockLineLayout.cpp', u'Source/WebCore/rendering/RenderText.cpp', u'Source/WebCore/rendering/SimpleLineLayout.cpp', u'Source/WebCore/rendering/line/BreakingContextInlineHeaders.h', u'Source/WebCore/rendering/style/RenderStyle.cpp', u'Source/WebCore/rendering/style/RenderStyle.h', u'Source/WebCore/rendering/style/StyleRareInheritedData.cpp', u'Source/WebCore/rendering/style/StyleRareInheritedData.h', '--commit-queue']" exit_code: 1
Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/main.py", line 154, in main
    patch_checker.check(patch)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/patchreader.py", line 71, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=None)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 118, in process_file
    lines = self._read_lines(file_path)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 86, in _read_lines
    contents = file.read()
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 671, in read
    return self.reader.read(size)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 477, in read
    newchars, decodedbytes = self.decode(data, self.errors)
UnicodeDecodeError: 'utf8' codec can't decode byte 0x89 in position 0: invalid start byte


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

[Myles C. Maxfield]

Relevant style bug is here: https://bugs.webkit.org/show_bug.cgi?id=126702

[Myles C. Maxfield]

Comment on attachment 220878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220878&action=review

> LayoutTests/css1/text_properties/word_spacing.html:19
> +.ten {word-spacing: -0.2em;}

Image uses wrong scrollbars

> LayoutTests/css2.1/20110323/c541-word-sp-001.htm:15
>     .eleven {word-spacing: -1em;}

Rename eleven to ten

[Simon Fraser (smfr)]

Comment on attachment 220878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220878&action=review

> Source/WebCore/rendering/RenderText.cpp:686
>      const Font& f = style.font(); // FIXME: This ignores first-line.
> -    float wordSpacing = style.wordSpacing();
> +    float wordSpacing = f.wordSpacing();

Please rename "f" to something longer.

> Source/WebCore/rendering/SimpleLineLayout.cpp:143
> +    if (style.wordSpacing().getFloatValue() || style.letterSpacing())

!isZero()?

> LayoutTests/ChangeLog:23
> +        * platform/mac/css1/text_properties/word_spacing-expected.png:

You should figure out why this doesn't have a mock scrollbar.

[Myles C. Maxfield]

Comment on attachment 220878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220878&action=review

>> Source/WebCore/rendering/RenderText.cpp:686
>> +    float wordSpacing = f.wordSpacing();
> 
> Please rename "f" to something longer.

Done.

>> Source/WebCore/rendering/SimpleLineLayout.cpp:143
>> +    if (style.wordSpacing().getFloatValue() || style.letterSpacing())
> 
> !isZero()?

Done.

[Build Bot]

Comment on attachment 220878 [details]
Patch

Attachment 220878 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4625311215910912

New failing tests:
fast/css3-text/css3-word-spacing-percentage/word-spacing-change-font.html

[Build Bot]

Created attachment 220888 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 220878 [details]
Patch

Attachment 220878 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6408786050809856

New failing tests:
fast/css3-text/css3-word-spacing-percentage/word-spacing-change-font.html

[Build Bot]

Created attachment 220899 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Myles C. Maxfield]

Comment on attachment 220878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220878&action=review

>> LayoutTests/ChangeLog:23
>> +        * platform/mac/css1/text_properties/word_spacing-expected.png:
> 
> You should figure out why this doesn't have a mock scrollbar.

filed https://bugs.webkit.org/show_bug.cgi?id=126787 (this occurs on ToT)

[Myles C. Maxfield]

http://trac.webkit.org/changeset/161696