Bug 57872

Summary:

REGRESSION (r46914, r48764): When typing in Mail, line wrapping frequently occurs in the middle of words

Product:

WebKit

Reporter:

Adele Peterson <adele>

Component:

HTML Editing

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, adele, commit-queue, darin, enrica, eric, mitz, rniwa, webkit.review.bot

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Bug Depends on:

57978, 58019, 58037

Bug Blocks:

Attachments:

Description	Flags
testcase	none
fixes the bug	none
work in progress (applies on top of the original patch)	none
complete patch (needs to rebaseline editing/inserting/6633727.html)	none
fixes the bug	none

Adele Peterson

Reported 2011-04-05 12:36:05 PDT

Created attachment 88293 [details] testcase <rdar://problem/8157205> http://trac.webkit.org/changeset/46914 was not supposed to change behavior, but it definitely did. From looking of different reports of this problem, it seems to frequently happen with tab characters. I'm attaching an example that shows the problem. Before r46914, when you hit enter after the tab character and start typing, the new text doesn't get the "white-space: pre" style, but after that change, it does. To fix Mail, if we changed the tab style to use "pre-wrap", that would fix this problem, but I'd like to figure out what went wrong in r46914, and whether we can go back to the old behavior.

Attachments
testcase (755 bytes, text/html) 2011-04-05 12:36 PDT, Adele Peterson	no flags	Details
fixes the bug (8.55 KB, patch) 2011-04-06 05:49 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
work in progress (applies on top of the original patch) (4.47 KB, patch) 2011-04-06 17:48 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
complete patch (needs to rebaseline editing/inserting/6633727.html) (17.90 KB, patch) 2011-04-07 08:02 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
fixes the bug (18.57 KB, patch) 2011-04-07 13:20 PDT, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2011-04-06 03:28:12 PDT

This bug was a regression from two change sets. http://trac.webkit.org/changeset/46914 initially introduced a regression because it wasn't avoiding tab span to obtain the computed style to apply after the insertion. http://trac.webkit.org/changeset/46914 introduced another regression by copying hierarchy under new block at the insertion position without avoiding the tab span. I'm going to upload a patch in a minute to fix this bug, but we should really get rid of this tab span business at some point.

Ryosuke Niwa

Comment 2 2011-04-06 03:28:45 PDT

I mean to say "r48764 introduced another regression".

Ryosuke Niwa

Comment 3 2011-04-06 05:49:34 PDT

Created attachment 88401 [details] fixes the bug

Eric Seidel (no email)

Comment 4 2011-04-06 05:53:22 PDT

Comment on attachment 88401 [details] fixes the bug Seems reasonable.

Ryosuke Niwa

Comment 5 2011-04-06 05:55:27 PDT

(In reply to comment #4) > (From update of attachment 88401 [details]) > Seems reasonable. Thanks for the review! Landing it now.

Ryosuke Niwa

Comment 6 2011-04-06 05:59:40 PDT

Committed r83039: <http://trac.webkit.org/changeset/83039>

WebKit Review Bot

Comment 7 2011-04-06 06:54:05 PDT

http://trac.webkit.org/changeset/83039 might have broken Windows 7 Release (Tests)

Adele Peterson

Comment 8 2011-04-06 13:03:53 PDT

I believe this caused a serious regression, and may need to be rolled out. Running Mail with this change, use the following steps: - Set a default signature - Place the caret at the beginning of the message - Hit tab - Copy a url from Safari's address field and paste it into the message - Hit Enter The caret will skip over the signature and land at the end of the message

Ryosuke Niwa

Comment 9 2011-04-06 13:22:10 PDT

(In reply to comment #8) > Running Mail with this change, use the following steps: > > - Set a default signature > - Place the caret at the beginning of the message > - Hit tab > - Copy a url from Safari's address field and paste it into the message > - Hit Enter > > The caret will skip over the signature and land at the end of the message Mn... that seems to imply that the pasted URL is inserted inside a tab span because all this patch does is to avoid inserting a paragraph separator inside a tab span. Do you happen to know the HTML right before you hit the enter?

Adele Peterson

Comment 10 2011-04-06 13:31:37 PDT

Yes, you're right that the url gets pasted in the tab-span. <html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><span class="Apple-tab-span" style="white-space:pre"> https://bugs.webkit.org/show_bug.cgi?id=57872</span><br><div id="AppleMailSignature"> <span class="Apple-style-span" style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; -webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; font-size: medium; "><div>Thanks,</div><div><span class="Apple-tab-span" style="white-space: pre; "> </span>Adele</div></span> </div> <br></body></html>

Ryosuke Niwa

Comment 11 2011-04-06 13:48:00 PDT

I'm rolling out http://trac.webkit.org/changeset/83039 for now per Adele's request. Will investigate the regression tomorrow.

Ryosuke Niwa

Comment 12 2011-04-06 17:48:14 PDT

Created attachment 88549 [details] work in progress (applies on top of the original patch) Here's working in progress. I'm reverting r48764 for the general path and instead solving the bug 29740 (a.k.a. rdar://problem/7168738) by splitting nodes up until the startBlock.

Ryosuke Niwa

Comment 13 2011-04-07 08:02:56 PDT

Created attachment 88639 [details] complete patch (needs to rebaseline editing/inserting/6633727.html)

Ryosuke Niwa

Comment 14 2011-04-07 08:04:11 PDT

Comment on attachment 88639 [details] complete patch (needs to rebaseline editing/inserting/6633727.html) Here's my latest work in progress. I can upload the final patch for review as soon as the bug 58037 is fixed.

Ryosuke Niwa

Comment 15 2011-04-07 13:20:07 PDT

Created attachment 88679 [details] fixes the bug

Ryosuke Niwa

Comment 16 2011-04-07 13:23:50 PDT

This patch hopefully fix all the regressions and bugs we have.

Darin Adler

Comment 17 2011-04-07 13:32:45 PDT

Comment on attachment 88679 [details] fixes the bug Looks like a good approach.

Ryosuke Niwa

Comment 18 2011-04-07 13:35:39 PDT

Comment on attachment 88679 [details] fixes the bug (In reply to comment #17) > (From update of attachment 88679 [details]) > Looks like a good approach. Thanks for the review. Hopefully, this won't introduce a new regression...

WebKit Commit Bot

Comment 19 2011-04-07 21:08:01 PDT

The commit-queue encountered the following flaky tests while processing attachment 88679 [details]: http/tests/websocket/tests/multiple-connections.html bug 53825 (author: abarth@webkit.org) The commit-queue is continuing to process your patch.

WebKit Commit Bot

Comment 20 2011-04-07 21:11:34 PDT

Comment on attachment 88679 [details] fixes the bug Clearing flags on attachment: 88679 Committed r83247: <http://trac.webkit.org/changeset/83247>

WebKit Commit Bot

Comment 21 2011-04-07 21:11:39 PDT

All reviewed patches have been landed. Closing bug.

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