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: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
Attachments: |
|
Description
Megan Gardner
2020-06-23 16:07:08 PDT
Created attachment 402604 [details]
Patch
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. 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 Created attachment 402613 [details]
Patch
Created attachment 402615 [details]
Patch
Created attachment 402622 [details]
Patch
Created attachment 402667 [details]
Patch
Created attachment 402668 [details]
Patch
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" Created attachment 402672 [details]
Patch
Created attachment 402686 [details]
Patch
Created attachment 402691 [details]
Patch
Created attachment 402695 [details]
Patch
Created attachment 402697 [details]
Patch
(My take: we’ll grep for "ositon" separately and fix all those typos. No need to mix it up in this patch.) I agree, I'll fix it in a different patch. Committed r263528: <https://trac.webkit.org/changeset/263528> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402697 [details]. |