Bug 56017

Summary:

REGRESSION: Soft hyphen is not always rendered

Product:

WebKit

Reporter:

David Sosby <dsosby>

Component:

Layout and Rendering

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, dbates, mitz, mrobinson, webkit.review.bot

Priority:

Keywords:

InRadar, Regression

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Test file exhibiting issue	none
Proposed patch	none
Updated patch to fix tabs/spaces style issue (whoops!)	mitz: review-
Proposed patch	mitz: review+, commit-queue: commit-queue-

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	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
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.