Closed Bug 769190 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16
Tracking Status
firefox13 --- unaffected
firefox14 --- unaffected
firefox15 --- verified
firefox16 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(2 files)

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.
This is the minimal patch fixing the behavior for the latest D3E spec.
Attachment #639264 - Flags: review?(bugs)
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).
Attachment #639264 - Flags: review?(bugs) → review+
Attachment #639266 - Flags: review?(bugs) → review+
Attachment #639264 - Flags: superreview?(jst) → superreview+
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.
Attachment #639264 - Flags: approval-mozilla-aurora?
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.
Attachment #639266 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/013bdf414357
https://hg.mozilla.org/mozilla-central/rev/c41bf7b9364c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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.
Attachment #639264 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #639266 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
(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.
(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.
Attachment #639264 - Flags: approval-mozilla-aurora- → approval-mozilla-aurora+
Attachment #639266 - Flags: approval-mozilla-aurora- → approval-mozilla-aurora+
fixed on 15, no need to track.
You need to log in before you can comment on or make changes to this bug.