third_party/WebKit/Source/core/page/FocusController.cpp - Issue 2616623002: Do not send redundant selectionchange-events (decouple focus)

Unified Diff: third_party/WebKit/Source/core/page/FocusController.cpp

Issue 2616623002: Do not send redundant selectionchange-events (decouple focus) (Closed)

Patch Set: Restore tab-key (and spatial) navigation's clearing behavior Created 3 years, 10 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

« content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java ('K') | « third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp ('k') | third_party/WebKit/Source/web/tests/WebFrameTest.cpp » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: third_party/WebKit/Source/core/page/FocusController.cpp

diff --git a/third_party/WebKit/Source/core/page/FocusController.cpp b/third_party/WebKit/Source/core/page/FocusController.cpp

index c75f524ac1ac4de629c2045c739a8667a6e8457c..9fe78372580730e4a686ca79ead8999de73fcc96 100644

--- a/third_party/WebKit/Source/core/page/FocusController.cpp

+++ b/third_party/WebKit/Source/core/page/FocusController.cpp

@@ -1040,6 +1040,14 @@ bool FocusController::advanceFocusInDocumentOrder(

setFocusedFrame(newDocument.frame());

+ if (!element->isTextControl()) {

+ // When tab-key makes focus jump from <input> to <button> (or to any other

+ // non-editable element) we clear the <input>'s selection. When jumping to

+ // an editable element, this clear() is not needed because clearing happens

+ // when the new element takes and resets the selection.

+ FrameSelection& selection = focusedFrame()->selection();

yosin_UTC9 2017/02/13 08:09:00 I'm OK this if Firefox and Edge do so. kochi@, wha

hugoh_UTC2 2017/02/13 09:18:52 Visually yes! That is why we cannot remove selecti

hugoh_UTC2 2017/02/13 09:55:52 Maybe a selection.hide() would be better here? Fir

yosin_UTC9 2017/02/13 10:18:55 If we don't paint selection if it doesn't have foc

hugoh_UTC2 2017/02/14 10:32:50 Good idea. I supposed you meant ||, not && ? That

Good idea. I supposed you meant ||, not && ? That visually cleared the selection during tab navigation (we really should add an automatic test case for tab navigation). But it cleared it too often: it broke 36 LayoutTests/editing tests as well as TextSelectionRepaintTest.RepaintSelectionOnFocus and LocalFrameTest.dragImageForSelectionUsesPageScaleFactor. This is the code I tried (without selection.clear() in FocusController). We need to find something better. Doesn't containsIncludingHostElements() return true too often? If <body> is the focused element and a child element has the selection, <body><p>[Selected]</p>, containsIncludingHostElements() will return true. Maybe that is the problem. +static bool selectionHasFocus(const LayoutView& layoutView, + const VisibleSelectionInFlatTree& selection) { + const Node* containerNode = selection.start().computeContainerNode(); + const LocalFrame* frame = layoutView.document().frame(); + const Document* document = frame->document(); + const Element* focusedElement = document->focusedElement(); + + return focusedElement && containerNode && + focusedElement->containsIncludingHostElements(*containerNode); +} - if (!selection.isRange()) { + if (!selectionHasFocus(layoutView, selection) || !selection.isRange()) { layoutView.clearSelection(); return; } I'm happy if this CL only fixes crbug.com/678601 and crbug.com/679635 . We could put removal of the last two selection.clear() in FocusController as TODO. That would reduce the impact and risk of this CL :)

+ selection.clear();

+ }

element->focus(

FocusParams(SelectionBehaviorOnFocus::Reset, type, sourceCapabilities));

return true;

@@ -1066,34 +1074,6 @@ static bool relinquishesEditingFocus(const Element& element) {

return element.document().frame() && rootEditableElement(element);

}

-static void clearSelectionIfNeeded(LocalFrame* oldFocusedFrame,

- LocalFrame* newFocusedFrame,

- Element* newFocusedElement) {

- if (!oldFocusedFrame || !newFocusedFrame)

- return;

- if (oldFocusedFrame->document() != newFocusedFrame->document())

- return;

- FrameSelection& selection = oldFocusedFrame->selection();

- if (selection.isNone())

- return;

- Node* selectionStartNode = selection.selection().start().anchorNode();

- if (selectionStartNode == newFocusedElement ||

- selectionStartNode->isDescendantOf(newFocusedElement))

- return;

- if (!enclosingTextControl(selectionStartNode))

- return;

- if (selectionStartNode->isInShadowTree() &&

- selectionStartNode->ownerShadowHost() == newFocusedElement)

- return;

- selection.clear();

bool FocusController::setFocusedElement(Element* element,

Frame* newFocusedFrame) {

return setFocusedElement(

@@ -1130,10 +1110,6 @@ bool FocusController::setFocusedElement(Element* element,

newDocument->focusedElement() == element)

return true;

- if (newFocusedFrame && newFocusedFrame->isLocalFrame())

- clearSelectionIfNeeded(oldFocusedFrame, toLocalFrame(newFocusedFrame),

- element);

if (oldDocument && oldDocument != newDocument)

oldDocument->clearFocusedElement();

@@ -1350,6 +1326,14 @@ bool FocusController::advanceFocusDirectionallyInContainer(

Element* element = toElement(focusCandidate.focusableNode);

DCHECK(element);

+ if (!element->isTextControl()) {

+ // Spatial navigation, just as "tab navigation", clears the selection as it

+ // moves focus. When jumping to an editable element, this clear() is not

+ // needed because clearing happens when the new element takes and resets the

+ // selection.

+ FrameSelection& selection = focusedFrame()->selection();

yosin_UTC9 2017/02/13 08:09:00 I'm OK this if Firefox and Edge do so. kochi@, wha

hugoh_UTC2 2017/02/13 09:18:52 advanceFocusDirectionallyInContainer() only runs f

+ selection.clear();

+ }

element->focus(FocusParams(SelectionBehaviorOnFocus::Reset, type, nullptr));

return true;

}