[Webkit-unassigned] [Bug 62092] setting innerText to an empty string on editable div loses focus

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=62092





--- Comment #57 from Ryosuke Niwa <rniwa at webkit.org>  2011-09-25 20:14:10 PST ---
(From update of attachment 108615)
View in context: https://bugs.webkit.org/attachment.cgi?id=108615&action=review

Please clean up the test code. The rest of the patch looks good to me.

>> Source/WebCore/dom/Position.h:123
>> +    void willRemoveNode(Node*);
> 
> This is going to require a bit of thought.
> 
> It's not good to have this function in a dom class, but not have code to ensure it is called any time a node is removed from its parent. The corresponding function in WebCore::Range is always called by the DOM itself, while this function is only called by editing machinery. There's no easy way to tell that when looking at this class, and in fact it would be reasonable to expect the other approach.
> 
> The question is whether we are redefining Position objects so they automatically update in this fashion, or if this is a function to be called voluntarily only by editing code, and not part of the DOM itself. Either would be OK, but this current version is half and half.
> 
> If this function is meant to be called only by editing machinery, it should probably not be in a class in the dom directory, or at the very least should have editing in its name. Although a function for editing that is baked into a basic class is an anti-pattern. The many editing-related functions in Node are a design mistake.

This is the reason I was talking about PersistenPosition a couple of months ago. FrameSelection, ReplaceSelectionCommand, and a couple other classes really want to make positions persist over DOM mutatuions but we can't do it at the moment because ending/starting positions used for undo/redo shouldn't automatically be updated.

I think it's okay to add this function for now.

> LayoutTests/editing/selection/character-data-mutation.html:63
> +    runTestPairs(function() { test.removeChild(test.firstChild); return 'Removing the parent of startContainer'; }, 0, undefined, true); 

You've inserted a space at the end of line here. Please remove.

> LayoutTests/editing/selection/document-mutation.html:22
> +    var selection = window.getSelection();

It seems like we can make this a global variable.

> LayoutTests/editing/selection/document-mutation.html:26
> +        selection.setBaseAndExtent(test.firstChild.firstChild, 19, test.firstChild.firstChild, 11);

Can we extract as a function like setRange(startContainer, startOffset, endContainer, endOffset) that automatically checks the value of baseIsFirst and call setBaseAndExtent with appropriate arguments?

> LayoutTests/editing/selection/document-mutation.html:37
> +            '" and expected selection is ' + '(' + expectedStartOffset + ', ' + expectedEndOffset + ')\n');

It seems like this can be extracted as a function.

> LayoutTests/editing/selection/editable-div-clear-on-keydown.html:18
> +    eventSender.keyDown("a");

I would do this at the very end so that readers can understand the test logic first.

> LayoutTests/platform/qt/editing/inserting/typing-003-expected.txt:747
>  EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification

With 1116 lines of output (or 747 lines after your patch), I really doubt that there's much benefit in dumping editing delegates in this test. We can't possibly make sense of this much output.

> LayoutTests/platform/qt/editing/selection/regional-indicators-expected.txt:5
> +FAIL document.getSelection().toString() should be 🇯🇵. Was .

This diff doesn't look good. Maybe you're missing some int'l package in your system?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.