Bug 13234

Summary:

Layout of selected justified text is broken

Product:

WebKit

Reporter:

Adam Treat <manyoso>

Component:

Layout and Rendering

Assignee:

Dave Hyatt <hyatt>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

mitz

Priority:

Version:

523.x (Safari 3)

Hardware:

All

OS:

Linux

Attachments:

Description	Flags
Patch that eliminates run.from() and run.to() in favor of passing them in as args only when needed	mitz: review-
Add test and fix bug pointed out by mitz	mitz: review+

Adam Treat

Reported 2007-03-30 14:20:20 PDT

Selections of justified text can create erratic movement of the text within the page. Here is a testcase that works with WebKitQt/QtLauncher: <html> <body>  <p align="justify">Foo Bar Baz Foo Baz Bar Foo Baz Bar Baz Foo Baz Bar Foo Baz</p> </body> </html> When the text is wrapped right after the last foo this bug appears. On the mac it can be seen by using ::selection.

Attachments
Patch that eliminates run.from() and run.to() in favor of passing them in as args only when needed (38.25 KB, patch) 2007-03-30 17:45 PDT, Dave Hyatt	mitz: review-	Details Formatted Diff Diff
Add test and fix bug pointed out by mitz (72.84 KB, patch) 2007-03-31 22:32 PDT, Dave Hyatt	mitz: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Dave Hyatt

Comment 1 2007-03-30 17:45:58 PDT

Created attachment 13897 [details] Patch that eliminates run.from() and run.to() in favor of passing them in as args only when needed

mitz

Comment 2 2007-03-31 02:22:05 PDT

Comment on attachment 13897 [details] Patch that eliminates run.from() and run.to() in favor of passing them in as args only when needed + // Using roundf() rather than ceilf() for the right edge as a compromise to ensure correct caret positioning + if (style.rtl()) { + it.advance(run.length()); + startX += it.m_finalRoundingWidth - afterWidth; The copy&pasted comment doesn't make sense there. Also, I think you need to cache the finalRoundingWidth before advancing to the end and use that (like floatWidthForSimpleText() did), otherwise RTL text will jiggle as you select it (if it's painted separately). Finally, you forgot to add it.m_runWidthSoFar there (that's why you advance to the end). So I think the correct code would be + if (style.rtl()) { + float finalRoundingWidth = it.m_finalRoundingWidth; + it.advance(run.length()); + startX += finalRoundingWidth + it.m_runWidthSoFar - afterWidth; + if (Font::isRoundingHackCharacter(nextCh) && (!isLastChar || params->m_style.applyRunRounding())){ Missing space before the { there. Please include a test for the justified text bug (similar to the one for bug 13224).

Dave Hyatt

Comment 3 2007-03-31 22:32:04 PDT

Created attachment 13913 [details] Add test and fix bug pointed out by mitz

mitz

Comment 4 2007-03-31 22:33:32 PDT

Comment on attachment 13913 [details] Add test and fix bug pointed out by mitz r=me

Dave Hyatt

Comment 5 2007-03-31 22:37:41 PDT

Fixed in r20646.

Note You need to log in before you can comment on or make changes to this bug.