308565 - `altKey` of `keypress` event is always false on macOS

Status: ASSIGNED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[Adam Winer]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050721 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050721 Firefox/1.0+

In the following HTML snippet:

<input type="text" onkeypress="alert('alt? ' + event.altKey)">

... entering the field and hitting the "alt-A" keystroke (aka option-A) on the
Mac just shows "alt? false".  This correctly shows "alt? true" on Safari.  It
does work for onkeydown and onkeyup.

Reproducible: Always

Steps to Reproduce:
1. Add the following to a document:
 <input type="text" onkeypress="alert('alt? ' + event.altKey)">
2. Enter the field.
3. press option-a


Actual Results:  
You see "alt? false" in an alert.

Expected Results:  
You should have seen "alt? true in an alert.

MacOS 10.4.2
Repros in Firefox 1.0.3 and Deerpark alpha 2

Comment 1

[Jo Hermans]

see bug 300678 : that piece of JS code correctly determines the state of the
altkey (using onkeydown)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Mac widgetry issue.

Comment 3

[Rahul Abrol]

it seems like one must use alt+(ctrl|cmd) to get a "true" value here.  for keycode 18 (alt) in about:config prefs. only the ctrl+alt variant works, though.

there must be some way to allow plain old (alt) as a modifier key...

Comment 4

[Rahul Abrol]

this should probably get marked as a duplicate of bug 44259.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Why?  It doesn't seem very related to me...

Comment 6

[Rahul Abrol]

(In reply to comment #5)
> Why?  It doesn't seem very related to me...
> 

yeah, maybe not.  i assumed that fixing bug 300678 would also take care of this one, and that was just marked as a dupe of 44259, ergo...

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

If it's not a dupe, then it's definitely a bug.  -->Cocoa

Comment 8

[Josh Aas]

Attachment: test case

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Moving all keyboard/IME handling bugs to DOM: UI Events & Focus Handling component.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Indeed, this could cause web-compat issue. I think that we can fix this bug with bug 1596916 because we need new flag in WidgetKeyboardEvent which indicate whether the eKeyPress event should introduce a character or not.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 308565 - Make `IMEInputHandler` stop unsetting `Alt` modifier state from `eKeyPress` event r=m_kato,smaug

The other browsers on macOS set altKey to true of keypress
events even when they introduce a chracter so that we should
follow same behavior for web-compat.

WidgetKeyboardEvent::IsInputtingText() has already ignore
MODIFIER_ALT flag only on macOS.
https://searchfox.org/mozilla-central/rev/4d9cd186767978a99dafe77eb536a9525980e118/widget/TextEvents.h#257-261
So, only IMEInputHandler stop the hack only for Alt state.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that we should put it off to change this our traditional behavior even though the new behavior is same as Blink/WebKit until COVID-19 calms down because we've experienced a lot of breakage with this kind of changes. What do you think?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Sounds reasonable.
In theory we could land the patch with a pref and enable the pref only on Nightly.

But this is a really old bug, so probably not hurry to land anything. Perhaps we want a meta bug to track which patches need to land later?

Comment 14

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Olli Pettay [:smaug] from comment #13)

Sounds reasonable.
In theory we could land the patch with a pref and enable the pref only on Nightly.

But this is a really old bug, so probably not hurry to land anything. Perhaps we want a meta bug to track which patches need to land later?

Agreed to not hurry! Thank you for raising the concern, Masayuki.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thanks, filed bug 1625822 for tracking bugs like this.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Smaug: I think that it's fine to land this since a lot of big changes have been landed in various area of Gecko. WDYT?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Would it make sense to add a pref and enable first on Nightly? (sorry about massive delay in responding)

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:masayuki, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I don't have much time to land this because I assume that this would cause trouble in some web apps (at least, I need to investigate how it causes).