|
|
DescriptionOOPIF: Enable TabAndMouseFocusNavigation.
Reland https://codereview.chromium.org/2796533002 with synchronization with
child frame navigation.
When advancing focus into an iframe, allow the frame to focus itself
after deciding which element will be focused. This prevents sending a
focus event to the previous element in that frame only to blur it
immediately. Non-oopifs simply fire a focus event to the new element,
the previously focused element in that frame received a blur event when
another frame was focused.
The test ensures a consistent state when mixing tab and clicking
navigation. In specific cases, focus would early out when the element to
advance to was previously focused, and never cleared when the mouse is
clicked in another frame.
BUG=702330
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2833503002
Cr-Commit-Position: refs/heads/master@{#465692}
Committed: https://chromium.googlesource.com/chromium/src/+/106f138fb306bb2283ce8adc22d1acd5eb31b891
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix comment by thakis. #Patch Set 3 : Fix comment by alexmos. #
Messages
Total messages: 24 (16 generated)
avallee@chromium.org changed reviewers: + alexmos@chromium.org, dcheng@chromium.org
Reland, added content::WaitForChildFrameSurfaceReady which ensures that we don't send click event meant for the child frames somewhere into the void when the navigation was not complete. It seems the test was flaking under PlzNavigate due to this race. Adding logging made the race less and less likely to lose.
The CQ bit was checked by avallee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
avallee@chromium.org changed reviewers: + thestig@chromium.org - dcheng@chromium.org
thestig: dcheng seems OOO, mind reviewing on this reland cl, blink hasn't changed since https://codereview.chromium.org/2796533002.
avallee@chromium.org changed reviewers: + thakis@chromium.org - thestig@chromium.org
thestig: sorry, copied the wrong name from git cl owners. thakis: please review the blink change, relanded from https://codereview.chromium.org/2796533002
pro tip for relanding CLs: Upload the original CL as patch set 1, then the fixed CL as patch set 2. Then it's easy to see what changed. blink lgtm, but: https://codereview.chromium.org/2833503002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2833503002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/page/FocusController.cpp:1005: // Focus wrapped around to the same element. The last time you landed the change, you removed this comment. Removing it looks right to me.
The CQ bit was checked by avallee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM to reland https://codereview.chromium.org/2833503002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2833503002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/page/FocusController.cpp:1029: // ClearFocusedElement() fires events that might detach the contentFrame, nit: s/contentFrame/ContentFrame/
The CQ bit was checked by avallee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Done. Thanks for the protip thakis. https://codereview.chromium.org/2833503002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/2833503002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/page/FocusController.cpp:1005: // Focus wrapped around to the same element. On 2017/04/19 16:53:16, Nico wrote: > The last time you landed the change, you removed this comment. Removing it looks > right to me. Done. https://codereview.chromium.org/2833503002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/page/FocusController.cpp:1029: // ClearFocusedElement() fires events that might detach the contentFrame, On 2017/04/19 16:57:33, alexmos (OOO soon) wrote: > nit: s/contentFrame/ContentFrame/ Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by avallee@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2833503002/#ps30001 (title: "Fix comment by alexmos.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 30001, "attempt_start_ts": 1492628569666650, "parent_rev": "5395ba5587248af158c295e543c9e8e8f2307c5c", "commit_rev": "106f138fb306bb2283ce8adc22d1acd5eb31b891"}
Message was sent while issue was closed.
Description was changed from ========== OOPIF: Enable TabAndMouseFocusNavigation. Reland https://codereview.chromium.org/2796533002 with synchronization with child frame navigation. When advancing focus into an iframe, allow the frame to focus itself after deciding which element will be focused. This prevents sending a focus event to the previous element in that frame only to blur it immediately. Non-oopifs simply fire a focus event to the new element, the previously focused element in that frame received a blur event when another frame was focused. The test ensures a consistent state when mixing tab and clicking navigation. In specific cases, focus would early out when the element to advance to was previously focused, and never cleared when the mouse is clicked in another frame. BUG=702330 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== OOPIF: Enable TabAndMouseFocusNavigation. Reland https://codereview.chromium.org/2796533002 with synchronization with child frame navigation. When advancing focus into an iframe, allow the frame to focus itself after deciding which element will be focused. This prevents sending a focus event to the previous element in that frame only to blur it immediately. Non-oopifs simply fire a focus event to the new element, the previously focused element in that frame received a blur event when another frame was focused. The test ensures a consistent state when mixing tab and clicking navigation. In specific cases, focus would early out when the element to advance to was previously focused, and never cleared when the mouse is clicked in another frame. BUG=702330 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2833503002 Cr-Commit-Position: refs/heads/master@{#465692} Committed: https://chromium.googlesource.com/chromium/src/+/106f138fb306bb2283ce8adc22d1... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:30001) as https://chromium.googlesource.com/chromium/src/+/106f138fb306bb2283ce8adc22d1...
Message was sent while issue was closed.
Description was changed from ========== OOPIF: Enable TabAndMouseFocusNavigation. Reland https://codereview.chromium.org/2796533002 with synchronization with child frame navigation. When advancing focus into an iframe, allow the frame to focus itself after deciding which element will be focused. This prevents sending a focus event to the previous element in that frame only to blur it immediately. Non-oopifs simply fire a focus event to the new element, the previously focused element in that frame received a blur event when another frame was focused. The test ensures a consistent state when mixing tab and clicking navigation. In specific cases, focus would early out when the element to advance to was previously focused, and never cleared when the mouse is clicked in another frame. BUG=702330 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2833503002 Cr-Commit-Position: refs/heads/master@{#465692} Committed: https://chromium.googlesource.com/chromium/src/+/106f138fb306bb2283ce8adc22d1... ========== to ========== OOPIF: Enable TabAndMouseFocusNavigation. Reland https://codereview.chromium.org/2796533002 with synchronization with child frame navigation. When advancing focus into an iframe, allow the frame to focus itself after deciding which element will be focused. This prevents sending a focus event to the previous element in that frame only to blur it immediately. Non-oopifs simply fire a focus event to the new element, the previously focused element in that frame received a blur event when another frame was focused. The test ensures a consistent state when mixing tab and clicking navigation. In specific cases, focus would early out when the element to advance to was previously focused, and never cleared when the mouse is clicked in another frame. BUG=702330 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2833503002 Cr-Commit-Position: refs/heads/master@{#465692} Committed: https://chromium.googlesource.com/chromium/src/+/106f138fb306bb2283ce8adc22d1... ========== |