Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't include extension actions in tab order until focused #59861

Merged
merged 3 commits into from
Oct 16, 2018

Conversation

ramya-rao-a
Copy link
Contributor

@ramya-rao-a ramya-rao-a commented Oct 2, 2018

This PR attempts to fix the bug #51496

In other lists (Search results, staged/unstaged files in gitview), only the focused list item's actions are shown. Therefore this problem doesnt exist there.

In Extensions viewlet, the actions on all list items are enabled and visible. This PR adds the below as per prior discussions: #59280

  • Extend ActionItem class so as to not set tab index on the action item unless it receives focus
  • Extend IExtension to include a event emitter and listener. The extensions list view would deal with this new class and fire the event on the extension list item that gets/loses focus
  • Renderer for the extension list items will use the extended ActionItem in the action bar. It will listen to the focus change event on each extension list item, and update the action item appropriately

@ramya-rao-a ramya-rao-a self-assigned this Oct 2, 2018
sandy081 added a commit that referenced this pull request Oct 3, 2018
sandy081 added a commit that referenced this pull request Oct 3, 2018
@sandy081
Copy link
Member

sandy081 commented Oct 12, 2018

@ramya-rao-a I am not a big fan of having two different UI / Keyboard navigation based on accessibility is enabled or not.

I agree that not showing install actions is not good.

I would rather fix keyboard navigation to be similar always like the better workaround we discussed here.

@ramya-rao-a ramya-rao-a force-pushed the ramyar/extension-actions-taborder branch from 80d78db to 79480ec Compare October 15, 2018 03:14
@ramya-rao-a ramya-rao-a changed the title In screenreader mode, dont show extension actions until focused/selected Don't include extension actions in tab order until focused Oct 15, 2018
@ramya-rao-a ramya-rao-a force-pushed the ramyar/extension-actions-taborder branch from 79480ec to ed4aa84 Compare October 15, 2018 03:20
@ramya-rao-a
Copy link
Contributor Author

@sandy081 PR is updated as per prior discussions. See
#59861 (comment) for the summary.

}
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sandy081 DropDownMenuActionItem is used for Enable and Disable features in the extension editor as well. Therefore, I couldn't simply make it extend TabOnlyOnFocusActionItem instead of ActionItem. So now I have the same logic repeated in 2 places. Any ideas on how we can improve this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about calling TabOnlyOnFocusActionItem -> ExtensionActionItem that takes tabOnlyOnFocus has input (just like DropDownMenuActionItem) ?

}
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about calling TabOnlyOnFocusActionItem -> ExtensionActionItem that takes tabOnlyOnFocus has input (just like DropDownMenuActionItem) ?

@sandy081 sandy081 added this to the October 2018 milestone Oct 16, 2018
@sandy081
Copy link
Member

LGTM

@ramya-rao-a
Copy link
Contributor Author

Thanks @sandy081

I've updated the code as per #59861 (comment)

@ramya-rao-a ramya-rao-a merged commit f3eb7ad into master Oct 16, 2018
@ramya-rao-a ramya-rao-a deleted the ramyar/extension-actions-taborder branch October 16, 2018 18:03
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants