Bug 195837 – Smart delete for paragraphs.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Smart delete for paragraphs.

[Megan Gardner]

Created attachment 364890 [details]
Patch

[EWS Watchlist]

Attachment 364890 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 364890 [details]
Patch

Attachment 364890 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11526722

New failing tests:
editing/deleting/5433862-2.html
editing/deleting/5126166.html

[EWS Watchlist]

Created attachment 364913 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Megan Gardner]

Created attachment 365088 [details]
Patch

[EWS Watchlist]

Attachment 365088 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Megan Gardner]

Created attachment 365114 [details]
Patch

[Megan Gardner]

<rdar://problem/48813963>

[EWS Watchlist]

Comment on attachment 365114 [details]
Patch

Attachment 365114 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11558425

New failing tests:
editing/deleting/smart-delete-paragraph-001.html
editing/deleting/smart-delete-paragraph-003.html
editing/deleting/smart-delete-paragraph-002.html
editing/deleting/smart-delete-paragraph-004.html

[EWS Watchlist]

Created attachment 365122 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365114 [details]
Patch

Attachment 365114 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11558927

New failing tests:
editing/deleting/smart-delete-paragraph-001.html
editing/deleting/smart-delete-paragraph-003.html
editing/deleting/smart-delete-paragraph-002.html
editing/deleting/smart-delete-paragraph-004.html

[EWS Watchlist]

Created attachment 365125 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365114 [details]
Patch

Attachment 365114 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11559303

New failing tests:
editing/deleting/smart-delete-paragraph-001.html
editing/deleting/smart-delete-paragraph-003.html
editing/deleting/smart-delete-paragraph-002.html
editing/deleting/smart-delete-paragraph-004.html

[EWS Watchlist]

Created attachment 365127 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365114 [details]
Patch

Attachment 365114 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11559241

New failing tests:
editing/deleting/delete-3959464-fix.html

[EWS Watchlist]

Created attachment 365131 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Megan Gardner]

Created attachment 365216 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 365216 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365216&action=review

I think we need one more iteration before we can land this patch.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:179
> +    

Nit: Whitespace. Also, please share this code with ReplaceSelectionCommand by moving it to VisibleUnits.h/cpp

Also, I've started to think that what we need to be checking is isStartOfParagraph.
X of line really is about soft wrapping, which shouldn't affect smart paragraph replacement / deletion.
So this should really be called isBlankParagraph.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:274
> +        if (document().editingBehavior().shouldSmartInsertDeleteParagraphs()) {

The content of this if statement should be moved to a separate member function.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:275
> +            VisiblePosition visStart(m_upstreamStart);

Use { ~ } to initialize start & end. Also, please don't abbreviate visual as "vis".

> Source/WebCore/editing/DeleteSelectionCommand.cpp:282
> +            bool nextPositionIsBlankLine = isBlankLine(visEnd);

"next position" is ambiguous here because it doesn't specify next to what.
It should probably say endPositonIsBlankLine.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:284
> +            bool isSameElement = unsplittableElementForPosition(visStart.deepEquivalent()) == unsplittableElementForPosition(visEnd.deepEquivalent());

This is not a descriptive name. Should say startAndEndInSameUnsplittableElement.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:285
> +        

Nit: Whitespace.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:290
> +                    pos = VisiblePosition(m_downstreamEnd, VP_DEFAULT_AFFINITY).next().next().deepEquivalent();

No need to specify VP_DEFAULT_AFFINITY. The default argument is VP_DEFAULT_AFFINITY.
We should instead do: endOfParagraph(startOfNextParagraph((m_downstreamEnd)); or something.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:295
> +                m_trailingWhitespace = m_downstreamEnd.trailingWhitespacePosition(VP_DEFAULT_AFFINITY);

We don't care about affinity here because we know m_downstreamEnd is at a blank paragraph.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:300
> +                VisiblePosition visiblePos = VisiblePosition(m_upstreamStart, VP_DEFAULT_AFFINITY).previous().previous();

We should probably be using startOfParagraph(VisiblePosition { m_upstreamStart }.previous())

> Source/WebCore/editing/DeleteSelectionCommand.cpp:301
> +                pos = visiblePos.deepEquivalent();

Please declare a new local variable here. It's confusing to be re-using variable declared above like this.
State dependency like that can lead to surprising bugs when someone else come in & modify code in the future.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:304
> +                m_leadingWhitespace = m_upstreamStart.leadingWhitespacePosition(visiblePos.affinity());

No need to pass in visiblePos.affinity here. We know it's always DOWNSTREAM because we're at a blank paragraph.

[Megan Gardner]

Comment on attachment 365216 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365216&action=review

>> Source/WebCore/editing/DeleteSelectionCommand.cpp:295
>> +                m_trailingWhitespace = m_downstreamEnd.trailingWhitespacePosition(VP_DEFAULT_AFFINITY);
> 
> We don't care about affinity here because we know m_downstreamEnd is at a blank paragraph.

Apparently we need to put something, so default seems as good as any.

[Megan Gardner]

Created attachment 365393 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 365393 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365393&action=review

> Source/WebCore/editing/DeleteSelectionCommand.cpp:175
> +static bool isBlankParagraph(VisiblePosition& position)

We should share this code with ReplaceSelectionCommand as I mentioned earlier.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:280
> +            VisiblePosition visibleStart { m_upstreamStart };

We should put this into a separate member function as I commented earlier.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:283
> +            bool selectionEndIsEndOfContent = endOfEditableContent(visibleEnd) == visibleEnd ? true : false;

Just do: selectionEndIsEndOfContent = endOfEditableContent(visibleEnd) == visibleEnd

> Source/WebCore/editing/DeleteSelectionCommand.cpp:290
> +            if (startAndEndInSameUnsplittableElement && previousPositionIsBlankParagraph && (!selectionEndIsEndOfContent && (endPositonIsBlankParagraph || selectionEndsInParagraphSeperator))) {

Why don't we define some helper local booleans like:
bool hasBlankParagraphAfterEnd = !selectionEndIsEndOfContent && (endPositonIsBlankParagraph || selectionEndsInParagraphSeperator);
to make the condition more obvious?

> Source/WebCore/editing/DeleteSelectionCommand.cpp:296
> +                    position = VisiblePosition(m_downstreamEnd, VP_DEFAULT_AFFINITY).next().deepEquivalent();

No need to specify VP_DEFAULT_AFFINITY. That's the default value.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:302
> +            if (selectionEndIsEndOfContent && previousPositionIsBlankParagraph && selectionEndsInParagraphSeperator) {

We need to check startAndEndInSameUnsplittableElement here as well. Maybe we can add a test?

> Source/WebCore/editing/DeleteSelectionCommand.cpp:304
> +                VisiblePosition visiblePos = endOfParagraph(VisiblePosition { m_upstreamStart }.previous().previous());

I think we should use a more descriptive name like endOfParagraphBeforeStart

[Megan Gardner]

Created attachment 365409 [details]
Patch

[Megan Gardner]

Created attachment 365425 [details]
Patch

[EWS Watchlist]

Comment on attachment 365425 [details]
Patch

Attachment 365425 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11587981

New failing tests:
editing/deleting/smart-delete-paragraph-004.html

[EWS Watchlist]

Created attachment 365447 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365393 [details]
Patch

Attachment 365393 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11588287

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html

[EWS Watchlist]

Created attachment 365454 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365425 [details]
Patch

Attachment 365425 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11589378

New failing tests:
editing/deleting/smart-delete-paragraph-004.html

[EWS Watchlist]

Created attachment 365463 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365425 [details]
Patch

Attachment 365425 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11589941

New failing tests:
editing/deleting/smart-delete-paragraph-004.html

[EWS Watchlist]

Created attachment 365465 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 365425 [details]
Patch

Attachment 365425 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11590262

New failing tests:
editing/deleting/smart-delete-paragraph-004.html

[EWS Watchlist]

Created attachment 365466 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Megan Gardner]

Created attachment 365478 [details]
Patch

[Megan Gardner]

Created attachment 365559 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 365559 [details]
Patch for landing

Clearing flags on attachment: 365559

Committed r243296: <https://trac.webkit.org/changeset/243296>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.