Bug 151442 - Selection.deleteFromDocument should not leave a selection character
Summary: Selection.deleteFromDocument should not leave a selection character
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk, LayoutTestFailure
Depends on: 151488
Blocks:
  Show dependency treegraph
 
Reported: 2015-11-19 04:16 PST by Carlos Garcia Campos
Modified: 2016-04-15 01:53 PDT (History)
1 user (show)

See Also:


Attachments
Patch (9.61 KB, patch)
2015-11-19 04:22 PST, Carlos Garcia Campos
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-yosemite (843.09 KB, application/zip)
2015-11-19 05:14 PST, Build Bot
no flags Details
Updated patch (10.46 KB, patch)
2015-11-20 09:00 PST, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff
Rebased patch for landing (10.46 KB, patch)
2016-04-15 01:13 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2015-11-19 04:16:57 PST
This is what happens with test imported/blink/editing/selection/deleteFromDocument-crash.html. It first run selectAll in the focused text area, and removeFromDocument() leaves the last character of the selection in the text area. This is what is causing a failure in GTK+ and win ports. The reason why it passes for mac is because the mac editor behaviour considers selections as not directional. For non directional selections, FrameSelection::willBeModified() when direction is backwards switches the selection base extent. So, when the caret is moved once character backwards in the case of mac, the cursor position is 0 and it doesn't have any effect. For the other ports, the position is the last one (extent). This was fixed in blink r172511, so we could just merge it in WebKit.
Comment 1 Carlos Garcia Campos 2015-11-19 04:22:36 PST
Created attachment 265857 [details]
Patch
Comment 2 Build Bot 2015-11-19 05:14:56 PST
Comment on attachment 265857 [details]
Patch

Attachment 265857 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/449673

New failing tests:
imported/blink/editing/selection/deleteFromDocument-undo-crash.html
Comment 3 Build Bot 2015-11-19 05:14:59 PST
Created attachment 265861 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 4 Darin Adler 2015-11-19 08:46:38 PST
Comment on attachment 265857 [details]
Patch

EWS shows the test deleteFromDocument-undo-crash.html is hitting an assertion on Mac debug build. We’ll need to find out what that assertion is to fix it.
Comment 5 Carlos Garcia Campos 2015-11-19 10:11:34 PST
Yes, I'll check what assert we are hitting.
Comment 6 Carlos Garcia Campos 2015-11-20 03:05:58 PST
It looks like a different issue revealed by the new test. I can reproduce it without the patch. See https://code.google.com/p/chromium/issues/detail?id=504932
Comment 7 Carlos Garcia Campos 2015-11-20 09:00:11 PST
Created attachment 265957 [details]
Updated patch

It skips the new test for Debug.
Comment 8 Darin Adler 2015-11-30 09:52:02 PST
Comment on attachment 265957 [details]
Updated patch

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

> LayoutTests/TestExpectations:455
> +webkit.org/b/151488 [ Debug ] imported/blink/editing/selection/deleteFromDocument-undo-crash.html [ Skip ]

I’d prefer that we investigate this first, and fix this before making the rest of this change. I don’t want to rush to get this change in and knowingly introduce a crash.
Comment 9 Carlos Garcia Campos 2015-11-30 10:47:55 PST
(In reply to comment #8)
> Comment on attachment 265957 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265957&action=review
> 
> > LayoutTests/TestExpectations:455
> > +webkit.org/b/151488 [ Debug ] imported/blink/editing/selection/deleteFromDocument-undo-crash.html [ Skip ]
> 
> I’d prefer that we investigate this first, and fix this before making the
> rest of this change. I don’t want to rush to get this change in and
> knowingly introduce a crash.

Note that this doesn't introduce a crash, but a test that reveals an existing crash. The new test crashes in debug build even without the changes introduced in this patch.
Comment 10 Darin Adler 2015-11-30 10:59:28 PST
(In reply to comment #9)
> Note that this doesn't introduce a crash, but a test that reveals an
> existing crash. The new test crashes in debug build even without the changes
> introduced in this patch.

Still seems that we should fix that bug before making this behavior change, though. Should not be a challenging project and there is no hurry to change this behavior since we have had it for a long time.
Comment 11 Carlos Garcia Campos 2015-11-30 23:59:54 PST
(In reply to comment #10)
> (In reply to comment #9)
> > Note that this doesn't introduce a crash, but a test that reveals an
> > existing crash. The new test crashes in debug build even without the changes
> > introduced in this patch.
> 
> Still seems that we should fix that bug before making this behavior change,
> though. Should not be a challenging project and there is no hurry to change
> this behavior since we have had it for a long time.

This behavior has always been there, but we didn't know it caused a crash (a real one, not an assert, so it happens in release as well) until editing/selection/deleteFromDocument-crash.html was imported from blink. That test crashes for all ports not using the mac editing behavior. This patch also adds editing/selection/deleteFromDocument-undo-crash.html, but because it's a blink merge and the test was part of the commit. So, what I can do, is fixing this real crash for the existing test, and add the new test as part of the bug #151488 with the patch to fix it.
Comment 12 Carlos Garcia Campos 2015-12-01 03:09:42 PST
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Note that this doesn't introduce a crash, but a test that reveals an
> > > existing crash. The new test crashes in debug build even without the changes
> > > introduced in this patch.
> > 
> > Still seems that we should fix that bug before making this behavior change,
> > though. Should not be a challenging project and there is no hurry to change
> > this behavior since we have had it for a long time.
> 
> This behavior has always been there, but we didn't know it caused a crash (a
> real one, not an assert, so it happens in release as well) until
> editing/selection/deleteFromDocument-crash.html was imported from blink.
> That test crashes for all ports not using the mac editing behavior. This
> patch also adds editing/selection/deleteFromDocument-undo-crash.html, but
> because it's a blink merge and the test was part of the commit. So, what I
> can do, is fixing this real crash for the existing test, and add the new
> test as part of the bug #151488 with the patch to fix it.

I'm wrong, I think I was confusing this with other test, this is just a failure, not a crash.
Comment 13 Michael Catanzaro 2016-02-15 13:00:10 PST
Comment on attachment 265957 [details]
Updated patch

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

I see you've opened bug #151488 to address Darin's concerns about the new test crashing in debug builds, so I think this is fine for now, since it fixes a crash in release builds, adds a new test, cleans up an existing test, and matches the behavior of IE/FF/Blink.

> LayoutTests/editing/selection/deleteFromDocument.html:36
> +r.setEnd(span2.firstChild, 3);

Can you explain why you changed this?
Comment 14 Carlos Garcia Campos 2016-04-15 01:13:02 PDT
Created attachment 276467 [details]
Rebased patch for landing
Comment 15 Carlos Garcia Campos 2016-04-15 01:52:04 PDT
(In reply to comment #13)
> Comment on attachment 265957 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265957&action=review
> 
> I see you've opened bug #151488 to address Darin's concerns about the new
> test crashing in debug builds, so I think this is fine for now, since it
> fixes a crash in release builds, adds a new test, cleans up an existing
> test, and matches the behavior of IE/FF/Blink.
> 
> > LayoutTests/editing/selection/deleteFromDocument.html:36
> > +r.setEnd(span2.firstChild, 3);
> 
> Can you explain why you changed this?

No, because I'm not the author of the patch, as the ChangeLog says, this is a merge of Blink r172511
Comment 16 Carlos Garcia Campos 2016-04-15 01:53:39 PDT
Committed r199585: <http://trac.webkit.org/changeset/199585>