Bug 213536

Summary:

Cannot delete last line of Mail Message

Product:

WebKit

Reporter:

Megan Gardner <megan_gardner>

Component:

New Bugs

Assignee:

Megan Gardner <megan_gardner>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, dbates, ews-watchlist, mifenton, thorton, wenson_hsieh

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Megan Gardner

Reported 2020-06-23 16:07:08 PDT

Cannot delete last line of Mail Message

Attachments
Patch (7.99 KB, patch) 2020-06-23 16:16 PDT, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (7.96 KB, patch) 2020-06-23 18:00 PDT, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (7.96 KB, patch) 2020-06-23 19:01 PDT, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (7.51 KB, patch) 2020-06-23 22:29 PDT, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (11.14 KB, patch) 2020-06-24 11:59 PDT, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (11.21 KB, patch) 2020-06-24 12:00 PDT, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (11.43 KB, patch) 2020-06-24 12:16 PDT, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (11.36 KB, patch) 2020-06-24 14:33 PDT, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (11.06 KB, patch) 2020-06-24 15:33 PDT, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (11.14 KB, patch) 2020-06-24 16:20 PDT, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (11.07 KB, patch) 2020-06-24 17:14 PDT, Megan Gardner	no flags	Details Formatted Diff Diff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

Megan Gardner

Comment 1 2020-06-23 16:16:50 PDT

Created attachment 402604 [details] Patch

Megan Gardner

Comment 2 2020-06-23 16:18:22 PDT

<rdar://problem/63420928>

Darin Adler

Comment 3 2020-06-23 16:47:02 PDT

Comment on attachment 402604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402604&action=review > Source/WebCore/editing/DeleteSelectionCommand.cpp:211 > + position = endOfParagraphBeforeStart.deepEquivalent(); This line doesn’t seem like it needs to be in the if statement.

Wenson Hsieh

Comment 4 2020-06-23 16:48:19 PDT

Comment on attachment 402604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402604&action=review Looks reasonable, but you might need to tweak the test or add different baselines for some of the other platforms. > Source/WebCore/editing/DeleteSelectionCommand.cpp:215 > + VisiblePosition endOfParagraphBeforeStart; > + Position position; > + if (previousPositionIsStartOfContent) { > + endOfParagraphBeforeStart = endOfParagraph(VisiblePosition { m_upstreamStart }.previous()); > + position = endOfParagraphBeforeStart.deepEquivalent(); > + } else { > + endOfParagraphBeforeStart = endOfParagraph(VisiblePosition { m_upstreamStart }.previous().previous()); > + position = endOfParagraphBeforeStart.deepEquivalent(); > + } Nit - I think this could be written a little more succinctly as: … VisiblePosition positionBeforeUpstreamStart { m_upstreamStart }.previous(); if (!previousPositionIsStartOfContent) positionBeforeUpstreamStart = positionBeforeUpstreamStart.previous(); auto position = endOfParagraph(positionBeforeUpstreamStart).deepEquivalent(); … ...or perhaps … auto positionBeforeUpstreamStart = previousPositionIsStartOfContent ? VisiblePosition { m_upstreamStart }.previous() : VisiblePosition { m_upstreamStart }.previous().previous(); auto position = endOfParagraph(positionBeforeUpstreamStart).deepEquivalent(); … At least, the last line (position = endOfParagraphBeforeStart.deepEquivalent();) could be moved out of the if/else. > LayoutTests/ChangeLog:8 > + * editing/deleting/smart-delete-paragraph-001.html: Nit - this ChangeLog entry looks out of date. > LayoutTests/editing/deleting/smart-delete-paragraph-005.html:28 > +Test must exist at the top of the file in order to correclty test the original bug. Nit - correclty => correctly

Megan Gardner

Comment 5 2020-06-23 18:00:53 PDT

Created attachment 402613 [details] Patch

Megan Gardner

Comment 6 2020-06-23 19:01:06 PDT

Created attachment 402615 [details] Patch

Megan Gardner

Comment 7 2020-06-23 22:29:26 PDT

Created attachment 402622 [details] Patch

Megan Gardner

Comment 8 2020-06-24 11:59:15 PDT

Created attachment 402667 [details] Patch

Megan Gardner

Comment 9 2020-06-24 12:00:08 PDT

Created attachment 402668 [details] Patch

Darin Adler

Comment 10 2020-06-24 12:05:31 PDT

Comment on attachment 402668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402668&action=review Not that you need to wait for me specifically, but I am planning to review once there’s a patch that passes all EWS. > Source/WebCore/editing/DeleteSelectionCommand.cpp:191 > bool endPositonIsBlankParagraph = isBlankParagraph(visibleEnd); Hey, look at that: "positon"

Megan Gardner

Comment 11 2020-06-24 12:16:13 PDT

Created attachment 402672 [details] Patch

Megan Gardner

Comment 12 2020-06-24 14:33:03 PDT

Created attachment 402686 [details] Patch

Megan Gardner

Comment 13 2020-06-24 15:33:39 PDT

Created attachment 402691 [details] Patch

Megan Gardner

Comment 14 2020-06-24 16:20:06 PDT

Created attachment 402695 [details] Patch