New issue
Advanced search Search tips

Issue 787137 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug

Blocking:
issue 25503



Sign in to add a comment

Conversions between ui::Event and content::NativeKeyboardEvent to ui::Accelerator lose AltGraph modifier

Project Member Reported by w...@chromium.org, Nov 20 2017

Issue description

AltGraph is not handled treated as a modifier when processing accelerators:

- ui::Accelerator does not include EF_ALTGR_DOWN in its modifiers-mask, causing it to be masked-out.
- GetAcceleratorFromNativeWebKeyboardEvent has its own hand-rolled helper to convert WebInputEvent modifiers to ui::Event modifiers, to pass to the ui::Accelerator constructor, which, again, is not AltGraph-aware.
 

Comment 1 by w...@chromium.org, Nov 20 2017

Blocking: 25503
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e66269c362f753cd57f5bf5bc8d33c7c81bbbdce

commit e66269c362f753cd57f5bf5bc8d33c7c81bbbdce
Author: Wez <wez@chromium.org>
Date: Wed Nov 22 00:17:45 2017

Add AltGraph to the set of modifier flags known to Accelerators.

ui::Accelerator instances built from ui::Events with the AltGraph flag
set lose that modifier, leading to potential for confusion with other
accelerator keys, e.g: both the Alt and AltGraph keys generate events
with key_code=VKEY_MENU, and may only be distinguished by the modifiers
set on the events.

This CL adds knowledge of AltGraph to the ui::Accelerator constructors,
and migrates the CreateAcceleratorFromNativeKeyboardEvent() helper to
use the //ui/events/blink utility functions to translate modifier
flags.

Bug:  787137 ,  25503 
Change-Id: I8bde31947b99d9ae113ed5a38053ae8c5e46d32f
Reviewed-on: https://chromium-review.googlesource.com/780449
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518472}
[modify] https://crrev.com/e66269c362f753cd57f5bf5bc8d33c7c81bbbdce/ui/base/accelerators/accelerator.cc
[modify] https://crrev.com/e66269c362f753cd57f5bf5bc8d33c7c81bbbdce/ui/content_accelerators/BUILD.gn
[modify] https://crrev.com/e66269c362f753cd57f5bf5bc8d33c7c81bbbdce/ui/content_accelerators/accelerator_util.cc

Comment 3 by w...@chromium.org, Nov 29 2017

Status: Fixed (was: Started)

Comment 4 by w...@chromium.org, Nov 30 2017

Status: Started (was: Fixed)
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/891184d01481bfe9af1318345ca22211ff53e020

commit 891184d01481bfe9af1318345ca22211ff53e020
Author: Wez <wez@chromium.org>
Date: Mon Dec 04 01:16:45 2017

Fix WebEventModifiersToEventFlags to handle the AltGraph modifier bit.

The accelerator system was fixed to use this function when mapping the
modifiers in a WebInputEvent to construct an Accelerator, and the
reverse mapping, from ui::Event flags to WebInputEvent modifiers,
already handles the AltGraph modifier correctly.

A simple test is also added to verify that the two functions both handle
the required set of modifier flags.

Bug:  787137 ,  25503 
Change-Id: I53e5353dc4f1fa5eba15f1b91ec4c361e50264db
Reviewed-on: https://chromium-review.googlesource.com/802081
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521247}
[modify] https://crrev.com/891184d01481bfe9af1318345ca22211ff53e020/ui/events/blink/blink_event_util.cc
[modify] https://crrev.com/891184d01481bfe9af1318345ca22211ff53e020/ui/events/blink/blink_event_util_unittest.cc

Comment 6 by w...@chromium.org, Dec 4 2017

Labels: -M-64 M-65
Status: Fixed (was: Started)

Sign in to add a comment