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 EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, adele, commit-queue, darin, enrica, eric, mitz, rniwa, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
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
DetailsFormatted DiffDiff
work in progress (applies on top of the original patch) (4.47 KB, patch)
2011-04-06 17:48 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
complete patch (needs to rebaseline editing/inserting/6633727.html) (17.90 KB, patch)
2011-04-07 08:02 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
fixes the bug (18.57 KB, patch)
2011-04-07 13:20 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
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.