Bug 194880 - Smart Insert for paragraphs.
Summary: Smart Insert 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-02-20 16:05 PST by Megan Gardner
Modified: 2019-03-21 11:20 PDT (History)
5 users (show)

See Also:


Attachments
Patch (12.58 KB, patch)
2019-02-20 16:10 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (629.56 KB, application/zip)
2019-02-20 16:51 PST, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-highsierra (353.91 KB, application/zip)
2019-02-20 16:59 PST, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-highsierra (345.12 KB, application/zip)
2019-02-20 17:06 PST, Build Bot
no flags Details
Archive of layout-test-results from ews200 for win-future (2.03 MB, application/zip)
2019-02-20 18:24 PST, Build Bot
no flags Details
Patch (6.30 KB, patch)
2019-02-22 12:45 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (41.32 KB, patch)
2019-02-25 18:09 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (41.45 KB, patch)
2019-02-26 14:45 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (41.54 KB, patch)
2019-02-26 18:11 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (10.80 MB, application/zip)
2019-02-26 22:12 PST, Build Bot
no flags Details
Patch (42.05 KB, patch)
2019-02-27 15:49 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.85 MB, application/zip)
2019-02-27 17:57 PST, Build Bot
no flags Details
Patch (48.86 KB, patch)
2019-03-12 16:17 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.80 MB, application/zip)
2019-03-12 19:27 PDT, Build Bot
no flags Details
Patch (46.75 KB, patch)
2019-03-12 19:38 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.73 MB, application/zip)
2019-03-12 21:36 PDT, Build Bot
no flags Details
Patch (47.11 KB, patch)
2019-03-13 09:16 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (2.56 MB, application/zip)
2019-03-13 10:12 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.50 MB, application/zip)
2019-03-13 10:39 PDT, Build Bot
no flags Details
Patch (47.37 KB, patch)
2019-03-13 10:42 PDT, Megan Gardner
rniwa: review+
Details | Formatted Diff | Diff
Patch (49.34 KB, patch)
2019-03-14 15:17 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (47.76 KB, patch)
2019-03-15 16:37 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (47.62 KB, patch)
2019-03-18 13:12 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.98 MB, application/zip)
2019-03-18 15:25 PDT, Build Bot
no flags Details
Patch for landing (47.62 KB, patch)
2019-03-18 18:17 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-02-20 16:05:04 PST
Smart Insert for paragraphs.
Comment 1 Megan Gardner 2019-02-20 16:10:50 PST
Created attachment 362561 [details]
Patch
Comment 2 Build Bot 2019-02-20 16:15:05 PST
Attachment 362561 [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 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Build Bot 2019-02-20 16:51:43 PST
Comment on attachment 362561 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 4 Build Bot 2019-02-20 16:51:45 PST
Created attachment 362570 [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 5 Build Bot 2019-02-20 16:59:41 PST
Comment on attachment 362561 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 6 Build Bot 2019-02-20 16:59:42 PST
Created attachment 362571 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 7 Build Bot 2019-02-20 17:06:14 PST
Comment on attachment 362561 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 8 Build Bot 2019-02-20 17:06:16 PST
Created attachment 362575 [details]
Archive of layout-test-results from ews113 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 9 Ryosuke Niwa 2019-02-20 17:20:34 PST
Comment on attachment 362561 [details]
Patch

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

> Source/WebCore/editing/CompositeEditCommand.h:155
> +    void insertParagraphSeparatorAtPosition(const Position&, bool useDefaultParagraphElement = false, bool pasteBlockqutoeIntoUnquotedArea = false);

We don't want to do this. We want to update the selection & have insertParagraphSeparator adjust the selection as it mutates DOM.
Otherwise, we can end up with an orphaned position (i.e. position is no long in document).

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1273
> +        addNewLinesForSmartReplace();

We definitely want to have a separate check for this feature.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1348
> +    Node* endNode = endUpstream.computeNodeBeforePosition();
> +    int endOffset = is<Text>(endNode) ? downcast<Text>(*endNode).length() : 0;
> +    if (endUpstream.anchorType() == Position::PositionIsOffsetInAnchor) {
> +        endNode = endUpstream.containerNode();
> +        endOffset = endUpstream.offsetInContainerNode();
> +    }

This doesn't look right. Why are getting these node & offset?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1356
> +    Position startDownstream = startOfInsertedContent.deepEquivalent().downstream();
> +    Node* startNode = startDownstream.computeNodeAfterPosition();
> +    unsigned startOffset = 0;
> +    if (startDownstream.anchorType() == Position::PositionIsOffsetInAnchor) {
> +        startNode = startDownstream.containerNode();
> +        startOffset = startDownstream.offsetInContainerNode();
> +    }

Ditto.
Comment 10 Build Bot 2019-02-20 18:24:30 PST
Comment on attachment 362561 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 11 Build Bot 2019-02-20 18:24:33 PST
Created attachment 362581 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 12 Ryosuke Niwa 2019-02-21 15:41:52 PST
Comment on attachment 362561 [details]
Patch

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

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1366
> +        if (isEndOfLine(positionBeforeStart))

Doesn't this return true even when positionBeforeStart is on an empty line?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1373
> +        if (isStartOfLine(positionAfterEnd))

Ditto.
Comment 13 Megan Gardner 2019-02-22 12:45:19 PST
Created attachment 362752 [details]
Patch
Comment 14 Build Bot 2019-02-22 12:49:14 PST
Attachment 362752 [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 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Ryosuke Niwa 2019-02-22 18:46:41 PST
Comment on attachment 362752 [details]
Patch

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

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1272
> +    

Nit: whitespace.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1275
> +    

Ditto.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1337
> +    // <MMG> add a check for iOS

You mean IMG?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1338
> +    

Nit: whitespace.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1355
> +    

Nit: Whitespace.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1358
> +    Position endUpstream = endOfInsertedContent.deepEquivalent().upstream();
> +    
> +    Position startDownstream = startOfInsertedContent.deepEquivalent().downstream();

These things aren't used anywhere. Remove?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1359
> +    

Nit: whitespace.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1361
> +    

Ditto.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1364
> +    

Ditto.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1369
> +            applyCommandToComposite(SetSelectionCommand::create(VisibleSelection(startOfInsertedContent), FrameSelection::defaultSetSelectionOptions()));

Just call setEndingSelection instead.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1378
> +            applyCommandToComposite(SetSelectionCommand::create(VisibleSelection(endOfInsertedContent), FrameSelection::defaultSetSelectionOptions()));

Ditto.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1382
> +}

We need to restore endingSelection here in the case we've inserted paragraphs above.
Comment 16 Megan Gardner 2019-02-25 18:09:19 PST
Created attachment 362948 [details]
Patch
Comment 17 Megan Gardner 2019-02-26 14:45:33 PST
Created attachment 363023 [details]
Patch
Comment 18 Ryosuke Niwa 2019-02-26 16:29:16 PST
Comment on attachment 363023 [details]
Patch

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

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1340
> +    Element* textControl = enclosingTextFormControl(positionAtStartOfInsertedContent().deepEquivalent());

Do we really need this check? Also use auto?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1359
> +    Position endUpstream = endOfInsertedContent.deepEquivalent().upstream();
> +    
> +    Position startDownstream = startOfInsertedContent.deepEquivalent().downstream();

These two variables are never used. Remove?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1372
> +            setEndingSelection(VisibleSelection(startOfInsertedContent));

This is correct use of setEndingSelection but the conversion to VisibleSelection is implicit
so you can just do setEndingSelection(startOfInsertedContent).

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1382
> +            setEndingSelection(VisibleSelection(endOfInsertedContent));

Ditto.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1388
> +    }

As I commented in the previous patch, we probably need to restore ending selection here in the case we've modified above.
Comment 19 Megan Gardner 2019-02-26 18:11:03 PST
Created attachment 363050 [details]
Patch
Comment 20 Megan Gardner 2019-02-26 18:17:11 PST
(In reply to Ryosuke Niwa from comment #18)
> Comment on attachment 363023 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363023&action=review
> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1340
> > +    Element* textControl = enclosingTextFormControl(positionAtStartOfInsertedContent().deepEquivalent());
> 
> Do we really need this check? Also use auto?

The check for smart paste for spaces checks to make sure this isn't a password field. I figured we just don't want any input fields at all, but I can just check for password fields specifically, it just seems like we just shouldn't even even try to put newlines of any form in an input field.

> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1359
> > +    Position endUpstream = endOfInsertedContent.deepEquivalent().upstream();
> > +    
> > +    Position startDownstream = startOfInsertedContent.deepEquivalent().downstream();
> 
> These two variables are never used. Remove?
> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1372
> > +            setEndingSelection(VisibleSelection(startOfInsertedContent));
> 
> This is correct use of setEndingSelection but the conversion to
> VisibleSelection is implicit
> so you can just do setEndingSelection(startOfInsertedContent).
> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1382
> > +            setEndingSelection(VisibleSelection(endOfInsertedContent));
> 
> Ditto.
> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:1388
> > +    }
> 
> As I commented in the previous patch, we probably need to restore ending
> selection here in the case we've modified above.

Ok, that's what I thought I was doing with " m_endOfInsertedContent = newEnd.deepEquivalent();" Is that now how to set the end of the selection? This is what I thought was doing the ending selection update in the insert smart spaces code... I'm not updating the start selection, though, that is probably something that I should do as well. Or is this not the right way to do this anymore?
Comment 21 Build Bot 2019-02-26 22:12:33 PST
Comment on attachment 363050 [details]
Patch

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

New failing tests:
editing/pasteboard/5065605.html
editing/pasteboard/4076267-3.html
editing/pasteboard/do-not-copy-unnecessary-styles.html
editing/pasteboard/paste-text-013.html
editing/pasteboard/paste-list-003.html
editing/pasteboard/paste-text-012.html
editing/pasteboard/paste-list-002.html
editing/pasteboard/pasting-into-p-should-not-nest-p.html
editing/pasteboard/paste-text-005.html
editing/pasteboard/preserve-line-break-at-end-of-pasted-content.html
editing/pasteboard/paste-text-016.html
editing/pasteboard/avoid-copying-body-with-background.html
editing/pasteboard/paste-list-001.html
editing/pasteboard/paste-text-011.html
editing/pasteboard/paste-text-006.html
editing/pasteboard/paste-text-014.html
editing/pasteboard/paste-text-015.html
editing/pasteboard/copy-paste-pre-line-content.html
editing/pasteboard/paste-text-007.html
Comment 22 Build Bot 2019-02-26 22:12:35 PST
Created attachment 363070 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 23 Megan Gardner 2019-02-27 15:49:55 PST
Created attachment 363148 [details]
Patch
Comment 24 Build Bot 2019-02-27 17:57:33 PST
Comment on attachment 363148 [details]
Patch

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

New failing tests:
editing/pasteboard/paste-text-016.html
editing/pasteboard/paste-text-007.html
editing/pasteboard/paste-text-006.html
editing/pasteboard/paste-text-005.html
editing/pasteboard/paste-text-014.html
Comment 25 Build Bot 2019-02-27 17:57:35 PST
Created attachment 363170 [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 26 Ryosuke Niwa 2019-03-01 12:39:41 PST
Comment on attachment 363148 [details]
Patch

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

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1355
> +static bool isNewLine(VisiblePosition &p)

Please don't use one letter variable names such as p.
Also, this is wrong style. & should be next to VisiblePosition.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1359
> +    if (isStartOfLine(p) && (startOfLine(p.next()) != startOfLine(p)))
> +        return true;
> +    return false;

It seems like should just do this in single line? There is no need for a if statement.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1367
> +    bool isFullParagraph = isStartOfParagraph(startOfInsertedContent) && isEndOfParagraph(endOfInsertedContent);

What does this check do? I don't get what you mean by "full paragraph".
We should either rid of this confusing local variable or come up with more a descriptive variable name instead.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1378
> +        if (!isNewLine(positionBeforeStart) && !isNewLine(startOfInsertedContent) && isEndOfLine(positionBeforeStart) && !isEndOfEditableOrNonEditableContent(positionAfterEnd) && !isEndOfEditableOrNonEditableContent(endOfInsertedContent)) {

It seems that we should also check whether we're at the start of the editable region or not?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1383
> +            VisiblePosition newStart = startOfInsertedContent.previous(CannotCrossEditingBoundary, &reachedBoundaryStart);
> +            if (!reachedBoundaryStart)
> +                m_startOfInsertedContent = newStart.deepEquivalent();

A better change would be to use endingSelection().deepEquivalent().
The trouble here is that startOfInsertedContent may not be in the document after executing insertParagraphSeparator().

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1387
> +    reachedBoundaryEnd = false;

Why is it okay to not re-set reachedBoundaryStart here?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1391
> +        if (isStartOfLine(positionAfterEnd) && !isEndOfLine(positionAfterEnd) && !isNewLine(positionAfterEnd) && !isNewLine(endOfInsertedContent)) {

Ditto.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1396
> +            VisiblePosition newEnd = endOfInsertedContent.next(CannotCrossEditingBoundary, &reachedBoundaryEnd);
> +            if (!reachedBoundaryEnd)
> +                m_endOfInsertedContent = newEnd.deepEquivalent();

Ditto about restoring m_endOfInsertedContent from endingSelection instead.
Comment 27 Megan Gardner 2019-03-12 16:17:49 PDT
Created attachment 364468 [details]
Patch
Comment 28 Ryosuke Niwa 2019-03-12 17:11:36 PDT
Comment on attachment 364468 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).

Nit: This line should appear before the description but after the bug URL.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:781
> +static bool isNewLine(VisiblePosition& position)

I'd prefer calling this isBlankLine.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:783
> +    return isStartOfLine(position) && (startOfLine(position.next()) != startOfLine(position));

Nit: We don't need parenthesis here.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1085
> +    if (!reachedBoundaryStart && !reachedBoundaryEnd && isNewLine(visibleInsertionPos) && isEndOfLine(positionBeforeInsertion) && isStartOfLine(positionAfterInsertion))
> +        hasBlankLinesBetweenParagraphs = true;

Let's extract this as a helper function like isBlankLineBetweenParagraphs.
Also, why don't we define a local bool like hasLineBeforePosition = isEndOfLine(positionBeforeInsertion)
so as to clarify the intent of the last two checks.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1144
> +        isList = true;
> +    }
>      else {

Nit: } else {

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1390
> +            VisiblePosition newStart = startOfInsertedContent.previous(CannotCrossEditingBoundary, &reachedBoundaryStart);

We still need to use VisiblePosition { endingSelection() }.previous() here. startOfInsertedContent could no longer be in the document.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1405
> +            VisiblePosition newEnd = endOfInsertedContent.next(CannotCrossEditingBoundary, &reachedBoundaryEnd);

Ditto; endingSelection().

> LayoutTests/ChangeLog:7
> +

Please mention which test is testing pasting into a list.
In particular, we need to make sure we cover pasting into OL, UL, and between LI's
as well as nested list items.

We also need a test case where we paste a list.

Also we need tests for pasting into table cells. Let's make sure we have some tests.
Comment 29 Build Bot 2019-03-12 19:27:57 PDT
Comment on attachment 364468 [details]
Patch

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

New failing tests:
editing/pasteboard/paste-text-016.html
Comment 30 Build Bot 2019-03-12 19:27:59 PDT
Created attachment 364495 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 31 Megan Gardner 2019-03-12 19:38:21 PDT
Created attachment 364496 [details]
Patch
Comment 32 Build Bot 2019-03-12 21:36:02 PDT
Comment on attachment 364496 [details]
Patch

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

New failing tests:
editing/pasteboard/smart-paste-paragraph-003.html
Comment 33 Build Bot 2019-03-12 21:36:04 PDT
Created attachment 364509 [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 34 Megan Gardner 2019-03-13 09:16:47 PDT
Created attachment 364539 [details]
Patch
Comment 35 Build Bot 2019-03-13 10:12:41 PDT
Comment on attachment 364539 [details]
Patch

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

New failing tests:
editing/pasteboard/smart-paste-paragraph-003.html
Comment 36 Build Bot 2019-03-13 10:12:43 PDT
Created attachment 364542 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 37 Build Bot 2019-03-13 10:39:18 PDT
Comment on attachment 364539 [details]
Patch

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

New failing tests:
editing/pasteboard/smart-paste-paragraph-003.html
Comment 38 Build Bot 2019-03-13 10:39:20 PDT
Created attachment 364547 [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 39 Megan Gardner 2019-03-13 10:42:48 PDT
Created attachment 364548 [details]
Patch
Comment 40 Megan Gardner 2019-03-13 13:17:10 PDT
<rdar://problem/48813873>
Comment 41 Ryosuke Niwa 2019-03-13 20:45:40 PDT
Comment on attachment 364548 [details]
Patch

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

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:933
> +static bool hasBlankLineBetweenParagraphs(Position &position)

Nit: & should be next to Position as in Position&.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:937
> +    VisiblePosition visiblePosition(position);

Nit: Use { ~ }?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:939
> +    VisiblePosition positionBeforeInsertion = visiblePosition.previous(CannotCrossEditingBoundary, &reachedBoundaryStart);
> +    VisiblePosition positionAfterInsertion = visiblePosition.next(CannotCrossEditingBoundary, &reachedBoundaryStart);

It's strange to call this positionBeforeInsertion & positionAfterInsertion since we're not talking about any insertion point here.
How about previousPosition & nextPosition?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:941
> +    

Nit: whitespace.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1137
> +    bool isList = false;

isList is a bit ambiguous here. What is a list?
I think we need to give it a more describe name like isPastingIntoList or isInsertingIntoList.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1357
> +    

Nit: whitespace.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1360
> +    

Nit: whitespace.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1362
> +    if (is<HTMLInputElement>(textControl))

Do we really need this check? hasBlankLinesBetweenParagraphs should be false since there won't be an extra line no?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1364
> +    

Nit: whitespace.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1378
> +    bool isParagraph = isStartOfParagraph(startOfInsertedContent) && isEndOfParagraph(endOfInsertedContent);

What is a paragraph? How about something like isPastedContentEntireParagraphs?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1380
> +    // if we aren't pasting a paragraph, no need to attempt to insert newlines.

Nit: Capitalize "if".

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1390
> +        if (!isBlankLine(positionBeforeStart) && !isBlankLine(startOfInsertedContent) && isEndOfLine(positionBeforeStart) && !isEndOfEditableOrNonEditableContent(positionAfterEnd) && !isEndOfEditableOrNonEditableContent(endOfInsertedContent)) {

It seems that !isEndOfEditableOrNonEditableContent(positionAfterEnd) should be checked for the end case instead??

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1393
> +            VisiblePosition newStart = VisiblePosition(endingSelection().start()).previous(CannotCrossEditingBoundary, &reachedBoundaryStart);

Use endingSelection().visibleStart() instead. Also, use auto?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1398
> +    

Nit: Whitespace.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1403
> +    

Nit: Whitespace.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1405
> +        if (isStartOfLine(positionAfterEnd) && !isEndOfLine(positionAfterEnd) && !isBlankLine(positionAfterEnd) && !isBlankLine(endOfInsertedContent)) {

Can we order the checks in the same order as the start case?
Also, don't we need to check !isEndOfEditableOrNonEditableContent(positionAfterEnd) here?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1408
> +            if (!reachedBoundaryEnd)

reachedBoundaryStart check here seems bogus. Remove?

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1409
> +                m_endOfInsertedContent = endingSelection().start();

Use visibleEnd() maybe? visibleStart() is okay too.

> LayoutTests/editing/pasteboard/smart-paste-paragraph-001.html:55
> +async function editingTest() {

Why not enable iOS editing behavior manually so that we can always test this:
internals.settings.setEditingBehavior("ios");

Ditto for all other tests.
Comment 42 Megan Gardner 2019-03-14 15:17:09 PDT
Created attachment 364699 [details]
Patch
Comment 43 Megan Gardner 2019-03-15 16:37:42 PDT
Created attachment 364876 [details]
Patch
Comment 44 Ryosuke Niwa 2019-03-15 22:10:29 PDT
Comment on attachment 364876 [details]
Patch

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

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:1144
>      if ((isListHTMLElement(refNode.get()) || (isLegacyAppleStyleSpan(refNode.get()) && isListHTMLElement(refNode->firstChild())))
> -        && blockStart && blockStart->renderer()->isListItem())
> +        && blockStart && blockStart->renderer()->isListItem()) {

Why don't we do:
bool isInsertingIntoList = (isListHTMLElement(refNode.get()) || ...;
if (isInsertingIntoList)
    refNode = ...;
instead?
Comment 45 Megan Gardner 2019-03-18 13:12:06 PDT
Created attachment 365049 [details]
Patch
Comment 46 Build Bot 2019-03-18 15:25:44 PDT
Comment on attachment 365049 [details]
Patch

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

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html
Comment 47 Build Bot 2019-03-18 15:25:46 PDT
Created attachment 365076 [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 48 Megan Gardner 2019-03-18 18:17:52 PDT
Created attachment 365097 [details]
Patch for landing
Comment 49 WebKit Commit Bot 2019-03-18 18:56:45 PDT
Comment on attachment 365097 [details]
Patch for landing

Clearing flags on attachment: 365097

Committed r243124: <https://trac.webkit.org/changeset/243124>
Comment 50 WebKit Commit Bot 2019-03-18 18:56:47 PDT
All reviewed patches have been landed.  Closing bug.