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

Adding keyboard shortcut test cases #29

Merged
merged 1 commit into from Nov 15, 2017
Merged

Adding keyboard shortcut test cases #29

merged 1 commit into from Nov 15, 2017

Conversation

@kjozwiak
Copy link
Member

kjozwiak commented Nov 7, 2017

Fix #28 and added keyboard shortcut test cases into both the wikitemplate-cr-upgrade.md and wikitemplate.md templates.

@kjozwiak kjozwiak added the enhancement label Nov 7, 2017
@kjozwiak kjozwiak self-assigned this Nov 7, 2017
@kjozwiak kjozwiak requested review from luixxiul, srirambv and LaurenWags Nov 7, 2017
Copy link
Contributor

luixxiul left a comment

how about capitalizing command and ctrl?

@luixxiul
Copy link
Contributor

luixxiul commented Nov 7, 2017

If we tackle with this, ideally we would need various keyboards to cover as many scenarios as possible.

  • Dvorak layout
  • Foreign layout other than en_US (which I cannot as I have only en_US version, Realforce)

Also each case should be tested on different language settings to cover an issue like brave/browser-laptop#9248 and brave/browser-laptop#6428, which are unresolved 😞

To make the manual QA meaningful, I think those should be addressed. If not, why wouldn't we just let the automated tests do the stuff ... ? I'm not quite sure what the object to check it manually is 🤔

@srirambv
Copy link
Contributor

srirambv commented Nov 7, 2017

Covering all other layouts apart from US will be difficult and quite impossible IMO. We could just do it of EN_US for now(shortcuts being similar across majority of other keyboards).

@luixxiul
Copy link
Contributor

luixxiul commented Nov 8, 2017

Also there is a lot of other shortcuts according to the list https://community.brave.com/t/brave-browser-shortcuts-currently-for-windows-only/10001 compiled by @eljuno.

@srirambv
Copy link
Contributor

srirambv commented Nov 8, 2017

Close/Reopen of tabs/windows and shortcuts for brave pages as listed in the change should be sufficient. May be we can link the community page and say verify if these work . May be these keyboard shortcuts test is needed only for CR upgrades and not the normal releases/hotfix

@kjozwiak
Copy link
Member Author

kjozwiak commented Nov 13, 2017

@luixxiul I agree that there's a lot of other shortcuts and keyboard layouts that I haven't added but as @srirambv mentioned above, it would be very difficult to add all the different cases, it's not really feasible/scalable right now. The main purpose of these additions is for a quick sanity check to make sure that some of the main/popular keyboard shortcuts are working as expected so we don't have issues like brave/browser-laptop#11331 which was the main driver to get hotfix4 released last week.

@srirambv, do we only see keyboard shortcut regressions during CR upgrades? Have we seen regressions in normal releases/hotfix? I definitely don't want to spin up another hotfix because we've missed a popular keyboard shortcut that upset several power users.

May be we can link the community page and say verify if these work.

That's a good idea... We can leverage our community and ask them to try the full shortcut list against several different keyboards and languages.

Hopefully in the future we can cover all these cases but right now, we just need to make sure we're not regressing the most used shortcuts, especially shortcuts that help users power navigate.

@kjozwiak kjozwiak force-pushed the keyboardShortcuts branch from f1bb680 to 36a9845 Nov 14, 2017
@kjozwiak
Copy link
Member Author

kjozwiak commented Nov 14, 2017

Fixed capitalization..... Capitalized the first letter for the following keys:

  • Shift
  • Option
  • Command
  • Ctrl

Checked several documentation resources online, including Googles... All seem to be using the above format in terms of capitalization.

Copy link
Contributor

srirambv left a comment

++

@srirambv srirambv merged commit bd9034e into master Nov 15, 2017
@kjozwiak kjozwiak deleted the keyboardShortcuts branch Apr 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.