Bug 39798 - No selection notification for editing operation that doesn't change the selection's DOM position
Summary: No selection notification for editing operation that doesn't change the selec...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-05-26 16:43 PDT by Justin Garcia
Modified: 2010-06-18 10:57 PDT (History)
6 users (show)

See Also:


Attachments
patch (25.13 KB, patch)
2010-05-27 13:46 PDT, Justin Garcia
no flags Details | Formatted Diff | Diff
patch (25.24 KB, patch)
2010-06-15 14:55 PDT, Justin Garcia
ojan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Garcia 2010-05-26 16:43:26 PDT
Suppose I do an editing operation that changes: <div style="rtl">^<br></div> to <div style="ltr">^<br></div>.  ApplyStyleCommand will just change the inline style declaration of the div without otherwise touching the DOM, so there is no selection change notification, but we need one because it changes visually (the caret moves from the right to the left).

<rdar://problem/8004202>
Comment 1 Justin Garcia 2010-05-27 13:46:07 PDT
Created attachment 57273 [details]
patch

Patch.  Layout test results for qt and chromium-win will also need to be updated.
Comment 2 Enrica Casucci 2010-06-11 15:44:06 PDT
Justin and I discussed this at length over irc. The patch looks good to me. Just fix the typo in th change log and in the comments in the code. 'effecting' --> 'affecting'
Comment 3 Tony Chang 2010-06-13 17:03:29 PDT
Maybe Ojan can r+?
Comment 4 Ojan Vafai 2010-06-14 18:18:15 PDT
I don't understand this patch too well. Doesn't this mean that we call respondToChangedSelection for every Command we apply? Wouldn't that cause us to start a new kill ring sequence too often (i.e. when the command didn't actually cause the selection to change)?

Also, is there a way to test this, e.g. the rtl/ltr case below?
Comment 5 Justin Garcia 2010-06-15 10:21:05 PDT
The operations "change" the selection, but the fact that the operations don't *change the selection's location in the DOM* is just an implementation detail.  

Note for example that RTL -> LTR *will* result in a call to respondToChangedSelection() if the DOM was:

<div contentEditable="true" style="direction: RTL">^<br></div>

since a new div will need to be created to give the paragraph direction:LTR.  This will change the DOM enough to change the selection's DOM location after the operation.

I don't see any reason why we should start a new kill ring sequence for this DOM but not the one mentioned in the bug.  Same for the change notification.
Comment 6 Justin Garcia 2010-06-15 14:55:54 PDT
Created attachment 58824 [details]
patch

> Also, is there a way to test this, e.g. the rtl/ltr case below?

Not exactly, but editing/style/block-style-001.html demonstrates the bug during a change in text-alignment, which is basically the same thing.

New patch mentions this and fixes the typo that Enrica found.
Comment 7 Ojan Vafai 2010-06-17 16:10:30 PDT
Comment on attachment 58824 [details]
patch

WebCore/editing/Editor.cpp:2894
 +      // For example when you press return in the following (the caret is marked by ^): 
s/effecting/affecting
Comment 8 Justin Garcia 2010-06-18 10:43:22 PDT
http://trac.webkit.org/changeset/61418
Comment 9 WebKit Review Bot 2010-06-18 10:57:29 PDT
http://trac.webkit.org/changeset/61418 might have broken Qt Linux Release