Bug 104996 – Implement hanging-punctuation: allow-end

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

[Yuki Sekiguchi]

Created attachment 179430 [details]
demo content

http://www.w3.org/TR/2012/WD-css3-text-20121113/#hanging-punctuation0

> ‘allow-end’
>    A stop or comma at the end of a line hangs if it does not otherwise fit prior to justification.

Implement this value.

The attached content is printed like attached image.

Layout of allow-end is easy to read for japanese reader.

[Yuki Sekiguchi]

Created attachment 179432 [details]
demo expectation

[Yuki Sekiguchi]

Created attachment 179434 [details]
Patch

[WebKit Review Bot]

Comment on attachment 179434 [details]
Patch

Attachment 179434 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15351002

New failing tests:
fast/js/regexp-overflow.html
fast/forms/input-maxlength.html
inspector/extensions/extensions-network.html
fast/css/counters/asterisk-counter-update-after-layout-crash.html
fast/text/find-backwards.html
fast/forms/input-implicit-length-limit.html

[Build Bot]

Comment on attachment 179434 [details]
Patch

Attachment 179434 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15320645

New failing tests:
fast/forms/input-maxlength.html
fast/forms/input-implicit-length-limit.html
fast/text/find-backwards.html
fast/css/counters/asterisk-counter-update-after-layout-crash.html

[WebKit Review Bot]

Comment on attachment 179434 [details]
Patch

Attachment 179434 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15310735

New failing tests:
fast/js/regexp-overflow.html
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
fast/forms/input-maxlength.html
inspector/extensions/extensions-network.html
fast/css/counters/asterisk-counter-update-after-layout-crash.html
fast/text/find-backwards.html
fast/forms/input-implicit-length-limit.html

[Yuki Sekiguchi]

Created attachment 179688 [details]
Rebaseline

[Build Bot]

Comment on attachment 179688 [details]
Rebaseline

Attachment 179688 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15360671

New failing tests:
fast/forms/input-maxlength.html
fast/forms/input-implicit-length-limit.html
fast/text/find-backwards.html
fast/css/counters/asterisk-counter-update-after-layout-crash.html

[WebKit Review Bot]

Comment on attachment 179688 [details]
Rebaseline

Attachment 179688 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15375606

New failing tests:
fast/js/regexp-overflow.html
fast/forms/input-maxlength.html
inspector/extensions/extensions-network.html
fast/css/counters/asterisk-counter-update-after-layout-crash.html
fast/text/find-backwards.html
fast/forms/input-implicit-length-limit.html

[Yuki Sekiguchi]

Created attachment 179699 [details]
Accelerate normal case

[Anders Carlsson]

Comment on attachment 179699 [details]
Accelerate normal case

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

[Dave Hyatt]

Created attachment 274686 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/RenderText.cpp:551:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/rendering/line/BreakingContext.h:732:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/line/BreakingContext.h:733:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
ERROR: Source/WebCore/rendering/line/BreakingContext.h:865:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/WebCore/rendering/line/BreakingContext.h:869:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/WebCore/rendering/line/BreakingContext.h:896:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/WebCore/rendering/line/BreakingContext.h:919:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 7 in 5 files


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

[Dave Hyatt]

This still has issues. Just testing the basic core against EWS. I have to handle more cases still.

[Build Bot]

Comment on attachment 274686 [details]
Patch

Attachment 274686 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1022748

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 274687 [details]
Archive of layout-test-results from ews100 for mac-yosemite

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

[Build Bot]

Comment on attachment 274686 [details]
Patch

Attachment 274686 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1022737

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 274688 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 274686 [details]
Patch

Attachment 274686 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1022750

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 274689 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 274686 [details]
Patch

Attachment 274686 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1022772

New failing tests:
imported/blink/http/tests/security/contentSecurityPolicy/object-src-applet-code.html
fast/text/simple-line-with-multiple-renderers.html
fast/canvas/drawImageFromRect_withToDataURLAsSource.html
fast/forms/basic-textareas.html
editing/execCommand/italicizeByCharacter.html
fast/text/whitespace/pre-wrap-spaces-after-newline-simple-lines.html
fast/text/whitespace/pre-wrap-line-test.html
editing/style/create-block-for-style-007.html
http/tests/security/contentSecurityPolicy/object-src-applet-code.html
imported/blink/http/tests/security/contentSecurityPolicy/object-src-applet-archive-codebase.html
http/tests/security/contentSecurityPolicy/object-src-applet-archive.html
fast/text/whitespace/pre-wrap-line-test-simple-lines.html
fast/block/float/editable-text-overlapping-float.html
fast/text/whitespace/pre-wrap-overflow-selection.html
imported/blink/http/tests/security/contentSecurityPolicy/object-src-applet-code-codebase.html
editing/selection/4983858.html
editing/execCommand/strikethroughSelection.html
fast/text/whitespace/pre-wrap-spaces-after-newline.html
http/tests/security/contentSecurityPolicy/object-src-applet-code-codebase.html
editing/execCommand/modifyForeColorByCharacter.html
http/tests/security/contentSecurityPolicy/object-src-applet-archive-codebase.html
editing/style/create-block-for-style-008.html
editing/selection/5081257-1.html
imported/blink/http/tests/security/contentSecurityPolicy/object-src-applet-archive.html

[Build Bot]

Created attachment 274693 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5

[Dave Hyatt]

Created attachment 274760 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/line/BreakingContext.h:918:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 5 files


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

[Dave Hyatt]

Created attachment 274782 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 274782 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:340
> +    bool isLogicallyLastRunWrapped = bidiRuns.logicallyLastRun()->renderer().isText() ? !reachedEndOfTextRenderer(bidiRuns) : (is<RenderInline>(bidiRuns.logicallyLastRun()->renderer()) ? false : true);

Isn't this the same as:

bool isLogicallyLastRunWrapped = bidiRuns.logicallyLastRun()->renderer().isText() ? !reachedEndOfTextRenderer(bidiRuns) : !is<RenderInline>(bidiRuns.logicallyLastRun()->renderer();

would be nice to somehow flip the "nots" in there.

> Source/WebCore/rendering/RenderText.h:109
> +    bool isHangableStopOrComma(UChar) const;

should be static, probably inline.

> Source/WebCore/rendering/line/BreakingContext.h:293
> +    bool m_hangsAtEnd { false };

Might as well update the other members to this style of initialization.

> Source/WebCore/rendering/line/BreakingContext.h:721
> +inline float BreakingContext::computeAdditionalBetweenWordsWidth(RenderText& renderText, TextLayout* textLayout, UChar c, WordTrailingSpace& wordTrailingSpace, HashSet<const Font*>& fallbackFonts, WordMeasurements& wordMeasurements, const FontCascade& font, bool isFixedPitch, unsigned lastSpace, float lastSpaceWordSpacing, float wordSpacingForWordMeasurement, unsigned offset)

It's unclear what the 'c' argument is for.

> Source/WebCore/rendering/line/BreakingContext.h:737
> +    if (wordTrailingSpaceWidth)
> +        additionalTempWidth = textWidth(renderText, lastSpace, offset + 1 - lastSpace, font, m_width.currentWidth(), isFixedPitch, m_collapseWhiteSpace, wordMeasurement.fallbackFonts, textLayout) - wordTrailingSpaceWidth.value();
> +    else
> +        additionalTempWidth = textWidth(renderText, lastSpace, offset - lastSpace, font, m_width.currentWidth(), isFixedPitch, m_collapseWhiteSpace, wordMeasurement.fallbackFonts, textLayout);

I would prefer a single call to textWidth(). It's hard to see how the params differ.

> Source/WebCore/rendering/line/BreakingContext.h:1052
> +                // Stop ignoring spaces and begin at this
> +                // new point.

Unwrap the comment.

[Dave Hyatt]

Landed in r198683.

[yisibl]

Why did I test the punctuation in the WebKit without hanging?
Because WebKit does not support 'text-align-last'?