Bug 3982 – webViewDidBeginEditing, webViewDidEndEditing notification methods not called on delegate

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

[Dan Wood]

When you have an editing delegate set on an editable webview, the webViewDidBeginEditing and 
webViewDidEndEditing notification methods don't get called on the delegate.

1) Run my attached modified "blot" application
2) Click on an editable block, notice what editing delegate methods get logged
3) Click outside an editable block; note again what gets logged

Expected: You should see indication in the long of webViewDidBeginEditing: and webViewDidEndEditing:

Actual: You don't, though you see other editing delegate methods get logged.

[Dan Wood]

Created attachment 2943 [details]
Modified BLOT sample app that logs all editing delegate methods

[Dan Wood]

Graham Dennis had these comments which he allowed me to paste in:  [edited for clarity]

Initially, I thought #3982 should be a quick fix, but it turns out that they only did the easy part by 
putting in the support code on the WebKit side, but WebCore doesn't support the notifications. The 
difficulty in fixing this depends on exactly when didBeginEditing/didEndEditing should be called 
(shouldEndEditingInDOMRange works in ToT). Should didBeginEditing be called on the first edit in the 
entire WebKit and didEndEditing be called when focus leaves the WebKit, or should it happen when 
focus leaves an editable block, and a new didBeginEditing be called when a new editable block is 
edited? [Dan -- I'd think the latter: Documentation says "when the user [begins|stops] editing the web 
view"] For the former (and less useful) case, it should be fairly easy. The latter case would require 
modifying the definition of KWQKHTMLPart::respondToChangedContents to give it another argument, 
so it can determine if a new editable block has been edited. The long and the short of it is that this 
could probably be fixed in a reasonable timeframe.

[Justin Garcia]

It seems to me that the didEndEditing notification should be sent after the editing delegate says YES to 
shouldEndEditingInDOMRange.  The documentation for shouldEndEditingInDOMRange says:
This method returns YES if the user should be allowed to end editing webView, NO otherwise. This 
method is invoked when a web view attempts to resign as the first responder. Typically, the range 
argument designates the current selection, although it might not. Use the range argument to help 
determine if the user can end editing. If this method returns YES, webView will end editing and resign 
as the first responder.
However this isn't what WebKit does.  The shouldEndEditingInDOMRange method is called every time an 
editable block loses focus.  Likewise shouldBeginEditingInDOMRange is called every time an editable 
block gains focus.  Also note that shouldEndEditingInDOMRange doesn't consider whether the editable 
block was edited while it was focused.  Is this the behavior developers want?  If so, than implementing 
didBegin/didEnd should be straightforward, just add didBegin/didEnd calls after shouldBegin/
shouldEnd.  If it isn't the behavior we want, we should fix it.

[Justin Garcia]

Darin says that we want these delegate methods to be called when editable blocks gain/lose focus, so 
should be straightforward to add didBeginEditing and didEndEditing.

[Graham Dennis]

Created attachment 5934 [details]
patch

With this patch, webViewDidBeginEditing and webViewDidEndEditing should now be called whenever the focus enters/leaves a contentEditable block. I had to define a whole bunch of functions for this, and I tried to follow the behaviour of shouldBeginEditing where appropriate.

[Justin Garcia]

Comment on attachment 5934 [details]
patch

looks good, r=me

[Justin Garcia]

I have to make a few changes to DumpRenderTree in order to add test coverage for this.

[Justin Garcia]

Created attachment 6324 [details]
patch + changes to allow automated testing

After applying Graham's patch, I noticed that very few of the editing tests' results showed shouldBegin, shouldEnd, didBegin and didEndEditing delegate calls.  They weren't being called because:

They're only called when contenteditable regions gain/lose focus.
Editing tests don't explicitly focus editable regions they are about to do editing in, they simply set selections inside them.
Setting selections inside contenteditable regions normally has the side effect of focusing that region, _except_ when the WebView was isn't focused.  
DumpRenderTree's WebView isn't focused.

Tim and I don't think there's a way to make the WebView focused without also making it visible and inside a frontmost window, which would prevent you from doing useful work while you run the layout tests.  So, I made the two calls made on the bridge into WebHTMLView SPI: 
_setWindowHasFocus, which tells WebCore if the window containing the WebView is key.  This will fire window.onfocus and window.onblur handlers.
_setDisplaysWithFocusAttributes, which tells WebCore that the WebView is in the responder chain, and should therefore do focusy things, like paint the caret, focus halos, and a selection with a normal tint.

Three things to note:
As I mentioned above, setting a selection inside a content editable region has the side effect of focusing that region.  Adele and I don't think that this is the behavior we want.  Discussion on that issue is at 7128.
This makes focus halos and carets show up in the pixel results, hurray.
There are mac line endings in some of my layout tests.  I'm removing them.

[Timothy Hatcher]

Comment on attachment 6324 [details]
patch + changes to allow automated testing

Looks good.

[Timothy Hatcher]

Justin landed this in r12683.

[Timothy Hatcher]

And r12684.

[David Kilzer (:ddkilzer)]

And r12685 (and fixes to layout tests in r12701-r12711?).

[Justin Garcia]

Yes.  I know, worst check in skills ever.