Skip to content

Commit

Permalink
Revert "[Switch Access] Refactor focus for clarity around back button"
Browse files Browse the repository at this point in the history 
This reverts commit 91a61a3.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 647532 as the
culprit for flakes in the build cycles as shown on:
https://analysis.chromium.org/p/chromium/flake-portal/analysis/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vOTFhNjFhMzEwY2M5Yjg0NTE3MTllOWJkNmRkMDQ5MTk5MzVlYTBmMQw

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.memory/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/32512

Sample Failed Step: viz_browser_tests

Sample Flaky Test: SwitchAccessNavigationManagerTest.TabNavigation

Original change's description:
> [Switch Access] Refactor focus for clarity around back button
> 
> Bug: None
> Change-Id: I3318ae4d5f37d67e7fe5abb894f9edacddc785d4
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1549944
> Commit-Queue: Anastasia Helfinstein <anastasi@google.com>
> Reviewed-by: Katie Dektar <katie@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#647532}

Change-Id: Ia9a3dd2924bc2e7d5719818635597081c072b51c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: None
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1552494
Cr-Commit-Position: refs/heads/master@{#647617}
  • Loading branch information
Findit committed Apr 4, 2019
1 parent a431384 commit 6954e12
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 134 deletions.
25 changes: 2 additions & 23 deletions chrome/browser/resources/chromeos/switch_access/back_button_manager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,6 @@ class BackButtonManager {

/** @private {PanelInterface} */
this.menuPanel_;

/** @private {chrome.automation.AutomationNode} */
this.buttonNode_;
}

/**
Expand Down Expand Up @@ -66,28 +63,10 @@ class BackButtonManager {
}

/**
* Returns the button node, if we have found it.
* @return {chrome.automation.AutomationNode}
*/
buttonNode() {
return this.buttonNode_;
}

/**
* Sets the reference to the menu panel and finds the back button node.
* Sets the reference to the menu panel.
* @param {!PanelInterface} menuPanel
*/
init(menuPanel, desktop) {
setMenuPanel(menuPanel) {
this.menuPanel_ = menuPanel;
this.buttonNode_ =
new AutomationTreeWalker(
desktop, constants.Dir.FORWARD,
{visit: (node) => node.htmlAttributes.id === SAConstants.BACK_ID})
.next()
.node;
// TODO(anastasi): Determine appropriate event and listen for it, rather
// than setting a timeout.
if (!this.buttonNode_)
setTimeout(this.init.bind(this, menuPanel, desktop), 500);
}
}
97 changes: 52 additions & 45 deletions chrome/browser/resources/chromeos/switch_access/navigation_manager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,13 @@ class NavigationManager {
*/
this.scopeStack_ = [];

/**
* Keeps track of when we're visiting the current scope as an actionable
* node.
* @private {boolean}
*/
this.visitingScopeAsActionable_ = false;

/**
* Keeps track of if we are currently in a system menu.
* @private {boolean}
Expand Down Expand Up @@ -109,31 +116,36 @@ class NavigationManager {

/**
* Find the previous interesting node and update |this.node_|. If there is no
* previous node, |this.node_| will be set to the back button.
* previous node, |this.node_| will be set to the youngest descendant in the
* SwitchAccess scope tree to loop again.
*/
moveBackward() {
if (this.menuManager_.moveBackward())
return;

if (this.node_ === this.backButtonManager_.buttonNode()) {
if (SwitchAccessPredicate.isActionable(this.scope_))
this.setCurrentNode_(this.scope_);
else
this.setCurrentNode_(this.youngestDescendant_(this.scope_));
return;
}

this.startAtValidNode_();

let treeWalker = new AutomationTreeWalker(
this.node_, constants.Dir.BACKWARD,
SwitchAccessPredicate.restrictions(this.scope_));

// Special case: Scope is actionable
if (this.node_ === this.scope_ && this.visitingScopeAsActionable_) {
this.visitingScopeAsActionable_ = false;
this.setCurrentNode_(this.node_);
return;
}

let node = treeWalker.next().node;

if (node === this.scope_)
node = this.backButtonManager_.buttonNode();
// Special case: Scope is actionable
if (node === this.scope_ && SwitchAccessPredicate.isActionable(node)) {
this.showScopeAsActionable_();
return;
}

// If treeWalker returns undefined, that means we're at the end of the tree
// and we should start over.
if (!node)
node = this.youngestDescendant_(this.scope_);

Expand All @@ -157,35 +169,24 @@ class NavigationManager {

this.startAtValidNode_();

const backButtonNode = this.backButtonManager_.buttonNode();
if (this.node_ === this.scope_ && backButtonNode) {
this.setCurrentNode_(backButtonNode);
return;
}

// Replace the back button with the scope to find the following node.
if (this.node_ === backButtonNode)
this.node_ = this.scope_;

let treeWalker = new AutomationTreeWalker(
this.node_, constants.Dir.FORWARD,
SwitchAccessPredicate.restrictions(this.scope_));

// Special case: Scope is actionable.
if (this.node_ === this.scope_ &&
SwitchAccessPredicate.isActionable(this.node_) &&
!this.visitingScopeAsActionable_) {
this.showScopeAsActionable_();
return;
}
this.visitingScopeAsActionable_ = false;

let node = treeWalker.next().node;
// If treeWalker returns undefined, that means we're at the end of the tree
// and we should start over.
if (!node) {
if (SwitchAccessPredicate.isActionable(this.scope_)) {
node = this.scope_;
} else if (backButtonNode) {
node = backButtonNode;
} else {
this.node_ = this.scope_;
this.moveForward();
return;
}
}
if (!node)
node = this.scope_;

// We can't interact with the desktop node, so skip it.
if (node === this.desktop_) {
Expand Down Expand Up @@ -246,8 +247,11 @@ class NavigationManager {
}

if (this.node_ === this.scope_) {
this.node_.doDefault();
return;
// If we're visiting the scope as actionable, perform the default action.
if (this.visitingScopeAsActionable_) {
this.node_.doDefault();
return;
}
}

if (SwitchAccessPredicate.isGroup(this.node_, this.scope_)) {
Expand Down Expand Up @@ -328,7 +332,7 @@ class NavigationManager {
* @return {!MenuManager}
*/
connectMenuPanel(menuPanel) {
this.backButtonManager_.init(menuPanel, this.desktop_);
this.backButtonManager_.setMenuPanel(menuPanel);
return this.menuManager_.connectMenuPanel(menuPanel);
}

Expand Down Expand Up @@ -483,17 +487,20 @@ class NavigationManager {
return;
this.scopeStack_.push(this.scope_);
this.scope_ = node;

// The first node will come immediately after the back button, so we set
// |this.node_| to the back button and call |moveForward|.
const backButtonNode = this.backButtonManager_.buttonNode();
if (backButtonNode)
this.node_ = backButtonNode;
else
this.node_ = this.scope_;
this.node_ = node;
this.moveForward();
}

/**
* Show the current scope as an actionable item.
*/
showScopeAsActionable_() {
this.node_ = this.scope_;
this.visitingScopeAsActionable_ = true;

this.updateFocusRings_(this.node_.location);
}

/**
* Checks if this.node_ is valid. If so, do nothing.
*
Expand Down Expand Up @@ -528,8 +535,8 @@ class NavigationManager {
* @private
*/
updateFocusRings_(opt_focusRect) {
if (this.node_ === this.backButtonManager_.buttonNode()) {
this.backButtonManager_.show(this.scope_);
if (!opt_focusRect && this.node_ === this.scope_) {
this.backButtonManager_.show(this.node_);

this.primaryFocusRing_.rects = [];
this.scopeFocusRing_.rects = [this.scope_.location];
Expand Down
84 changes: 18 additions & 66 deletions chrome/browser/resources/chromeos/switch_access/navigation_manager_test.extjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,24 +29,10 @@ function navigateToWebpage(desktop) {
switchAccess.moveForward();
}

function navigateToFirstTab(desktop) {
assertTrue(desktop != null);
const node = new AutomationTreeWalker(desktop, constants.Dir.FORWARD,
{ visit: (node) => node.role === chrome.automation.RoleType.TAB }
).next().node;

assertTrue(node != null);
switchAccess.navigationManager_.node_ = node;
}

function currentNode() {
return switchAccess.navigationManager_.node_;
}

function backButton() {
return switchAccess.navigationManager_.backButtonManager_.buttonNode();
}

TEST_F('SwitchAccessNavigationManagerTest', 'SelectButton', function() {
const website = `data:text/html;charset=utf-8,
<button id="test" aria-pressed="false"></button>
Expand Down Expand Up @@ -100,10 +86,10 @@ TEST_F('SwitchAccessNavigationManagerTest', 'MoveForward', function() {
const button3 = currentNode();
assertEquals('button3', button3.name);

// Moving forwards should take us to the back button.
// TODO(anastasi): Determine why the back button doesn't show up here.
//switchAccess.moveForward();
//assertEquals(backButton(), currentNode());
switchAccess.moveForward();

// check that the initialScope is the final element.
assertEquals(currentNode(), initialScope);

switchAccess.moveForward();

Expand Down Expand Up @@ -131,12 +117,12 @@ TEST_F('SwitchAccessNavigationManagerTest', 'MoveBackward', function() {

switchAccess.moveBackward();

// Moving backwards from the first button should take us to the back button.
// TODO(anastasi): Determine why the back button doesn't show up here.
//switchAccess.moveBackward();
//assertEquals(backButton(), currentNode());
// Moving backwards from the first button should take us to the initialScope.
assertEquals(currentNode(), initialScope);

switchAccess.moveBackward();

// Moving backwards from the back button should take us to the third button.
// Moving backwards from the initialScope should take us to the last button.
const button3 = currentNode();
assertEquals('button3', button3.name);

Expand Down Expand Up @@ -168,62 +154,28 @@ TEST_F('SwitchAccessNavigationManagerTest', 'MoveBackAndForth', function() {
const button1 = currentNode();
assertEquals('button1', button1.name);

// Moving backwards from the first button should take us to the back button.
// TODO(anastasi): Determine why the back button doesn't show up here.
//switchAccess.moveBackward();
//assertEquals(backButton(), currentNode());
switchAccess.moveBackward();

// Moving backwards from the first button should take us to the initialScope.
assertEquals(currentNode(), initialScope);

switchAccess.moveBackward();

// Moving backwards from the back button should take us to the last button.
// Moving backwards from the initialScope should take us to the last button.
const button3 = currentNode();
assertEquals('button3', button3.name);

// Moving forwards from the last button should take us to the back button.
// TODO(anastasi): Determine why the back button doesn't show up here.
//switchAccess.moveForward();
//assertEquals(backButton(), currentNode());

switchAccess.moveForward();
assertEquals(currentNode(), button1);

switchAccess.moveForward();
const button2 = currentNode();
assertEquals('button2', button2.name);
});
});

TEST_F('SwitchAccessNavigationManagerTest', 'TabNavigation',
function() {
const website = `data:text/html;charset=utf-8,
<button>button1</button>`;

this.runWithLoadedTree(website, function(desktop) {
navigateToFirstTab(desktop);
const tabNode = currentNode();

// Selecting the tab should enter it, placing our focus on the close button.
switchAccess.selectCurrentNode();

const closeButton = currentNode();
assertEquals('Close', closeButton.name);
assertEquals(chrome.automation.RoleType.BUTTON, closeButton.role);
assertEquals(currentNode(), initialScope);

switchAccess.moveForward();
assertEquals(tabNode, currentNode());


// Moving forwards from the tab node should take us to the back button.
// TODO(anastasi): Determine why the back button doesn't show up here.
//switchAccess.moveForward();
//assertEquals(backButton(), currentNode());
assertEquals(currentNode(), button1);

switchAccess.moveForward();
assertEquals(closeButton, currentNode());

// Moving backwards from the close button should take us to the back button.
// TODO(anastasi): Determine why the back button doesn't show up here.
//switchAccess.moveBackward();
//assertEquals(backButton, currentNode());
const button2 = currentNode();
assertEquals('button2', button2.name);
});
});

0 comments on commit 6954e12

Please sign in to comment.