Bug 104996 - Implement hanging-punctuation: allow-end
Summary: Implement hanging-punctuation: allow-end
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks: 18109
  Show dependency treegraph
 
Reported: 2012-12-13 23:51 PST by Yuki Sekiguchi
Modified: 2016-06-22 20:35 PDT (History)
23 users (show)

See Also:


Attachments
demo content (564 bytes, text/html)
2012-12-13 23:51 PST, Yuki Sekiguchi
no flags Details
demo expectation (64.30 KB, image/png)
2012-12-13 23:52 PST, Yuki Sekiguchi
no flags Details
Patch (36.70 KB, patch)
2012-12-14 00:11 PST, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Rebaseline (36.67 KB, patch)
2012-12-16 21:40 PST, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Accelerate normal case (36.90 KB, patch)
2012-12-17 00:46 PST, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (17.10 KB, patch)
2016-03-22 14:41 PDT, Dave Hyatt
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-yosemite (923.47 KB, application/zip)
2016-03-22 15:16 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (1000.25 KB, application/zip)
2016-03-22 15:16 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (964.83 KB, application/zip)
2016-03-22 15:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.43 MB, application/zip)
2016-03-22 15:34 PDT, Build Bot
no flags Details
Patch (18.53 KB, patch)
2016-03-23 10:36 PDT, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (29.56 KB, patch)
2016-03-23 15:11 PDT, Dave Hyatt
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuki Sekiguchi 2012-12-13 23:51:04 PST
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.
Comment 1 Yuki Sekiguchi 2012-12-13 23:52:12 PST
Created attachment 179432 [details]
demo expectation
Comment 2 Yuki Sekiguchi 2012-12-14 00:11:29 PST
Created attachment 179434 [details]
Patch
Comment 3 WebKit Review Bot 2012-12-14 07:31:43 PST
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
Comment 4 Build Bot 2012-12-14 08:09:12 PST
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
Comment 5 WebKit Review Bot 2012-12-14 08:48:04 PST
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
Comment 6 Yuki Sekiguchi 2012-12-16 21:40:53 PST
Created attachment 179688 [details]
Rebaseline
Comment 7 Build Bot 2012-12-16 23:35:36 PST
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
Comment 8 WebKit Review Bot 2012-12-16 23:55:03 PST
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
Comment 9 Yuki Sekiguchi 2012-12-17 00:46:28 PST
Created attachment 179699 [details]
Accelerate normal case
Comment 10 Anders Carlsson 2014-02-05 11:06:09 PST
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.
Comment 11 Dave Hyatt 2016-03-22 14:41:24 PDT
Created attachment 274686 [details]
Patch
Comment 12 WebKit Commit Bot 2016-03-22 14:43:27 PDT
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.
Comment 13 Dave Hyatt 2016-03-22 14:56:49 PDT
This still has issues. Just testing the basic core against EWS. I have to handle more cases still.
Comment 14 Build Bot 2016-03-22 15:16:27 PDT
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.
Comment 15 Build Bot 2016-03-22 15:16:33 PDT
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
Comment 16 Build Bot 2016-03-22 15:16:50 PDT
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.
Comment 17 Build Bot 2016-03-22 15:16:56 PDT
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
Comment 18 Build Bot 2016-03-22 15:18:17 PDT
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.
Comment 19 Build Bot 2016-03-22 15:18:23 PDT
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
Comment 20 Build Bot 2016-03-22 15:34:38 PDT
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
Comment 21 Build Bot 2016-03-22 15:34:45 PDT
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
Comment 22 Dave Hyatt 2016-03-23 10:36:00 PDT
Created attachment 274760 [details]
Patch
Comment 23 WebKit Commit Bot 2016-03-23 10:37:33 PDT
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.
Comment 24 Dave Hyatt 2016-03-23 15:11:55 PDT
Created attachment 274782 [details]
Patch
Comment 25 Simon Fraser (smfr) 2016-03-23 15:22:25 PDT
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.
Comment 26 Dave Hyatt 2016-03-25 12:25:25 PDT
Landed in r198683.
Comment 27 yisibl 2016-05-11 20:13:04 PDT
Why did I test the punctuation in the WebKit without hanging?
Because WebKit does not support 'text-align-last'?