Bug 39798 – No selection notification for editing operation that doesn't change the selection's DOM position

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Justin Garcia]

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>

[Justin Garcia]

Created attachment 57273 [details]
patch

Patch.  Layout test results for qt and chromium-win will also need to be updated.

[Enrica Casucci]

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'

[Tony Chang]

Maybe Ojan can r+?

[Ojan Vafai]

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?

[Justin Garcia]

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.

[Justin Garcia]

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.

[Ojan Vafai]

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

[Justin Garcia]

http://trac.webkit.org/changeset/61418

[WebKit Review Bot]

http://trac.webkit.org/changeset/61418 might have broken Qt Linux Release