Bug 110742

Created attachment 190024 [details] Patch

Comment on attachment 190024 [details] Patch Attachment 190024 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16749177 New failing tests: editing/selection/caret-after-keypress.html

What do other browsers do? Should the event be async? Cancellable?

(In reply to comment #3) > What do other browsers do? Should the event be async? Cancellable? In MSDN it is said: Synchronous No; Bubbles No; Cancelable No In Mozilla Event reference it is said: Bubbles Yes; Cancelable No In current Firefox release selectionStart/selectionEnd is coherent with input content. In whatwg.org specs it is mentioned that firing event must be queued on input content change (so, implicitly, caret/selection during dispatching will be actual). Also several "input" events could be collapsed (to denote that user stopped typing).

Comment on attachment 190024 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190024&action=review I'm somewhat nervous about changing the timing of this event because of the way copy & paste uses webkitEditableContentChangedEvent and how alternative text controller may depends on events firing in advance but I can't think of bugs that this change will introduce off the top of my head. > LayoutTests/editing/selection/caret-after-keypress-expected.txt:6 > +[] 0 0 > +[a] 1 1 > +[ab] 2 2 Instead of just logging values like this, can we use shouldBe and other functions in js-test-pre.js to make the test more explicit? As is, it's hard to tell when the test is passing or why these outputs ought to be correct.

(In reply to comment #5) > ...and how alternative text controller may depends on events firing in advance but I can't think of bugs that this change will introduce off the top of my head. IMHO controller couldn't depend on receiving notification "in advance" because in other cases it receives notifications after selection is fixed (known cases are: first keypress after input is focused; user deleted char in input). As a best effort we could implement deferred "onInput" notifications...

Comment on attachment 190024 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190024&action=review >> LayoutTests/editing/selection/caret-after-keypress-expected.txt:6 >> +[ab] 2 2 > > Instead of just logging values like this, can we use shouldBe and other functions in js-test-pre.js to make the test more explicit? > As is, it's hard to tell when the test is passing or why these outputs ought to be correct. Fixed.

Created attachment 190922 [details] Patch

Comment on attachment 190922 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190922&action=review > LayoutTests/editing/selection/caret-after-keypress.html:46 > + test.addEventListener("input", step); I really don't understand why we need to make this test asynchronous given that input event fires synchronously. We can run all the steps synchronously.

(In reply to comment #9) > I really don't understand why we need to make this test asynchronous given that input event fires synchronously. > We can run all the steps synchronously. OK, I'll make test synchronous. But as I mentioned, in HTML5 spec it is not yet determined if "input" event is synchronous or asynchronous. So making test asynchronous shields us from details of implementation that could change.

Created attachment 191167 [details] Patch

Comment on attachment 191167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191167&action=review > LayoutTests/editing/selection/caret-after-keypress-expected.txt:12 > +PASS 0 > +PASS 1 > +PASS 2 > +PASS 3 > +PASS 4 > +PASS 5 > +PASS 6 I'm sorry but this output is not exactly helpful. It doesn't tell me what each test case is testing and why they're passing. > LayoutTests/editing/selection/caret-after-keypress.html:27 > + assertEquals(test.value, output.join(""), "value"); > + assertEquals(test.selectionStart, output.length, "selectionStart"); > + assertEquals(test.selectionEnd, output.length, "selectionEnd"); It seems like all we need to do is to use shouldBe instead of assertEquals. > LayoutTests/editing/selection/caret-after-keypress.html:28 > + testPassed("" + pass++); This is going to output PASS regardless of whether tests have passed or not. > LayoutTests/editing/selection/caret-after-keypress.html:34 > + var key = input.shift(); > + if (key === backSpace) > + output.pop(); > + else > + output.push(key); This makes it really hard to understand what's happening in the test. I would have preferred to hard code the expected output instead. It's very important for tests to be obvious and self-evidently correct. Unfortunately, this test isn't so self-evident to me and I think we can do better.

Comment on attachment 191167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191167&action=review >> LayoutTests/editing/selection/caret-after-keypress.html:27 >> + assertEquals(test.selectionEnd, output.length, "selectionEnd"); > > It seems like all we need to do is to use shouldBe instead of assertEquals. OK >> LayoutTests/editing/selection/caret-after-keypress.html:28 >> + testPassed("" + pass++); > > This is going to output PASS regardless of whether tests have passed or not. oops. fixed. >> LayoutTests/editing/selection/caret-after-keypress.html:34 >> + output.push(key); > > This makes it really hard to understand what's happening in the test. I would have preferred to hard code the expected output instead. > It's very important for tests to be obvious and self-evidently correct. > Unfortunately, this test isn't so self-evident to me and I think we can do better. Done

Created attachment 192444 [details] Patch

Comment on attachment 192444 [details] Patch Any news?

Comment on attachment 192444 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192444&action=review > Source/WebCore/ChangeLog:3 > + [Editor] selectionStart/selectionEnd return "obsolete" values when requested during "input" event. We don't prefix a bug with "[Editor]". > Source/WebCore/ChangeLog:7 > + This patch changes the timing at which webkitEditableContentChangedEvent is fired. You should explain why that results in "input" event being fired at the right time.

Comment on attachment 192444 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=192444&action=review > Source/WebCore/ChangeLog:3 > + [Editor] selectionStart/selectionEnd return "obsolete" values when requested during "input" event. We don't prefix a bug with "[Editor]". > Source/WebCore/ChangeLog:7 > + This patch changes the timing at which webkitEditableContentChangedEvent is fired. You should explain why that results in "input" event being fired at the right time.

Analysis: after key-press there are 2 possibilities 1) We recently focused input, so new CompositeEditCommand is created and applied. 2) We have done some editing already, so insertion command is appended. Here are two scenarios in time/nesting cut. First (good) scenario. keypress event dispatching { textInput event dispatching { CompositeEditCommand::apply creates scope { Editor::appliedEditing invoked { dispatchEditableContentChangedEvents invoked { webkitEditableContentChanged dispatched { input event enqueued (because it is scoped event) } } * FrameSelection::setSelection invoked } } * input event is dispatched because scope is closed } } Second (bad) scenario. keypress event dispatching { textInput event dispatching { Editor::appliedEditing invoked { dispatchEditableContentChangedEvents invoked { webkitEditableContentChanged dispatched { * input event dispatched (no scope this time) } } * FrameSelection::setSelection invoked (too late) } } } So I see 3 possible solutions 1) Invoke dispatchEditableContentChangedEvents after setSelection (but you are right, this should be done for all 3 methods: applied/unapplied/reapplied editing). 2) Create scope in those 3 methods; actually this will save order of execution of webkitEditableContentChanged and setSelection and just pull out dispatching of input event. 3) Place more coarse scope (several stack levels up), but we can't predict how it will affect execution. Overall (2) solution looks like least invasive.

Why don't we have EditCommand in the stack in the second scenario?

As I said earlier 2 paths are "create composition" and "append to composition". Details. Both paths has common point: TypingCommand::insertText(document, text, selection, options, compositionType) Condition "if (lastTypingCommandIfStillOpenForTyping(...))" splits them. First (good) scenarion we create typing command, and then: TextInsertionBaseCommand::applyTextInsertionCommand applyCommand [CompositeEditCommand.cpp:170] * CompositeEditCommand::apply TypingCommand::doApply = TypingCommand::insertText = forEachLineInString [TextInsertionBaseCommand.h:61] = TypingCommandLineOperation::operator() = TypingCommand::insertTextRunWithoutNewlines = TypingCommand::typingAddedToOpenCommand = Editor::appliedEditing "*" is where scope create. Second scenario is shorter: it consists only of lines marked with "=". So the answer is: we do not apply command because we do not create one.

to Ryosuke Niwa: any thoughts how this should be fixed?

We should fix this but I don't think the patch posted here is correct.

(In reply to comment #22) > We should fix this but I don't think the patch posted here is correct. Surely. Earlier I've proposed 3 patch ideas. Personally I like this one: create EventQueueScope in 3 Editor methods: appliedEditing, unappliedEditing, reappliedEditing. This is least invasive solution, because it will affect only on time when "input" event is dispatched (in case it is fired). It is up to you to choose the best way to fix. Just post a comment with decision and I'll implement corresponding patch.

If we were to use the event scope to resolve this bug, I'd instantiate the scope object in TypingCommand::insertText and other static functions of TypingCommand since the event scope was invented to delay firing of mutation events. On the other hand, it's strange that the event scope is needed for correctness of ordering of events. Ordering of the events should be correct regardless of whether we define the scope or not.

(In reply to comment #24) > If we were to use the event scope to resolve this bug, I'd instantiate the scope object in TypingCommand::insertText and other static functions of TypingCommand since the event scope was invented to delay firing of mutation events. > > On the other hand, it's strange that the event scope is needed for correctness of ordering of events. Ordering of the events should be correct regardless of whether we define the scope or not. That is what we began with. I've proposed to move "setSelection" before "dispatchEvent". Because that is what expected by user. But it turned out that scoping could be an alternate solution. This solution looks in terms of "how alternative text controller may depends on events firing in advance". Summary. We have to care about 2 message types: "webkitEditableContentChanged" and "input". The later one is fired from the first. We are aimed to make "input" be dispatched after setSelection, but it not known how change of "webkitEditableContentChanged" firing time will affect different aspects of editing.

We need to look at each place we listen to webkitEditableContentChanged, and make sure that code still works when we change the order. I'm sorry I sound like blocking your patch but we need to verify the correctness regardless of which approach we take. And the burden of the proof is on each patch author. I can't, as a reviewer, tell you which approach is better, let alone correct.

(In reply to comment #26) > We need to look at each place we listen to webkitEditableContentChanged, and make sure that code still works when we change the order. I'm sorry I sound like blocking your patch but we need to verify the correctness regardless of which approach we take. And the burden of the proof is on each patch author. I can't, as a reviewer, tell you which approach is better, let alone correct. I quite agree with you =) But your expertise in the area is very valuable! I've checked: we have no explicit listeners of webkitEditableContentChanged, but some handlers process this message specifically. As I see none of work with selection. When Node dispatches "webkitEditableContentChanged" it create "input" event and dispatches it as scoped. So it is at least suggested that scope is placed. There are two distinct invocations of Editor::appliedEditing. One from composite command, other one from typing command. un-/re-appliedEditing is invoked only from composite command. To be honest, none of those calls are wrapped directly by scope. So, what I see: 1) commands do not care about "input" event at all, and it is implementation detail of editor, when to dispatch this event; 2) webkitEditableContentChanged dispatchers do not depend on selection As a result, I propose to swap setSelection and dispatch of event in aforementioned 3 methods. I'll upload patch soon.

Created attachment 197012 [details] Patch

Created attachment 203349 [details] Patch

Created attachment 203355 [details] Patch

Comment on attachment 203355 [details] Patch Attachment 203355 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/661182 New failing tests: fast/dom/location-new-window-no-crash.html

Created attachment 203363 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3

(In reply to comment #27) > (In reply to comment #26) > > We need to look at each place we listen to webkitEditableContentChanged, and make sure that code still works when we change the order. I'm sorry I sound like blocking your patch but we need to verify the correctness regardless of which approach we take. And the burden of the proof is on each patch author. I can't, as a reviewer, tell you which approach is better, let alone correct. > > I quite agree with you =) But your expertise in the area is very valuable! > > I've checked: we have no explicit listeners of webkitEditableContentChanged, but some handlers process this message specifically. As I see none of work with selection. > > When Node dispatches "webkitEditableContentChanged" it create "input" event and dispatches it as scoped. So it is at least suggested that scope is placed. > > There are two distinct invocations of Editor::appliedEditing. One from composite command, other one from typing command. un-/re-appliedEditing is invoked only from composite command. To be honest, none of those calls are wrapped directly by scope. > > So, what I see: > 1) commands do not care about "input" event at all, and it is implementation detail of editor, when to dispatch this event; > 2) webkitEditableContentChanged dispatchers do not depend on selection > > As a result, I propose to swap setSelection and dispatch of event in aforementioned 3 methods. Thanks for the investigation.

Committed r151009: <http://trac.webkit.org/changeset/151009>

Reopening to attach new patch.

Created attachment 203563 [details] Patch

Ooops. Attached to wrong bug. (In reply to comment #36) > Created an attachment (id=203563) [details] > Patch

Where should this patch be attached? Bug 117070?

Comment on attachment 203563 [details] Patch Please post this patch on a new bug.