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

fix: don't append Shift modifier text twice to accelerators #15400

Merged
merged 5 commits into from Oct 31, 2018

Conversation

Projects
None yet
5 participants
@brenca
Copy link
Member

commented Oct 25, 2018

Description of Change

Fixes #15392. For some reason our patch applied a Shift modifier to a whole bunch of accelerator characters (like Backspace, but Tab and Enter were also affected), my best guess is that those used to be broken with Shift but now are working properly in chromium.

I'll backport this manually, trop couldn't deal with 4-0-x.

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • PR title follows semantic commit guidelines

Release Notes

Notes: fixed some accelerators having Shift appended to them twice

@@ -19,17 +19,17 @@ index 7e55ef366ac8320f730cdcb268453b1fa2710887..c3fb98b426cd7c12f66eaaf358f4ff18
@@ -21,9 +22,7 @@
#include <windows.h>
#endif

This comment has been minimized.

Copy link
@nornagon

nornagon Oct 26, 2018

Contributor

those spaces are important, leave them in please! this patch should be exactly as exported by git-export-patches.

This comment has been minimized.

Copy link
@brenca

brenca Oct 26, 2018

Author Member

I knew there was something up with those spaces, gonna fix asap

@brenca brenca force-pushed the brenca/fix-shift-accelerators branch from 65559f0 to 55dabaf Oct 29, 2018

@ckerr

ckerr approved these changes Oct 30, 2018

@ckerr

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

@nornagon thoughts on the revised PR?

@nornagon
Copy link
Contributor

left a comment

Looks good to me, I still want tests though :(

@brenca brenca force-pushed the brenca/fix-shift-accelerators branch from 55dabaf to 569e264 Oct 30, 2018

@brenca

This comment has been minimized.

Copy link
Member Author

commented Oct 30, 2018

@nornagon ask, and thou shall receive 🙇

@@ -155,6 +155,13 @@ base::string16 Menu::GetSublabelAt(int index) const {
return model_->GetSublabelAt(index);
}

base::string16 Menu::GetAcceleratorTextAt(int index) const {
auto* accelerator = new ui::Accelerator();

This comment has been minimized.

Copy link
@deepak1556

deepak1556 Oct 30, 2018

Member

Who owns this accelerator ? I don't see any explicit ownership claim in the code path, maybe I am missing something here.

This comment has been minimized.

Copy link
@nornagon

nornagon Oct 30, 2018

Contributor

oooh, good catch! this should probably be on the stack.

Suggested change
auto* accelerator = new ui::Accelerator();
ui::Accelerator accelerator;
model_->GetAcceleratorAtWithParams(index, true, &accelerator);

This comment has been minimized.

Copy link
@brenca

brenca Oct 30, 2018

Author Member

Done! Thanks for catching this

@brenca brenca force-pushed the brenca/fix-shift-accelerators branch from 976b4d1 to 880b9bb Oct 30, 2018

@brenca brenca force-pushed the brenca/fix-shift-accelerators branch from 3a6c073 to 2b0e4be Oct 30, 2018

@jkleinsc jkleinsc merged commit aa6f7a5 into master Oct 31, 2018

26 checks passed

Absolute Zero
Semantic Pull Request ready to be squashed
Details
WIP ready for review
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
electron-arm-testing Build #20181030.23 succeeded
Details
electron-arm64-testing Build #20181030.23 succeeded
Details
electron-lint Build #20181030.29 succeeded
Details
electron-mas-testing Build #20181030.31 succeeded
Details
electron-osx-testing Build #20181030.32 succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Oct 31, 2018

Release Notes Persisted

fixed some accelerators having Shift appended to them twice

@jkleinsc jkleinsc deleted the brenca/fix-shift-accelerators branch Oct 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.