Bug 57872 - REGRESSION (r46914, r48764): When typing in Mail, line wrapping frequently occurs in the middle of words
Summary: REGRESSION (r46914, r48764): When typing in Mail, line wrapping frequently oc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 57978 58019 58037
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-05 12:36 PDT by Adele Peterson
Modified: 2011-04-07 21:11 PDT (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Adele Peterson 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.
Comment 1 Ryosuke Niwa 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.
Comment 2 Ryosuke Niwa 2011-04-06 03:28:45 PDT
I mean to say "r48764 introduced another regression".
Comment 3 Ryosuke Niwa 2011-04-06 05:49:34 PDT
Created attachment 88401 [details]
fixes the bug
Comment 4 Eric Seidel (no email) 2011-04-06 05:53:22 PDT
Comment on attachment 88401 [details]
fixes the bug

Seems reasonable.
Comment 5 Ryosuke Niwa 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.
Comment 6 Ryosuke Niwa 2011-04-06 05:59:40 PDT
Committed r83039: <http://trac.webkit.org/changeset/83039>
Comment 7 WebKit Review Bot 2011-04-06 06:54:05 PDT
http://trac.webkit.org/changeset/83039 might have broken Windows 7 Release (Tests)
Comment 8 Adele Peterson 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
Comment 9 Ryosuke Niwa 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?
Comment 10 Adele Peterson 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>
Comment 11 Ryosuke Niwa 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.
Comment 12 Ryosuke Niwa 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.
Comment 13 Ryosuke Niwa 2011-04-07 08:02:56 PDT
Created attachment 88639 [details]
complete patch (needs to rebaseline editing/inserting/6633727.html)
Comment 14 Ryosuke Niwa 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.
Comment 15 Ryosuke Niwa 2011-04-07 13:20:07 PDT
Created attachment 88679 [details]
fixes the bug
Comment 16 Ryosuke Niwa 2011-04-07 13:23:50 PDT
This patch hopefully fix all the regressions and bugs we have.
Comment 17 Darin Adler 2011-04-07 13:32:45 PDT
Comment on attachment 88679 [details]
fixes the bug

Looks like a good approach.
Comment 18 Ryosuke Niwa 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...
Comment 19 WebKit Commit Bot 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.
Comment 20 WebKit Commit Bot 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>
Comment 21 WebKit Commit Bot 2011-04-07 21:11:39 PDT
All reviewed patches have been landed.  Closing bug.