Created attachment 55301 [details] Test case Given an element that has contentEditable. Focus it and select some text. Now click on something else that does not start a new selection (Node::canStartSelection is false). The selection is kept which is fine, however, now press a key on your keyboard. The contentEditable element steals the focus and modifies its content. The same problem exists for input and textareas but there is a workaround in the code to always collapse their selection in FocusController::clearSelectionIfNeeded. See test case for clearer repro steps.
I'm not sure if I'd expect the focus to remain in the contenteditable div after clicking a button, or to move out. Apparently, WebCore also isn't sure about that.
The ideal behavior for web devs would be to keep the selection, but move the focus. This lets you write code that deals with the selection in an editable area onclick of a button without doing workarounds. That's tricky to make work as there are clearly places in WebCore where we assume that the selection is in the focused node.
Another approach to make web development sane and keep the invariant that the selection is inside the focused node, would be to clear the selection, but store it per editable element and restore it when the element regains focus.
Created attachment 56540 [details] test case with keydown I saw this recently too. It's also a problem that it allows modification to the content editable field without firing the keydown event.
Also, see bug 38548 for related discussion around this behavior.
Gecko has similar bugs: https://bugzilla.mozilla.org/show_bug.cgi?id=567213
Created attachment 86009 [details] Work in progress Firefox has fixed this (I've verified with Firefox 4) so we probably should too.
Created attachment 86432 [details] Patch
Comment on attachment 86432 [details] Patch Removing the r? as there's still some things to be done here... mainly unifying the expression in Editor.cpp with the code in setFocusedNodeIfNeeded to use some function like focusableRoot. This change incorporates a fix for 56271, as they both have to do with selection and focus being bound together differently than in other browsers, and both result in many rebaselines. This change, however, is risky. Sites that do a lot of fancy editing and selection that may have WebKit-specific paths could break. Ideally I'd like to see the final version of this patch put into a Chrome Dev-channel release to get some widespread exposure and identify possible site breakage. Thoughts and comments are appreciated, and keep in mind this doesn't include the couple hundred rebaselines due to changes in focus ring rects and editing delegates.
Created attachment 86538 [details] Patch There are over 600 layout tests with either editing delegate changes, focus ring rect changes, or both. We could fix the focus ring pixel changes by explicitly focusing the applicable nodes in the tests, though it's a large change (over 400 tests appear to have this issue). Two tests will become totally invalid if we make this change, as both expect an onFocus handler to fire in a selection or drag-and-drop. These are - fast/events/content-changed-during-drop.html - fast/events/5056619.html Also, fast/events/inputText-never-fired-on-keydown-cancel.html has part of the test explicitly checking to see if focus follows selection. I've removed that part of the test in the patch already. I believe this is a change we want to make. Thoughts?
Listing this as blocking 56271 since the're related and I believe they should be fixed together given the rebaselining effort each will require.
Comment on attachment 86538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86538&action=review I'm supportive of this change but I'd like to hear opinions of other contributors. > LayoutTests/platform/mac/editing/input/insert-delete-smp-symbol.html:15 > - textInputController.insertText("ð"); > + textInputController.insertText("Æ"); Why do we have this change? > LayoutTests/fast/events/script-tests/inputText-never-fired-on-keydown-cancel.js:-60 > -window.getSelection().setBaseAndExtent(targetEditable, 0, targetEditable, 0); > -test(targetEditable); > - Are you sure removing this test is the correct fix? Isn't that the author forgot to focus targetEditable? > LayoutTests/editing/selection/focus-doesnt-follow-selection.html:2 > + I don't think we need this blank line. > LayoutTests/editing/selection/focus-doesnt-follow-selection.html:14 > +test.addEventListener("focus", function() { > + test.innerText = "FAIL"; > +}, false); It seems like it's simpler to put this in the onfocus attribute of the div. > LayoutTests/editing/editing.js:877 > + if (document.getElementsByTagName("body")[0].contentEditable) Why can't we do document.body.contentEditable? > LayoutTests/editing/editing.js:901 > + if (document.getElementsByTagName("body")[0].contentEditable) Ditto. > LayoutTests/editing/input/only-focused-field-receives-input.html:2 > + Same comment about the blank line. > LayoutTests/editing/input/only-focused-field-receives-input.html:11 > + var ev = document.createEvent("KeyboardEvent"); Let's use "event" instead of "ev". > LayoutTests/editing/input/only-focused-field-receives-input.html:19 > +function pressKey( key ) { > + if (window.KeyEvent) { > + var ev = document.createEvent("KeyboardEvent"); > + ev.initKeyEvent("keypress", true, true, window, 0,0,0,0, 0, key.charCodeAt(0)); > + document.body.dispatchEvent(ev); > + } > + else { > + var ev = document.createEvent("TextEvent"); > + ev.initTextEvent('textInput', true, true, null, key.charAt(0)); > + document.body.dispatchEvent(ev); > + } Isn't it better to use eventSender here? > LayoutTests/editing/input/only-focused-field-receives-input.html:34 > + eventSender.mouseDown(); > + eventSender.mouseUp(); Maybe add leapForward between mouseDown and mouseUp? > LayoutTests/editing/style/highlight-insert-paragraph.html:17 > - document.body.dispatchEvent(ev); > + document.getElementById("test").dispatchEvent(ev); Why do we need this change? > LayoutTests/editing/style/highlight-insert-paragraph.html:22 > - document.body.dispatchEvent(ev); > + document.getElementById("test").dispatchEvent(ev); Ditto. > Source/WebCore/editing/Editor.cpp:188 > +static Node* focusableRoot(Node* node) > +{ > + if (node->supportsFocus()) I'm surprised that such a function didn't already exist. > Source/WebCore/editing/Editor.cpp:211 > + if (selectionStart && selectionStart != targetNode > + && (!focusableRoot(targetNode) || !focusableRoot(targetNode)->containsIncludingShadowDOM(focusableRoot(selectionStart)))) > + return false; It's not great that we call focusableRoot twice here. Can't we have a nested if with just one call to focusableRoot? Also, can targetNode be null when selectionStart is not null? > Source/WebCore/ChangeLog:16 > + > + One extra blank line here.
Comment on attachment 86538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86538&action=review Thanks for the thorough review as usual! Do you have any thoughts about whether you think this change is fundamentally sound? >> LayoutTests/platform/mac/editing/input/insert-delete-smp-symbol.html:15 >> + textInputController.insertText("Æ"); > > Why do we have this change? This seems to be an encoding issue from my editor. I'll fix it. >> LayoutTests/fast/events/script-tests/inputText-never-fired-on-keydown-cancel.js:-60 >> - > > Are you sure removing this test is the correct fix? Isn't that the author forgot to focus targetEditable? Except in that case it'd just be a rehashing of the previous test. The setBaseAndExtent would become meaningless. >> LayoutTests/editing/selection/focus-doesnt-follow-selection.html:2 >> + > > I don't think we need this blank line. You never *need* a blank line ;) >> LayoutTests/editing/selection/focus-doesnt-follow-selection.html:14 >> +}, false); > > It seems like it's simpler to put this in the onfocus attribute of the div. Fair enough. >> LayoutTests/editing/editing.js:877 >> + if (document.getElementsByTagName("body")[0].contentEditable) > > Why can't we do document.body.contentEditable? Good call. >> LayoutTests/editing/input/only-focused-field-receives-input.html:19 >> + } > > Isn't it better to use eventSender here? We could, yes. >> LayoutTests/editing/style/highlight-insert-paragraph.html:17 >> + document.getElementById("test").dispatchEvent(ev); > > Why do we need this change? This change has the effect of not dispatching text events to the selection that aren't destined for the focused container. This matches other browsers. >> LayoutTests/editing/style/highlight-insert-paragraph.html:22 >> + document.getElementById("test").dispatchEvent(ev); > > Ditto. See above. >> Source/WebCore/editing/Editor.cpp:188 >> + if (node->supportsFocus()) > > I'm surprised that such a function didn't already exist. As far as I can tell, the only similar functionality is in SelectionController::setFocusedNodeIfNeeded. I hoped to be able to unify their implementations, but setFocusedNodeIfNeeded explicitly avoids Frames, which we explicitly *don't* want to do here. >> Source/WebCore/editing/Editor.cpp:211 >> + return false; > > It's not great that we call focusableRoot twice here. Can't we have a nested if with just one call to focusableRoot? Also, can targetNode be null when selectionStart is not null? This could be nested to avoid this, that's a valid performance point. As for targetNode being null, I believe that's an invalid case, but I'm not confident enough to say for sure. I did quite a bit of testing to get this condition correct and never ran into such a case. I'd be more than happy to add an assert. >> Source/WebCore/ChangeLog:16 >> + > > One extra blank line here. Ryosuke Niwa, destroyer of whitespace! :)
This is just a guess. I don’t have time right at the moment to investigate further. The idea that only something focused can have a selection is probably something that came from the Mac UI. I am not certain, but I suspect that the heart of this is the difference in approach to focus on Mac vs. other platforms. It’s common to have the web reflect the Windows approach. And platforms other than Mac often simply echo it. Those of us making WebKit work were all working for Apple and we may have changed WebKit’s semantics to match the Mac. So the most likely trouble spots for this change will be in uses outside of web page context. It seems likely this will change behavior of some Dashboard widgets or Mac OS X applications. They may even need a way to request the old behavior. For web browsers themselves, it’s probably OK to reflect this Windows style rather than matching the rest of Mac OS X.
(In reply to comment #14) > So the most likely trouble spots for this change will be in uses outside of web page context. It seems likely this will change behavior of some Dashboard widgets or Mac OS X applications. They may even need a way to request the old behavior. Thanks Darin! I added an option flag and two convenience functions that replicate the old behavior. I would expect the issues you're talking about coming up in external apps, you're right. On Mac WebKit, there are only two places in WebFrame where one can explicitly set the selection. These could easily be changed to call the setSelectionAndFocus method I added if you believe that would result in the expected behavior. If that gets me the green light I want to move forward with landing this and getting it into dev builds of Chromium sooner than later to start ferreting out any website regressions based on the old behavior.
To get the “green light” we’d need to do some kind of testing. We can’t make assumptions about how applications and Dashboard widgets set the selection. They can both have large pieces written in JavaScript, and can access the DOM and JavaScript from Objective-C as well.
(In reply to comment #14) > So the most likely trouble spots for this change will be in uses outside of web page context. It seems likely this will change behavior of some Dashboard widgets or Mac OS X applications. They may even need a way to request the old behavior. Is there a good way to test this?
(In reply to comment #17) > Is there a good way to test this? Like all application compatibility issues there are two parts: 1) Getting the applications. 2) Trying out the parts of the applications that are likely to be affected. For Dashboard widgets, there is a set included with Mac OS X. For other applications it can be challenging. Many use WebKit!
(In reply to comment #18) > For Dashboard widgets, there is a set included with Mac OS X. For other applications it can be challenging. Many use WebKit! Just to clarify, do you still think keeping the behavior the same from Objective-C to stay consistent with OSX is the right path? We would, of course, still have JavaScript to contend with, which I'd expect most likely to be a problem in places like Dashboard where the JavaScript was written specifically with WebKit in mind (since other browsers don't have focus follow selection). Is this testing something Apple will do as part of its QA process or is there additional testing you want ahead of that?
(In reply to comment #19) > Just to clarify, do you still think keeping the behavior the same from Objective-C to stay consistent with OSX is the right path? Differences between Objective-C bindings and JavaScript would be a last resort if we couldn’t find any other solution. If we had some sort of quirk, it would be more likely tied to the context WebKit is running in rather than the caller. > We would, of course, still have JavaScript to contend with, which I'd expect most likely to be a problem in places like Dashboard where the JavaScript was written specifically with WebKit in mind (since other browsers don't have focus follow selection). There are many Mac OS X and iOS applications that use JavaScript too, not just Dashboard widgets. > Is this testing something Apple will do as part of its QA process or is there additional testing you want ahead of that? Apple’s QA process might include some testing of third party applications, but probably long after any code change; there’s no team I can assign this to. My department at Apple doesn’t have the resources to test all those applications and Dashboard widgets; normally we use technical strategies that minimize the need for that kind of extensive testing.
Created attachment 91338 [details] Patch with updated expectations (In reply to comment #20) > Apple’s QA process might include some testing of third party applications, but probably long after any code change; there’s no team I can assign this to. My department at Apple doesn’t have the resources to test all those applications and Dashboard widgets; normally we use technical strategies that minimize the need for that kind of extensive testing. I did the following testing with this change: 1) Ran Dashboard with this change, loaded every Widget that ships with Snow Leopard, and manually attempted to exhaust the functionality of each. 2) Ran the BrowserScope RTE suite (http://www.browserscope.org/richtext2/test) and validated we didn't regress. 3) Validated that forms that automatically set focus while you fill them out still work properly (tested Amazon, Yahoo, Google Checkout, and PayPal payment forms). 4) Validated that Rich Text Editors continue to work properly (TinyMCE, Aloha-Editor). Throughout all this testing, I ran into no issues. Once again, for the web ideally we'd get this change into a Chrome beta to ferret out issues on the Web. This doesn't address concerns about Mac Apps with embedded WebViews. If this isn't something you can QA, we could make this an editing behavior, but I don't think that would be a clean solution.
(In reply to comment #21) > Created an attachment (id=91338) [details] > Patch with updated expectations TYI: This patch includes Mac test expectation changes and is nearly 8 megs. The code change is present in the previous patch (86538) has the code change in a format that's easier to look at, but the point of contention here is mostly about the behavior itself.
Attachment 91338 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8519057
Attachment 91338 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8511500
Attachment 91338 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8517129
*** Bug 65663 has been marked as a duplicate of this bug. ***
It is impossible to open the patch in the review tool. Please upload a smaller patch. Split out the test change from the code changes if you have to. r-.
I'm happy to work with you to get this reviewed. I just learned from your peer review that this had been stalled.
(In reply to comment #28) > I'm happy to work with you to get this reviewed. I just learned from your peer review that this had been stalled. Awesome :) The tricky thing is preventing regressions for Mac apps, I'm not sure how we can ensure coverage there. Let's discuss this next week (I'm heads down on subpixel and perf for now).
Is this the same bug? (If the focus happened AFTER the transition ends, the field wouldn't be lost as in this example) http://jsfiddle.net/dNk9v/ (Navigating with the TAB key should NOT break the transition properly working when clicking the tabs - legend tags) In this example, the form inside a container with overflow:hidden is MOVED somehow to display the focused field, but such motion is NOT reflected in ANY CSS value, so it's impossible to control/prevent the transition to break.