769190 - Should change the acceptable value of the argument of getModifierState()

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

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

The D3E spec was updated:

http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-KeyboardEvent-getModifierState

The "Scroll" is renamed to "ScrollLock". And "Win" is renamed to "OS". The new values are also defined in the key value list.

However, if we changed them, that would break the compatibility with IE9, any idea, smaug? Should we support both values? Or should we just drop supporting legacy values? WebKit hasn't supported this API yet. So, I think this isn't used on so many sites.

Comment 1

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

Attachment: part.1 Rename Scroll and Win to ScrollLock and OS for the argument of getModifierState()

This is the minimal patch fixing the behavior for the latest D3E spec.

Comment 2

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

Attachment: part.2 Rename nsInputEvent::IsWin() and widget::MODIFIER_(SCROLL|WIN) to nsInputEvent::IsOS() and widget::MODIFIER_(SCROLLLOCK|OS)

This patch renames the internal constants and methods.

Comment 3

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

After we implement KeyboardEvent.getModifierState() and MouseEvent.getModifierState(), the spec for the argument of these methods were changed. "Scroll" was renamed to "ScrollLock" and "Win" was renamed to "OS". We should fix the key names before releasing our first implemented version (Fx15).

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/013bdf414357
https://hg.mozilla.org/integration/mozilla-inbound/rev/c41bf7b9364c

Comment 5

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

Comment on attachment 639264 [details] [diff] [review]
part.1 Rename Scroll and Win to ScrollLock and OS for the argument of getModifierState()

[Approval Request Comment]

Bug caused by (feature/regressing bug #): Starting Fx15, Gecko supports DOM Level3 Events KeyboardEvent.getModifierState() and MouseEvent.getModifierState(). They has an argument whose value is a key name of checking modifier state. In the latest draft, two key names are changed: "Scroll" -> "ScrollLock", "Win" -> "OS". We should use the new standard key names instead of the older draft's names.

User impact if declined: If we wouldn't land them on Fx15, Fx15 would provide "wrong" API for web developers. It might cause compatibility problem in the future. However, the methods have not been supported on WebKit yet. Therefore, the possibility isn't high.

Testing completed (on m-c, etc.): This patch includes the test.

Risk to taking this patch (and alternatives if risky): This patch changes the new constant names in nsIDOMWindowUtils. They were implemented for testing the new APIs. If some addons used them when it worked on Nightly or Aurora, the compatibility would be broken if they checked the two keys' state. However, I think that such problem should be fixed in the addons.

String or UUID changes made by this patch: Just changed the constants of nsIDOMWindowUtils. So, the UUID isn't changed.

Comment 6

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

Comment on attachment 639266 [details] [diff] [review]
part.2 Rename nsInputEvent::IsWin() and widget::MODIFIER_(SCROLL|WIN) to nsInputEvent::IsOS() and widget::MODIFIER_(SCROLLLOCK|OS)

[Approval Request Comment]

Bug caused by (feature/regressing bug #): See previous comment. This patch only changes the internal constants and method names for the new key names.

User impact if declined: none.

Testing completed (on m-c, etc.): The tests in previous patch covers this too.

Risk to taking this patch (and alternatives if risky): Noting but something to be unexpected might happen though...

String or UUID changes made by this patch: No.

The part.1 patch is minimal patch for fixing this bug. This part.2 patch isn't so important. If we wouldn't land this patch on Aurora, it would cause rejecting some patches which were created for mozilla-central. But it's not important.

Comment 7

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/013bdf414357
https://hg.mozilla.org/mozilla-central/rev/c41bf7b9364c

Comment 8

[Alex Keybl [:akeybl]]

Comment on attachment 639264 [details] [diff] [review]
part.1 Rename Scroll and Win to ScrollLock and OS for the argument of getModifierState()

[Triage Comment]
Let's take these correctness patches if we do land on top of this code. Otherwise, it's not clear we need these from the user perspective.

Comment 9

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

(In reply to Alex Keybl [:akeybl] from comment #8)
> Comment on attachment 639264 [details] [diff] [review]
> part.1 Rename Scroll and Win to ScrollLock and OS for the argument of
> getModifierState()
> 
> [Triage Comment]
> Let's take these correctness patches if we do land on top of this code.

Sorry, I don't understand what did "land on top of this code" mean.

> Otherwise, it's not clear we need these from the user perspective.

IE9 is using the older draft's values (i.e., "Scroll" and "Win"). If Fx15 would ship without these patches, some web applications would not work fine on Fx16 or later if they are tested with Fx15 and their developers don't know the changes of the values in D3E draft. If we use the new values (i.e., "ScrollLock" and "OS") from the first release of the API (i.e., Fx15), we can appeal the difference in the new feature list.

Comment 10

[Alex Keybl [:akeybl]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> (In reply to Alex Keybl [:akeybl] from comment #8)
> > [Triage Comment]
> > Let's take these correctness patches if we do land on top of this code.
> 
> Sorry, I don't understand what did "land on top of this code" mean.
> 
> > Otherwise, it's not clear we need these from the user perspective.
> 
> IE9 is using the older draft's values (i.e., "Scroll" and "Win"). If Fx15
> would ship without these patches, some web applications would not work fine
> on Fx16 or later if they are tested with Fx15 and their developers don't
> know the changes of the values in D3E draft. If we use the new values (i.e.,
> "ScrollLock" and "OS") from the first release of the API (i.e., Fx15), we
> can appeal the difference in the new feature list.

Hi Masayuki - your original approval request noted that "User impact if declined: none." The justification above does appear to be user impact. Given that, approving.

Comment 11

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

I meant that the first patch has the impact for users but the second patch only cleans the code.

https://hg.mozilla.org/releases/mozilla-aurora/rev/b0b269418ecb
https://hg.mozilla.org/releases/mozilla-aurora/rev/28f2c543ac9f

Comment 12

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

updated some documents in MDN:

https://developer.mozilla.org/en/Firefox_15_for_developers#section_4
https://developer.mozilla.org/en/DOM/KeyboardEvent#section_7

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

fixed on 15, no need to track.

Comment 14

[Ioana (away)]

content/events/test/test_dom_keyboard_event.html and content/events/test/test_dom_mouse_event.html verify this bug fix and they are passing on all the OSs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=14176088&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=14174223&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=14179177&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=14178606&full=1&branch=mozilla-beta