Bug 56017 - REGRESSION: Soft hyphen is not always rendered
Summary: REGRESSION: Soft hyphen is not always rendered
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Nobody
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2011-03-09 08:14 PST by David Sosby
Modified: 2011-03-13 18:10 PDT (History)
5 users (show)

See Also:


Attachments
Test file exhibiting issue (758 bytes, text/html)
2011-03-09 08:14 PST, David Sosby
no flags Details
Proposed patch (3.65 KB, patch)
2011-03-09 09:19 PST, David Sosby
no flags Details | Formatted Diff | Diff
Updated patch to fix tabs/spaces style issue (whoops!) (3.69 KB, patch)
2011-03-09 09:29 PST, David Sosby
mitz: review-
Details | Formatted Diff | Diff
Proposed patch (9.99 KB, patch)
2011-03-09 13:11 PST, David Sosby
mitz: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Sosby 2011-03-09 08:14:04 PST
Created attachment 85179 [details]
Test file exhibiting issue

Some strings with soft hyphens (­) do not render the soft hyphen though the line breaks on the hyphen. The trigger strings are ones that do not have a breakpoint between the soft hyphen and the end of the text block.

This behavior has been observed on Chrome 9.0.597.107 534.13 (Windows and Ubuntu10.10) and WebKitGTK 534.21+. The behavior does not appear in Windows Safari 5.0.3 (7533.19.4). 

Attachment demonstrates issue.
Comment 1 WebKit Review Bot 2011-03-09 08:15:59 PST
Attachment 85179 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1

Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 2 mitz 2011-03-09 08:17:29 PST
<rdar://problem/9108239>
Comment 3 David Sosby 2011-03-09 09:19:07 PST
Created attachment 85182 [details]
Proposed patch

This is a proposed fix that resolves the issue seen in the demo file. 

The soft hyphen check was only being done inside of the while loop when a word break or space was encountered. With some lines, there are no other word breaks after the soft hyphen. The fix was to add a soft hyphen check after the loop next to the platform hyphenation check (tryHyphenating).
Comment 4 WebKit Review Bot 2011-03-09 09:21:32 PST
Attachment 85182 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderBlockLineLayout.cpp:1931:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/rendering/RenderBlockLineLayout.cpp:1932:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/rendering/RenderBlockLineLayout.cpp:1935:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 David Sosby 2011-03-09 09:29:05 PST
Created attachment 85185 [details]
Updated patch to fix tabs/spaces style issue (whoops!)

Fixing tabs/spaces style issue (whoops!)
Comment 6 mitz 2011-03-09 10:34:26 PST
Comment on attachment 85185 [details]
Updated patch to fix tabs/spaces style issue (whoops!)

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

Code change is good, but I have comments about the change log and the test.

> Source/WebCore/ChangeLog:5
> +        Resolve issue with soft hyphen not rendering

It’s customary to include the bug title in the change log.

> Source/WebCore/ChangeLog:11
> +        (WebCore::RenderBlock::findNextLineBreak):

Please describe the change to this function.

> LayoutTests/ChangeLog:8
> +        * fast/text/soft-hyphen-4.html: Added.

Where are the expected results?

> LayoutTests/fast/text/soft-hyphen-4.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Why the antiquated DOCTYPE?

> LayoutTests/fast/text/soft-hyphen-4.html:12
> +            font-family: sans-serif;

Please consider rewriting the test to use the Ahem fonts so that the results are consistent across platforms (and can be landed alongside the test, rather than in platform-specific locations.
Comment 7 David Sosby 2011-03-09 13:11:51 PST
Created attachment 85220 [details]
Proposed patch

-Updated change logs as requested
-Updated the doc type of the test file
-Modified the test to use Ahem font and be platform independent
-Added platform independent expected results
Comment 8 mitz 2011-03-09 13:17:29 PST
Is this ready for review?
Comment 9 David Sosby 2011-03-09 13:19:46 PST
(In reply to comment #8)
> Is this ready for review?

Yes sir. :)
Comment 10 WebKit Commit Bot 2011-03-10 17:24:47 PST
Comment on attachment 85220 [details]
Proposed patch

Rejecting attachment 85220 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'bu..." exit_code: 2

Last 500 characters of output:
.............................................................................................................................
fast/table/border-collapsing ..............
fast/text .............................................................................................
fast/text/soft-hyphen-4.html -> failed

Exiting early after 1 failures. 11049 tests run.
209.78s total testing time

11048 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
6 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8131092
Comment 11 mitz 2011-03-13 18:10:27 PDT
Landed with tweaked test in r80984.
<http://trac.webkit.org/changeset/80984>