Bug 35368 – Frame should use SelectionController's TextGranularity instead of its own

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

[Ojan Vafai]

Frame should use SelectionController's TextGranularity instead of it's own

[Ojan Vafai]

Created attachment 49451 [details]
Patch

[Sam Weinig]

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.

[Ojan Vafai]

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.

[Sam Weinig]

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.

[Ojan Vafai]

Created attachment 49759 [details]
Patch

[Alexey Proskuryakov]

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?

[Ojan Vafai]

(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. :)

[Ojan Vafai]

Comment on attachment 49759 [details]
Patch

r- until bug 35713 is resolved and the other issue AP brought up is addressed.

[Ojan Vafai]

(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).

[Ojan Vafai]

Created attachment 50046 [details]
mistmatched editing callbacks

[Ojan Vafai]

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.

[Ojan Vafai]

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.