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

Side by Side 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:

« 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 »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 /*	1 /*

2 * Copyright (C) 2006, 2007 Apple Inc. All rights reserved.	2 * Copyright (C) 2006, 2007 Apple Inc. All rights reserved.

3 * Copyright (C) 2008 Nuanti Ltd.	3 * Copyright (C) 2008 Nuanti Ltd.

4 *	4 *

5 * Redistribution and use in source and binary forms, with or without	5 * Redistribution and use in source and binary forms, with or without

6 * modification, are permitted provided that the following conditions	6 * modification, are permitted provided that the following conditions

7 * are met:	7 * are met:

8 * 1. Redistributions of source code must retain the above copyright	8 * 1. Redistributions of source code must retain the above copyright

9 * notice, this list of conditions and the following disclaimer.	9 * notice, this list of conditions and the following disclaimer.

10 * 2. Redistributions in binary form must reproduce the above copyright	10 * 2. Redistributions in binary form must reproduce the above copyright

(...skipping 1022 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
1033 Document& newDocument = element->document();	1033 Document& newDocument = element->document();

1034	1034

1035 if (&newDocument != document) {	1035 if (&newDocument != document) {

1036 // Focus is going away from this document, so clear the focused element.	1036 // Focus is going away from this document, so clear the focused element.

1037 document->clearFocusedElement();	1037 document->clearFocusedElement();

1038 document->setSequentialFocusNavigationStartingPoint(nullptr);	1038 document->setSequentialFocusNavigationStartingPoint(nullptr);

1039 }	1039 }

1040	1040

1041 setFocusedFrame(newDocument.frame());	1041 setFocusedFrame(newDocument.frame());

1042	1042

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

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

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

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

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

	1048 FrameSelection& selection = focusedFrame()->selection();
	yosin_UTC9 2017/02/13 08:09:00 I'm OK this if Firefox and Edge do so. kochi@, wha I'm OK this if Firefox and Edge do so. kochi@, what do you think? Do Firefox and Edge clear selection? Do they fire "selectionchange" event? hugoh_UTC2 2017/02/13 09:18:52 Visually yes! That is why we cannot remove selecti Show quoted text On 2017/02/13 08:09:00, yosin_UTC9 wrote: > I'm OK this if Firefox and Edge do so. > kochi@, what do you think? > > Do Firefox and Edge clear selection? Visually yes! That is why we cannot remove selection.clear() completely. Show quoted text > Do they fire "selectionchange" event? Firefox fires two identical (?) selectionchange events when focus jumps. But the spec only mentions one event: "When the selection is dissociated with its range, associated with a new range or the associated range's boundary point is mutated either by the user or the content script, the user agent must queue a task to fire an event with the name selectionchange." hugoh_UTC2 2017/02/13 09:55:52 Maybe a selection.hide() would be better here? Fir Show quoted text On 2017/02/13 09:18:52, hugoh wrote: > On 2017/02/13 08:09:00, yosin_UTC9 wrote: > > I'm OK this if Firefox and Edge do so. > > kochi@, what do you think? > > > > Do Firefox and Edge clear selection? > Visually yes! That is why we cannot remove selection.clear() completely. Maybe a selection.hide() would be better here? Firefox only seem to hide the selection, it doesn't send a selectionchange event when tabbing from <input> to <button>. yosin_UTC9 2017/02/13 10:18:55 If we don't paint selection if it doesn't have foc Show quoted text On 2017/02/13 at 09:18:52, hugoh wrote: > On 2017/02/13 08:09:00, yosin_UTC9 wrote: > > I'm OK this if Firefox and Edge do so. > > kochi@, what do you think? > > > > Do Firefox and Edge clear selection? > Visually yes! That is why we cannot remove selection.clear() completely. If we don't paint selection if it doesn't have focus, it can also be visually equal. Show quoted text > > Do they fire "selectionchange" event? > Firefox fires two identical (?) selectionchange events when focus jumps. But the spec only mentions one event: > > "When the selection is dissociated with its range, associated with a new range or the associated range's boundary point is mutated either by the user or the content script, the user agent must queue a task to fire an event with the name selectionchange." Firing two "selectionchange" events comes from \|selection.clear()\| and setting selection to another INPUT element. This means Blink,(and Firefox) still dispatch redundant "selectionchange" for TAB navigation (and spatial navigation) case. How about not painting selection if it doesn't have focus? It can be done in PendingSelection::commit(): 129 if (!selection.isRange()) { 130 layoutView.clearSelection(); 131 return; 132 } We can change L129 to if (!selection.isRange() && !doseSelectionHaveFocus(selection)) doeSelectionHaveFocus() is similar to enabledInFocusedEditableText() hugoh_UTC2 2017/02/14 10:32:50 Good idea. I supposed you meant \|\|, not && ? That Show quoted text On 2017/02/13 10:18:55, yosin_UTC9 wrote: > How about not painting selection if it doesn't have focus? > > It can be done in PendingSelection::commit(): > > 129 if (!selection.isRange()) { > 130 layoutView.clearSelection(); > 131 return; > 132 } > > We can change L129 to > > if (!selection.isRange() && !doseSelectionHaveFocus(selection)) > > > doeSelectionHaveFocus() is similar to enabledInFocusedEditableText() 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 :)
	1049 selection.clear();

	1050 }

1043 element->focus(	1051 element->focus(

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

1045 return true;	1053 return true;

1046 }	1054 }

1047	1055

1048 Element* FocusController::findFocusableElement(WebFocusType type,	1056 Element* FocusController::findFocusableElement(WebFocusType type,

1049 Element& element) {	1057 Element& element) {

1050 // FIXME: No spacial navigation code yet.	1058 // FIXME: No spacial navigation code yet.

1051 DCHECK(type == WebFocusTypeForward \|\| type == WebFocusTypeBackward);	1059 DCHECK(type == WebFocusTypeForward \|\| type == WebFocusTypeBackward);

1052 ScopedFocusNavigation scope = ScopedFocusNavigation::createFor(element);	1060 ScopedFocusNavigation scope = ScopedFocusNavigation::createFor(element);

1053 return findFocusableElementAcrossFocusScopes(type, scope);	1061 return findFocusableElementAcrossFocusScopes(type, scope);

1054 }	1062 }

1055	1063

1056 Element* FocusController::findFocusableElementInShadowHost(	1064 Element* FocusController::findFocusableElementInShadowHost(

1057 const Element& shadowHost) {	1065 const Element& shadowHost) {

1058 DCHECK(shadowHost.authorShadowRoot());	1066 DCHECK(shadowHost.authorShadowRoot());

1059 ScopedFocusNavigation scope =	1067 ScopedFocusNavigation scope =

1060 ScopedFocusNavigation::ownedByShadowHost(shadowHost);	1068 ScopedFocusNavigation::ownedByShadowHost(shadowHost);

1061 return findFocusableElementAcrossFocusScopes(WebFocusTypeForward, scope);	1069 return findFocusableElementAcrossFocusScopes(WebFocusTypeForward, scope);

1062 }	1070 }

1063	1071

1064 static bool relinquishesEditingFocus(const Element& element) {	1072 static bool relinquishesEditingFocus(const Element& element) {

1065 DCHECK(hasEditableStyle(element));	1073 DCHECK(hasEditableStyle(element));

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

1067 }	1075 }

1068	1076

1069 static void clearSelectionIfNeeded(LocalFrame* oldFocusedFrame,

1070 LocalFrame* newFocusedFrame,

1071 Element* newFocusedElement) {

1072 if (!oldFocusedFrame \|\| !newFocusedFrame)

1073 return;

1074

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

1076 return;

1077

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

1079 if (selection.isNone())

1080 return;

1081

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

1083 if (selectionStartNode == newFocusedElement \|\|

1084 selectionStartNode->isDescendantOf(newFocusedElement))

1085 return;

1086

1087 if (!enclosingTextControl(selectionStartNode))

1088 return;

1089

1090 if (selectionStartNode->isInShadowTree() &&

1091 selectionStartNode->ownerShadowHost() == newFocusedElement)

1092 return;

1093

1094 selection.clear();

1095 }

1096

1097 bool FocusController::setFocusedElement(Element* element,	1077 bool FocusController::setFocusedElement(Element* element,

1098 Frame* newFocusedFrame) {	1078 Frame* newFocusedFrame) {

1099 return setFocusedElement(	1079 return setFocusedElement(

1100 element, newFocusedFrame,	1080 element, newFocusedFrame,

1101 FocusParams(SelectionBehaviorOnFocus::None, WebFocusTypeNone, nullptr));	1081 FocusParams(SelectionBehaviorOnFocus::None, WebFocusTypeNone, nullptr));

1102 }	1082 }

1103	1083

1104 bool FocusController::setFocusedElement(Element* element,	1084 bool FocusController::setFocusedElement(Element* element,

1105 Frame* newFocusedFrame,	1085 Frame* newFocusedFrame,

1106 const FocusParams& params) {	1086 const FocusParams& params) {

(...skipping 16 matching lines...) Expand all Loading...
1123 Document* newDocument = nullptr;	1103 Document* newDocument = nullptr;

1124 if (element)	1104 if (element)

1125 newDocument = &element->document();	1105 newDocument = &element->document();

1126 else if (newFocusedFrame && newFocusedFrame->isLocalFrame())	1106 else if (newFocusedFrame && newFocusedFrame->isLocalFrame())

1127 newDocument = toLocalFrame(newFocusedFrame)->document();	1107 newDocument = toLocalFrame(newFocusedFrame)->document();

1128	1108

1129 if (newDocument && oldDocument == newDocument &&	1109 if (newDocument && oldDocument == newDocument &&

1130 newDocument->focusedElement() == element)	1110 newDocument->focusedElement() == element)

1131 return true;	1111 return true;

1132	1112

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

1134 clearSelectionIfNeeded(oldFocusedFrame, toLocalFrame(newFocusedFrame),

1135 element);

1136

1137 if (oldDocument && oldDocument != newDocument)	1113 if (oldDocument && oldDocument != newDocument)

1138 oldDocument->clearFocusedElement();	1114 oldDocument->clearFocusedElement();

1139	1115

1140 if (newFocusedFrame && !newFocusedFrame->page()) {	1116 if (newFocusedFrame && !newFocusedFrame->page()) {

1141 setFocusedFrame(nullptr);	1117 setFocusedFrame(nullptr);

1142 return false;	1118 return false;

1143 }	1119 }

1144 setFocusedFrame(newFocusedFrame);	1120 setFocusedFrame(newFocusedFrame);

1145	1121

1146 if (newDocument) {	1122 if (newDocument) {

(...skipping 196 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
1343 if (focusCandidate.isOffscreenAfterScrolling) {	1319 if (focusCandidate.isOffscreenAfterScrolling) {

1344 Node* container = focusCandidate.enclosingScrollableBox;	1320 Node* container = focusCandidate.enclosingScrollableBox;

1345 scrollInDirection(container, type);	1321 scrollInDirection(container, type);

1346 return true;	1322 return true;

1347 }	1323 }

1348	1324

1349 // We found a new focus node, navigate to it.	1325 // We found a new focus node, navigate to it.

1350 Element* element = toElement(focusCandidate.focusableNode);	1326 Element* element = toElement(focusCandidate.focusableNode);

1351 DCHECK(element);	1327 DCHECK(element);

1352	1328

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

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

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

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

	1333 // selection.

	1334 FrameSelection& selection = focusedFrame()->selection();
	yosin_UTC9 2017/02/13 08:09:00 I'm OK this if Firefox and Edge do so. kochi@, wha I'm OK this if Firefox and Edge do so. kochi@, what do you think? Do Firefox and Edge clear selection? Do they fire "selectionchange" event? hugoh_UTC2 2017/02/13 09:18:52 advanceFocusDirectionallyInContainer() only runs f Show quoted text On 2017/02/13 08:09:00, yosin_UTC9 wrote: > I'm OK this if Firefox and Edge do so. > kochi@, what do you think? > > Do Firefox and Edge clear selection? Do they fire "selectionchange" event? advanceFocusDirectionallyInContainer() only runs for --enable-spatial-navigation. Neither Firefox nor Edge (?) implement spatial navigation natively. But I believe that spatial navigation should behave as "tab navigation" (both are user triggered focus jumps).
	1335 selection.clear();

	1336 }

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

1354 return true;	1338 return true;

1355 }	1339 }

1356	1340

1357 bool FocusController::advanceFocusDirectionally(WebFocusType type) {	1341 bool FocusController::advanceFocusDirectionally(WebFocusType type) {

1358 // FIXME: Directional focus changes don't yet work with RemoteFrames.	1342 // FIXME: Directional focus changes don't yet work with RemoteFrames.

1359 if (!focusedOrMainFrame()->isLocalFrame())	1343 if (!focusedOrMainFrame()->isLocalFrame())

1360 return false;	1344 return false;

1361 LocalFrame* curFrame = toLocalFrame(focusedOrMainFrame());	1345 LocalFrame* curFrame = toLocalFrame(focusedOrMainFrame());

1362 DCHECK(curFrame);	1346 DCHECK(curFrame);

(...skipping 51 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
1414 it->focusedFrameChanged();	1398 it->focusedFrameChanged();

1415 }	1399 }

1416	1400

1417 DEFINE_TRACE(FocusController) {	1401 DEFINE_TRACE(FocusController) {

1418 visitor->trace(m_page);	1402 visitor->trace(m_page);

1419 visitor->trace(m_focusedFrame);	1403 visitor->trace(m_focusedFrame);

1420 visitor->trace(m_focusChangedObservers);	1404 visitor->trace(m_focusChangedObservers);

1421 }	1405 }

1422	1406

1423 } // namespace blink	1407 } // namespace blink

OLD	NEW