RESOLVED FIXED 56017
REGRESSION: Soft hyphen is not always rendered 
https://bugs.webkit.org/show_bug.cgi?id=56017
Summary REGRESSION: Soft hyphen is not always rendered
David Sosby
Reported 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.
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
DetailsFormatted DiffDiff
Updated patch to fix tabs/spaces style issue (whoops!) (3.69 KB, patch)
2011-03-09 09:29 PST, David Sosby 		mitz: review-
DetailsFormatted DiffDiff
Proposed patch (9.99 KB, patch)
2011-03-09 13:11 PST, David Sosby 		mitz: review+
commit-queue: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
WebKit Review Bot
Comment 1 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.
mitz
Comment 2 2011-03-09 08:17:29 PST
<rdar://problem/9108239>
David Sosby
Comment 3 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).
WebKit Review Bot
Comment 4 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.
David Sosby
Comment 5 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!)
mitz
Comment 6 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.
David Sosby
Comment 7 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
mitz
Comment 8 2011-03-09 13:17:29 PST
Is this ready for review?
David Sosby
Comment 9 2011-03-09 13:19:46 PST
(In reply to comment #8) > Is this ready for review? Yes sir. :)
WebKit Commit Bot
Comment 10 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
mitz
Comment 11 2011-03-13 18:10:27 PDT
Landed with tweaked test in r80984. <http://trac.webkit.org/changeset/80984>
Note You need to log in before you can comment on or make changes to this bug.