Bug 195837 - Smart delete for paragraphs.
Summary: Smart delete for paragraphs.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-15 17:35 PDT by Megan Gardner
Modified: 2019-03-21 10:03 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.49 KB, patch)
2019-03-15 17:38 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (6.63 MB, text/plain)
2019-03-15 21:33 PDT, EWS Watchlist
no flags Details
Patch (5.70 KB, patch)
2019-03-18 16:32 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (20.67 KB, patch)
2019-03-18 20:21 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (2.46 MB, application/zip)
2019-03-18 21:24 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews112 for mac-highsierra (2.52 MB, application/zip)
2019-03-18 23:03 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.70 MB, application/zip)
2019-03-18 23:56 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (11.14 MB, application/zip)
2019-03-19 00:20 PDT, EWS Watchlist
no flags Details
Patch (32.73 KB, patch)
2019-03-19 13:04 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (33.78 KB, patch)
2019-03-20 13:45 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (38.53 KB, patch)
2019-03-20 15:10 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (38.51 KB, patch)
2019-03-20 16:04 PDT, Megan Gardner
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (2.45 MB, application/zip)
2019-03-20 17:33 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.85 MB, application/zip)
2019-03-20 18:00 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews117 for mac-highsierra (2.28 MB, application/zip)
2019-03-20 19:23 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews201 for win-future (13.00 MB, application/zip)
2019-03-20 19:56 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.72 MB, application/zip)
2019-03-20 20:08 PDT, EWS Watchlist
no flags Details
Patch (38.47 KB, patch)
2019-03-20 21:00 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (38.47 KB, patch)
2019-03-21 09:40 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2019-03-15 17:35:59 PDT
Smart delete for paragraphs.
Comment 1 Megan Gardner 2019-03-15 17:38:43 PDT
Created attachment 364890 [details]
Patch
Comment 2 EWS Watchlist 2019-03-15 17:40:28 PDT
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.
Comment 3 EWS Watchlist 2019-03-15 21:33:13 PDT
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
Comment 4 EWS Watchlist 2019-03-15 21:33:14 PDT
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
Comment 5 Megan Gardner 2019-03-18 16:32:25 PDT
Created attachment 365088 [details]
Patch
Comment 6 EWS Watchlist 2019-03-18 16:34:46 PDT
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.
Comment 7 Megan Gardner 2019-03-18 20:21:33 PDT
Created attachment 365114 [details]
Patch
Comment 8 Megan Gardner 2019-03-18 20:24:08 PDT
<rdar://problem/48813963>
Comment 9 EWS Watchlist 2019-03-18 21:24:26 PDT
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
Comment 10 EWS Watchlist 2019-03-18 21:24:28 PDT
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
Comment 11 EWS Watchlist 2019-03-18 23:03:23 PDT
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
Comment 12 EWS Watchlist 2019-03-18 23:03:24 PDT
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
Comment 13 EWS Watchlist 2019-03-18 23:56:13 PDT
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
Comment 14 EWS Watchlist 2019-03-18 23:56:14 PDT
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
Comment 15 EWS Watchlist 2019-03-19 00:20:49 PDT
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
Comment 16 EWS Watchlist 2019-03-19 00:20:51 PDT
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
Comment 17 Megan Gardner 2019-03-19 13:04:44 PDT
Created attachment 365216 [details]
Patch
Comment 18 Ryosuke Niwa 2019-03-19 17:35:24 PDT
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.
Comment 19 Megan Gardner 2019-03-20 13:40:56 PDT
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.
Comment 20 Megan Gardner 2019-03-20 13:45:18 PDT
Created attachment 365393 [details]
Patch
Comment 21 Ryosuke Niwa 2019-03-20 14:39:24 PDT
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
Comment 22 Megan Gardner 2019-03-20 15:10:15 PDT
Created attachment 365409 [details]
Patch
Comment 23 Megan Gardner 2019-03-20 16:04:56 PDT
Created attachment 365425 [details]
Patch
Comment 24 EWS Watchlist 2019-03-20 17:33:12 PDT
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
Comment 25 EWS Watchlist 2019-03-20 17:33:14 PDT
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
Comment 26 EWS Watchlist 2019-03-20 18:00:21 PDT
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
Comment 27 EWS Watchlist 2019-03-20 18:00:23 PDT
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
Comment 28 EWS Watchlist 2019-03-20 19:23:46 PDT
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
Comment 29 EWS Watchlist 2019-03-20 19:23:48 PDT
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
Comment 30 EWS Watchlist 2019-03-20 19:56:36 PDT
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
Comment 31 EWS Watchlist 2019-03-20 19:56:47 PDT
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
Comment 32 EWS Watchlist 2019-03-20 20:08:29 PDT
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
Comment 33 EWS Watchlist 2019-03-20 20:08:33 PDT
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
Comment 34 Megan Gardner 2019-03-20 21:00:52 PDT
Created attachment 365478 [details]
Patch
Comment 35 Megan Gardner 2019-03-21 09:40:31 PDT
Created attachment 365559 [details]
Patch for landing
Comment 36 WebKit Commit Bot 2019-03-21 10:03:23 PDT
Comment on attachment 365559 [details]
Patch for landing

Clearing flags on attachment: 365559

Committed r243296: <https://trac.webkit.org/changeset/243296>
Comment 37 WebKit Commit Bot 2019-03-21 10:03:25 PDT
All reviewed patches have been landed.  Closing bug.