Cannot delete last line of Mail Message

[Megan Gardner]

Cannot delete last line of Mail Message

[Megan Gardner]

Created attachment 402604 [details]
Patch

[Megan Gardner]

<rdar://problem/63420928>

[Darin Adler]

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 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]

Created attachment 402613 [details]
Patch

[Megan Gardner]

Created attachment 402615 [details]
Patch

[Megan Gardner]

Created attachment 402622 [details]
Patch

[Megan Gardner]

Created attachment 402667 [details]
Patch

[Megan Gardner]

Created attachment 402668 [details]
Patch

[Darin Adler]

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]

Created attachment 402672 [details]
Patch

[Megan Gardner]

Created attachment 402686 [details]
Patch

[Megan Gardner]

Created attachment 402691 [details]
Patch

[Megan Gardner]

Created attachment 402695 [details]
Patch

[Megan Gardner]

Created attachment 402697 [details]
Patch

[Darin Adler]

(My take: we’ll grep for "ositon" separately and fix all those typos. No need to mix it up in this patch.)

[Megan Gardner]

I agree, I'll fix it in a different patch.

[EWS]

Committed r263528: <https://trac.webkit.org/changeset/263528>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402697 [details].