Last Comment Bug 769190 - Should change the acceptable value of the argument of getModifierState()
: Should change the acceptable value of the argument of getModifierState()
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
Depends on:
Blocks: 680830 630811 731878
  Show dependency treegraph
 
Reported: 2012-06-28 02:41 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2012-08-07 05:09 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
verified
fixed


Attachments
part.1 Rename Scroll and Win to ScrollLock and OS for the argument of getModifierState() (6.92 KB, patch)
2012-07-05 02:04 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
jst: superreview+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review
part.2 Rename nsInputEvent::IsWin() and widget::MODIFIER_(SCROLL|WIN) to nsInputEvent::IsOS() and widget::MODIFIER_(SCROLLLOCK|OS) (10.56 KB, patch)
2012-07-05 02:06 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-06-28 02:41:25 PDT
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] (Mozilla Japan) 2012-07-05 02:04:56 PDT
Created attachment 639264 [details] [diff] [review]
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] (Mozilla Japan) 2012-07-05 02:06:02 PDT
Created attachment 639266 [details] [diff] [review]
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] (Mozilla Japan) 2012-07-05 02:08:45 PDT
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 5 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-07-05 17:52:22 PDT
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] (Mozilla Japan) 2012-07-05 17:58:11 PDT
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 8 Alex Keybl [:akeybl] 2012-07-08 18:10:34 PDT
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] (Mozilla Japan) 2012-07-08 19:18:07 PDT
(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] 2012-07-09 13:12:43 PDT
(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] (Mozilla Japan) 2012-07-09 19:22:22 PDT
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] (Mozilla Japan) 2012-07-09 20:03:51 PDT
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 2012-07-12 15:51:41 PDT
fixed on 15, no need to track.

Note You need to log in before you can comment on or make changes to this bug.