Fix focus navigation getting stuck

Fix focus navigation getting stuck

The ScopedFocusNavigation class, which is used internally for
keeping state of traversing next/previous focusable element in a
scope, could become inconsistent state that the current element
of the scope does not point to the element that is being traversed.

Specifically, such situation happens when
nextElementWithGreaterTabIndex() and previousElementWithLowerTabIndex()
are called. These functions moves the current pointer within the
whole scope and when the function finishes, all elements are
exhausted and the current pointer will be null, while it can
return some element inside the scope.

Therefore, in findFocusableElementRecursivelyForward() and
findFocusableElementRecursivelyBackward(), both of which
has a loop

  while (found) {
    ...
  }

and inside the loop, the following pattern

  found= findFocusableElementInternal();
  continue;

can be affected by this inconsistency, because
findFocusableElementInternal() can end up calling those
problematic functions.

This CL also refactors the ScopedFocusNavigation class a bit,
to hide these traversal state within the class, and it now
exposes only "findFocusableElement(forward or backward)" API
for traversing.

BUG=671428
Committed: https://crrev.com/960b8cda43c4d804f79ecf75b82559751f21819c
Cr-Commit-Position: refs/heads/master@{#438108}

[kochi, 2016-12-09 11:33:57]

The CQ bit was checked by kochi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 11:34:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kochi, 2016-12-09 12:17:10]

Description was changed from

==========
Fix FocusController.


BUG=671428
==========

to

==========
Fix focus navigation getting stuck

The ScopedFocusNavigation class, which is used internally for
keeping state of traversing next/previous focusable element in a
scope, could become inconsistent state that the current element
of the scope does not point to the element that is being traversed.

Specifically, such situation happens when
nextElementWithGreaterTabIndex() and previousElementWithLowerTabIndex()
are called.

Therefore, in findFocusableElementRecursivelyForward() and
findFocusableElementRecursivelyBackward(), both of which
has a loop
  while (found) {
    ...
  }
and inside the loop, the following pattern
  found= findFocusableElementInternal();
  continue;
can be affected by this inconsistency.

BUG=671428
==========

[kochi, 2016-12-09 12:17:10]

kochi@chromium.org changed reviewers:
+ hayato@chromium.org

[kochi, 2016-12-09 12:17:28]

Description was changed from

==========
Fix focus navigation getting stuck

The ScopedFocusNavigation class, which is used internally for
keeping state of traversing next/previous focusable element in a
scope, could become inconsistent state that the current element
of the scope does not point to the element that is being traversed.

Specifically, such situation happens when
nextElementWithGreaterTabIndex() and previousElementWithLowerTabIndex()
are called.

Therefore, in findFocusableElementRecursivelyForward() and
findFocusableElementRecursivelyBackward(), both of which
has a loop
  while (found) {
    ...
  }
and inside the loop, the following pattern
  found= findFocusableElementInternal();
  continue;
can be affected by this inconsistency.

BUG=671428
==========

to

==========
Fix focus navigation getting stuck

The ScopedFocusNavigation class, which is used internally for
keeping state of traversing next/previous focusable element in a
scope, could become inconsistent state that the current element
of the scope does not point to the element that is being traversed.

Specifically, such situation happens when
nextElementWithGreaterTabIndex() and previousElementWithLowerTabIndex()
are called.

Therefore, in findFocusableElementRecursivelyForward() and
findFocusableElementRecursivelyBackward(), both of which
has a loop

  while (found) {
    ...
  }

and inside the loop, the following pattern

  found= findFocusableElementInternal();
  continue;

can be affected by this inconsistency.

BUG=671428
==========

[kochi, 2016-12-09 12:19:46]

Description was changed from

==========
Fix focus navigation getting stuck

The ScopedFocusNavigation class, which is used internally for
keeping state of traversing next/previous focusable element in a
scope, could become inconsistent state that the current element
of the scope does not point to the element that is being traversed.

Specifically, such situation happens when
nextElementWithGreaterTabIndex() and previousElementWithLowerTabIndex()
are called.

Therefore, in findFocusableElementRecursivelyForward() and
findFocusableElementRecursivelyBackward(), both of which
has a loop

  while (found) {
    ...
  }

and inside the loop, the following pattern

  found= findFocusableElementInternal();
  continue;

can be affected by this inconsistency.

BUG=671428
==========

to

==========
Fix focus navigation getting stuck

The ScopedFocusNavigation class, which is used internally for
keeping state of traversing next/previous focusable element in a
scope, could become inconsistent state that the current element
of the scope does not point to the element that is being traversed.

Specifically, such situation happens when
nextElementWithGreaterTabIndex() and previousElementWithLowerTabIndex()
are called. These functions moves the current pointer on the
whole scope and when the function finishes, all elements are
exhausted and the current pointer will be null, while it can
return some element.

Therefore, in findFocusableElementRecursivelyForward() and
findFocusableElementRecursivelyBackward(), both of which
has a loop

  while (found) {
    ...
  }

and inside the loop, the following pattern

  found= findFocusableElementInternal();
  continue;

can be affected by this inconsistency.

BUG=671428
==========

[kochi, 2016-12-09 12:21:54]

Description was changed from

==========
Fix focus navigation getting stuck

The ScopedFocusNavigation class, which is used internally for
keeping state of traversing next/previous focusable element in a
scope, could become inconsistent state that the current element
of the scope does not point to the element that is being traversed.

Specifically, such situation happens when
nextElementWithGreaterTabIndex() and previousElementWithLowerTabIndex()
are called. These functions moves the current pointer on the
whole scope and when the function finishes, all elements are
exhausted and the current pointer will be null, while it can
return some element.

Therefore, in findFocusableElementRecursivelyForward() and
findFocusableElementRecursivelyBackward(), both of which
has a loop

  while (found) {
    ...
  }

and inside the loop, the following pattern

  found= findFocusableElementInternal();
  continue;

can be affected by this inconsistency.

BUG=671428
==========

to

==========
Fix focus navigation getting stuck

The ScopedFocusNavigation class, which is used internally for
keeping state of traversing next/previous focusable element in a
scope, could become inconsistent state that the current element
of the scope does not point to the element that is being traversed.

Specifically, such situation happens when
nextElementWithGreaterTabIndex() and previousElementWithLowerTabIndex()
are called. These functions moves the current pointer within the
whole scope and when the function finishes, all elements are
exhausted and the current pointer will be null, while it can
return some element inside the scope.

Therefore, in findFocusableElementRecursivelyForward() and
findFocusableElementRecursivelyBackward(), both of which
has a loop

  while (found) {
    ...
  }

and inside the loop, the following pattern

  found= findFocusableElementInternal();
  continue;

can be affected by this inconsistency, because
findFocusableElementInternal() can end up calling those
problematic functions.

BUG=671428
==========

[kochi, 2016-12-09 12:34:20]

The CQ bit was checked by kochi@chromium.org to run a CQ dry run

[kochi, 2016-12-09 12:34:27]

PTAL

[commit-bot: I haz the power, 2016-12-09 12:34:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 14:15:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 14:15:40]

Dry run: This issue passed the CQ dry run.

[hayato, 2016-12-13 05:41:57]

https://codereview.chromium.org/2562753003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2562753003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:561:
scope.setCurrentElement(found);
It does not look a good idea to call setCurrentElement() outside of
ScopedFocusNavigation.

I think this is not the fault of this CL. It looks FocusController already calls
setCurrentElement() from the outside of ScopedFocusNavigator.

However, ideally, we should not make setCurrentElement a public member function.
m_currentElement should be set only via ScopedFocusNavigator's other traversal
functions.

Could you try to make ScopedFocusNavigator::setCurrentElement() a private member
function? I guess this requires a kind of refactoring, but it would make a
responsibility much clearer and would become less error-prone.

[kochi, 2016-12-13 07:20:37]

https://codereview.chromium.org/2562753003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/2562753003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:561:
scope.setCurrentElement(found);
On 2016/12/13 05:41:57, hayato wrote:
> It does not look a good idea to call setCurrentElement() outside of
> ScopedFocusNavigation.
> 
> I think this is not the fault of this CL. It looks FocusController already
calls
> setCurrentElement() from the outside of ScopedFocusNavigator.
> 
> However, ideally, we should not make setCurrentElement a public member
function.
> m_currentElement should be set only via ScopedFocusNavigator's other traversal
> functions.
> 
> Could you try to make ScopedFocusNavigator::setCurrentElement() a private
member
> function? I guess this requires a kind of refactoring, but it would make a
> responsibility much clearer and would become less error-prone.

Agreed.

[kochi, 2016-12-13 08:10:17]

Patchset #3 (id:40001) has been deleted

[kochi, 2016-12-13 08:10:23]

The CQ bit was checked by kochi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 08:10:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kochi, 2016-12-13 08:14:49]

Description was changed from

==========
Fix focus navigation getting stuck

The ScopedFocusNavigation class, which is used internally for
keeping state of traversing next/previous focusable element in a
scope, could become inconsistent state that the current element
of the scope does not point to the element that is being traversed.

Specifically, such situation happens when
nextElementWithGreaterTabIndex() and previousElementWithLowerTabIndex()
are called. These functions moves the current pointer within the
whole scope and when the function finishes, all elements are
exhausted and the current pointer will be null, while it can
return some element inside the scope.

Therefore, in findFocusableElementRecursivelyForward() and
findFocusableElementRecursivelyBackward(), both of which
has a loop

  while (found) {
    ...
  }

and inside the loop, the following pattern

  found= findFocusableElementInternal();
  continue;

can be affected by this inconsistency, because
findFocusableElementInternal() can end up calling those
problematic functions.

BUG=671428
==========

to

==========
Fix focus navigation getting stuck

The ScopedFocusNavigation class, which is used internally for
keeping state of traversing next/previous focusable element in a
scope, could become inconsistent state that the current element
of the scope does not point to the element that is being traversed.

Specifically, such situation happens when
nextElementWithGreaterTabIndex() and previousElementWithLowerTabIndex()
are called. These functions moves the current pointer within the
whole scope and when the function finishes, all elements are
exhausted and the current pointer will be null, while it can
return some element inside the scope.

Therefore, in findFocusableElementRecursivelyForward() and
findFocusableElementRecursivelyBackward(), both of which
has a loop

  while (found) {
    ...
  }

and inside the loop, the following pattern

  found= findFocusableElementInternal();
  continue;

can be affected by this inconsistency, because
findFocusableElementInternal() can end up calling those
problematic functions.

This CL also refactors the ScopedFocusNavigation class a bit,
to hide these traversal state within the class, and it now
exposes only "findFocusableElement(forward or backward)" API
for traversing.

BUG=671428
==========

[kochi, 2016-12-13 08:15:17]

Refactored the class a bit, and updated the description
(added at the bottom).

PTAL.

[hayato, 2016-12-13 08:30:23]

LGTM. Thank you for the refactoring. That is exactly what I would like to see.
:)

[kochi, 2016-12-13 08:40:16]

The CQ bit was unchecked by kochi@chromium.org

[kochi, 2016-12-13 08:40:17]

The CQ bit was checked by kochi@chromium.org

[commit-bot: I haz the power, 2016-12-13 08:40:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 09:23:29]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481618417681900,
"parent_rev": "0dc221437ecf519f673c89c8e472ab0378f7c804", "commit_rev":
"9c39bf509a9c6459f55b7d9200c29ab56236cd5c"}

[commit-bot: I haz the power, 2016-12-13 09:23:55]

Description was changed from

==========
Fix focus navigation getting stuck

The ScopedFocusNavigation class, which is used internally for
keeping state of traversing next/previous focusable element in a
scope, could become inconsistent state that the current element
of the scope does not point to the element that is being traversed.

Specifically, such situation happens when
nextElementWithGreaterTabIndex() and previousElementWithLowerTabIndex()
are called. These functions moves the current pointer within the
whole scope and when the function finishes, all elements are
exhausted and the current pointer will be null, while it can
return some element inside the scope.

Therefore, in findFocusableElementRecursivelyForward() and
findFocusableElementRecursivelyBackward(), both of which
has a loop

  while (found) {
    ...
  }

and inside the loop, the following pattern

  found= findFocusableElementInternal();
  continue;

can be affected by this inconsistency, because
findFocusableElementInternal() can end up calling those
problematic functions.

This CL also refactors the ScopedFocusNavigation class a bit,
to hide these traversal state within the class, and it now
exposes only "findFocusableElement(forward or backward)" API
for traversing.

BUG=671428
==========

to

==========
Fix focus navigation getting stuck

The ScopedFocusNavigation class, which is used internally for
keeping state of traversing next/previous focusable element in a
scope, could become inconsistent state that the current element
of the scope does not point to the element that is being traversed.

Specifically, such situation happens when
nextElementWithGreaterTabIndex() and previousElementWithLowerTabIndex()
are called. These functions moves the current pointer within the
whole scope and when the function finishes, all elements are
exhausted and the current pointer will be null, while it can
return some element inside the scope.

Therefore, in findFocusableElementRecursivelyForward() and
findFocusableElementRecursivelyBackward(), both of which
has a loop

  while (found) {
    ...
  }

and inside the loop, the following pattern

  found= findFocusableElementInternal();
  continue;

can be affected by this inconsistency, because
findFocusableElementInternal() can end up calling those
problematic functions.

This CL also refactors the ScopedFocusNavigation class a bit,
to hide these traversal state within the class, and it now
exposes only "findFocusableElement(forward or backward)" API
for traversing.

BUG=671428

Review-Url: https://codereview.chromium.org/2562753003
==========

[commit-bot: I haz the power, 2016-12-13 09:23:56]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-12-13 09:26:40]

Description was changed from

==========
Fix focus navigation getting stuck

The ScopedFocusNavigation class, which is used internally for
keeping state of traversing next/previous focusable element in a
scope, could become inconsistent state that the current element
of the scope does not point to the element that is being traversed.

Specifically, such situation happens when
nextElementWithGreaterTabIndex() and previousElementWithLowerTabIndex()
are called. These functions moves the current pointer within the
whole scope and when the function finishes, all elements are
exhausted and the current pointer will be null, while it can
return some element inside the scope.

Therefore, in findFocusableElementRecursivelyForward() and
findFocusableElementRecursivelyBackward(), both of which
has a loop

  while (found) {
    ...
  }

and inside the loop, the following pattern

  found= findFocusableElementInternal();
  continue;

can be affected by this inconsistency, because
findFocusableElementInternal() can end up calling those
problematic functions.

This CL also refactors the ScopedFocusNavigation class a bit,
to hide these traversal state within the class, and it now
exposes only "findFocusableElement(forward or backward)" API
for traversing.

BUG=671428

Review-Url: https://codereview.chromium.org/2562753003
==========

to

==========
Fix focus navigation getting stuck

The ScopedFocusNavigation class, which is used internally for
keeping state of traversing next/previous focusable element in a
scope, could become inconsistent state that the current element
of the scope does not point to the element that is being traversed.

Specifically, such situation happens when
nextElementWithGreaterTabIndex() and previousElementWithLowerTabIndex()
are called. These functions moves the current pointer within the
whole scope and when the function finishes, all elements are
exhausted and the current pointer will be null, while it can
return some element inside the scope.

Therefore, in findFocusableElementRecursivelyForward() and
findFocusableElementRecursivelyBackward(), both of which
has a loop

  while (found) {
    ...
  }

and inside the loop, the following pattern

  found= findFocusableElementInternal();
  continue;

can be affected by this inconsistency, because
findFocusableElementInternal() can end up calling those
problematic functions.

This CL also refactors the ScopedFocusNavigation class a bit,
to hide these traversal state within the class, and it now
exposes only "findFocusableElement(forward or backward)" API
for traversing.

BUG=671428

Committed: https://crrev.com/960b8cda43c4d804f79ecf75b82559751f21819c
Cr-Commit-Position: refs/heads/master@{#438108}
==========

[commit-bot: I haz the power, 2016-12-13 09:26:41]

Patchset 3 (id:??) landed as
https://crrev.com/960b8cda43c4d804f79ecf75b82559751f21819c
Cr-Commit-Position: refs/heads/master@{#438108}