Bug 35368

Summary: Frame should use SelectionController's TextGranularity instead of its own
Product: WebKit Reporter: Ojan Vafai <ojan>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, darin, enrica, eric, mitz, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 35314    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
mistmatched editing callbacks none

Description Ojan Vafai 2010-02-24 16:57:32 PST
Frame should use SelectionController's TextGranularity instead of it's own
Comment 1 Ojan Vafai 2010-02-24 17:01:47 PST
Created attachment 49451 [details]
Patch
Comment 2 Sam Weinig 2010-02-24 18:42:10 PST
Your changelog says there is no functionality change but it seems like a lot of tests are changing their results.  Is there a way you can construct this patch such that results don't change.
Comment 3 Ojan Vafai 2010-02-24 19:37:25 PST
All the changes in test results are either extraneous "webViewDidChangeSelection:WebViewDidChangeSelectionNotification" or selection changes from one range to the *same* range, at least according to what editing delegate dumps. I don't really understand why this change affects affects those.

I went through each of these tests and manually verified that the user-visible behavior is the same with and without the patch, hence my comment that there's no functionality change. I came to the conclusion that it's just a quirk of how we dump editing delegate info. That's just a guess though. 

I can spend some time tomorrow trying to understand where the extra notifications are coming from.
Comment 4 Sam Weinig 2010-02-26 08:37:36 PST
Comment on attachment 49451 [details]
Patch

I am going to r-. A patch which claims no change should not change test behavior, especially if it cannot be fully explained.
Comment 5 Ojan Vafai 2010-03-01 15:38:24 PST
Created attachment 49759 [details]
Patch
Comment 6 Alexey Proskuryakov 2010-03-02 20:54:28 PST
With this patch, a shouldChangeSelectedDOMRange delegate method is being called, but a subsequent webViewDidChangeSelection one no longer is. This is quite suspicious, and perhaps it violates what can be considered an invariant in the API.

VisibleSelection is a member if EditCommand, used for restoring selection after undo/redo. This patch looks like it actually changes behavior, so that selection granularity isn't restored. Is this indeed the case?
Comment 7 Ojan Vafai 2010-03-03 18:45:49 PST
(In reply to comment #6)
> With this patch, a shouldChangeSelectedDOMRange delegate method is being
> called, but a subsequent webViewDidChangeSelection one no longer is. This is
> quite suspicious, and perhaps it violates what can be considered an invariant
> in the API.

OK. I'll look into this more.

> VisibleSelection is a member if EditCommand, used for restoring selection after
> undo/redo. This patch looks like it actually changes behavior, so that
> selection granularity isn't restored. Is this indeed the case?

I didn't think about this case. It turns out that our current behavior doesn't match TextEdit anyways. TextEdit, as best I can tell, always uses CharacterGranularity after an undo, but it selects the space that got smart deleted. I've filed a patch at bug 35713 to fix that. Then this problem goes away. :)
Comment 8 Ojan Vafai 2010-03-03 18:46:36 PST
Comment on attachment 49759 [details]
Patch

r- until bug 35713 is resolved and the other issue AP brought up is addressed.
Comment 9 Ojan Vafai 2010-03-04 12:08:02 PST
(In reply to comment #7)
> (In reply to comment #6)
> > With this patch, a shouldChangeSelectedDOMRange delegate method is being
> > called, but a subsequent webViewDidChangeSelection one no longer is. This is
> > quite suspicious, and perhaps it violates what can be considered an invariant
> > in the API.
> 
> OK. I'll look into this more.

This is not currently an invariant of the API. Anytime SelectionController::setSelection early returns, we'd see this mismatch. In this case, this patch hits an early return at line 141 of SelectionController.cpp. Since the granularity is no longer a member of the VisibleSelection, the selection itself is not actually changing.

I'm attaching an example test that also has this mismatch and hits the same early return on a clean trunk build (single click in the same spot twice).
Comment 10 Ojan Vafai 2010-03-04 12:08:44 PST
Created attachment 50046 [details]
mistmatched editing callbacks
Comment 11 Ojan Vafai 2010-03-04 12:09:43 PST
Comment on attachment 49759 [details]
Patch

Marking for review again as the callback concern has been addressed and the undo concern will be addressed once bug 35713 is resolved.
Comment 12 Ojan Vafai 2010-03-04 17:09:20 PST
Comment on attachment 49759 [details]
Patch

I've rolled this patch into the fix for bug 35314 since that patch would remove many of the lines added in this one. I'll close the bug once bug 35314 is resolved.