Closed Bug 930893 Opened 11 years ago Closed 10 years ago

Implement D3E KeyboardEvent constructor

Categories

(Core :: DOM: Events, enhancement)

Product:
Core
Component:
DOM: Events
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla31

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(3 files)

testcase
9.18 KB, text/html
Details
part.1 Implement constructor of KeyboardEvent
13.01 KB, patch
smaug
: review+
Details | Diff | Splinter Review
part.2 Implement KeyboardEvent.initKeyboardEvent()
14.34 KB, patch
smaug
: review+
Details | Diff | Splinter Review 
We've not supported |new KeyboardEvent({ ... });| yet. I'm not sure the reason.

Smaug, is there some reasons? If not, I'll try to implement it.
Flags: needinfo?(bugs)
The reason was that at the time I was implementing various ctors, no spec or spec draft defined
ctor for Key(board)Event.
Flags: needinfo?(bugs)
Okay, then, I'll try to fix this next week.
Attached file testcase — 
Hmm, IE 11 doesn't support KeyboardEvent constructor.

Chromium supports it but web developers cannot initialize keyCode/charCode/which from the JSON argument.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #3)
> Chromium supports it but web developers cannot initialize
> keyCode/charCode/which from the JSON argument.

It is unsurprising because keyCode/charCode/which are not defined in KeyboardEvent.
https://dvcs.w3.org/hg/d4e/raw-file/tip/source_respec.htm#constructor-keyboardevent
I thinks it's reasonable not to support those legacy members for the new interface.
> not defined in KeyboardEvent.

KeyboardEventInit
Depends on: 680830
(In reply to Masatoshi Kimura [:emk] from comment #4)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #3)
> > Chromium supports it but web developers cannot initialize
> > keyCode/charCode/which from the JSON argument.
> 
> It is unsurprising because keyCode/charCode/which are not defined in
> KeyboardEvent.
> https://dvcs.w3.org/hg/d4e/raw-file/tip/source_respec.htm#constructor-
> keyboardevent
> I thinks it's reasonable not to support those legacy members for the new
> interface.

D3E defines them in KeyboardEventInit...
https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#idl-interface-KeyboardEvent-initializers

And I think that all attributes should be able to be initialized at constructing event.
Smaug:

Do you think we should also support initKeyboardEvent() which is defined in D3E?
https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#idl-interface-KeyboardEvent-initializers
Flags: needinfo?(bugs)
D3E defines legacy members in KeyboardEvent, but D4E doesn't.
https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#KeyboardEvent-supplemental-interface
Therefore both specs are self-consistent.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #8)
> Smaug:
> 
> Do you think we should also support initKeyboardEvent() which is defined in
> D3E?
> https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#idl-
> interface-KeyboardEvent-initializers

IMO we shouldn't implement D3E specific things from the start unless they are already implemented. It's hard to remove things once it is implemented.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> And I think that all attributes should be able to be initialized at
> constructing event.

We can infer .keyCode/.charCode/.which from .key/.char. Why all of them should be able to be initialized separately? It's ridiculous.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #7)
> I filed a spec bug:
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=23697

I don't think this is a spec bug. DOM4 removed many legacy things which had been defined in DOM Level 3 Core. Same for D4E.
(In reply to Masatoshi Kimura [:emk] from comment #11)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> > And I think that all attributes should be able to be initialized at
> > constructing event.
> 
> We can infer .keyCode/.charCode/.which from .key/.char.

I don't think that it's good behavior. It sounds keeping compatibility between browsers is too hard. And it causes a lot of additional code in nsDOMKeyboardEvent.

Additionally, I think that synthetic events should be free from the rules of trusted event. They should be able to store inconsistency values between two or more attributes.

# FYI: .char is dropped from D3E and not putting off to UI Events.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #13)
> I don't think that it's good behavior. It sounds keeping compatibility
> between browsers is too hard. And it causes a lot of additional code in
> nsDOMKeyboardEvent.

.keyCode/.charCode/.which is incompatible between browsers anyway as written in the D3E spec.
They we can leave those legacy members uninitialized.

> Additionally, I think that synthetic events should be free from the rules of
> trusted event. They should be able to store inconsistency values between two
> or more attributes.

Our .which is defined in UIEvent (not KeyboardEvent), but UIEventInit can't initialize legacy extensions.
https://mxr.mozilla.org/mozilla-central/source/dom/webidl/UIEvent.webidl
(In reply to Masatoshi Kimura [:emk] from comment #14)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #13)
> > I don't think that it's good behavior. It sounds keeping compatibility
> > between browsers is too hard. And it causes a lot of additional code in
> > nsDOMKeyboardEvent.
> 
> .keyCode/.charCode/.which is incompatible between browsers anyway as written
> in the D3E spec.
> They we can leave those legacy members uninitialized.

The incompatibility is about trusted event. Not for untrusted (synthetic) events.

> > Additionally, I think that synthetic events should be free from the rules of
> > trusted event. They should be able to store inconsistency values between two
> > or more attributes.
> 
> Our .which is defined in UIEvent (not KeyboardEvent), but UIEventInit can't
> initialize legacy extensions.
> https://mxr.mozilla.org/mozilla-central/source/dom/webidl/UIEvent.webidl

Indeed.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #15)
> The incompatibility is about trusted event. Not for untrusted (synthetic)
> events.

Given that Chrome doesn't support .keyCode/.charCode/.which, we will introduce incompatibility if we support them.

> > Our .which is defined in UIEvent (not KeyboardEvent), but UIEventInit can't
> > initialize legacy extensions.
> > https://mxr.mozilla.org/mozilla-central/source/dom/webidl/UIEvent.webidl
> 
> Indeed.

Then why we should be inconsistent about KeyboardEvent?

Anyway, we should finish KeyboardEvent.key before this bug. If we added KeyboardEventInit.keyCode without completing KeyboardEvent.key, it's very likely that people will depend on KeyboardEvent.keyCode.
(In reply to Masatoshi Kimura [:emk] from comment #16)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #15)
> > The incompatibility is about trusted event. Not for untrusted (synthetic)
> > events.
> 
> Given that Chrome doesn't support .keyCode/.charCode/.which, we will
> introduce incompatibility if we support them.

I don't think so. The constructor spec is still unstable because IE hasn't implemented it yet.

> > > Our .which is defined in UIEvent (not KeyboardEvent), but UIEventInit can't
> > > initialize legacy extensions.
> > > https://mxr.mozilla.org/mozilla-central/source/dom/webidl/UIEvent.webidl
> > 
> > Indeed.
> 
> Then why we should be inconsistent about KeyboardEvent?
> 
> Anyway, we should finish KeyboardEvent.key before this bug. If we added
> KeyboardEventInit.keyCode without completing KeyboardEvent.key, it's very
> likely that people will depend on KeyboardEvent.keyCode.

Why? We refuses untrusted key events from default action handlers (perhaps, there are some handlers remaining, though). So, synthetic events are used only web apps internally.

Random thought:

If .keyCode/.charCode/.which are not initialized from KeyboardEvent constructor, web apps which handle the legacy attributes need to initialize them with initKey(board)Event which are different on each browser. So, ignoring them from initialization list might make creating automated tests for such web apps harder.

If they are not initialized from KeyboardEvent constructor, web apps also needs to call initKey(board)Event() after that. It sounds the code is redundant.

Is it right behavior to keep the other attributes which are not initialized by initKey(board)Event() after initKey(board)Event() is called?
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #8)
> Smaug:
> 
> Do you think we should also support initKeyboardEvent() which is defined in
> D3E?
> https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#idl-
> interface-KeyboardEvent-initializers

We should support whatever version lets us initialize at least all the commonly user properties.
Which certainly includes kyeCode and charCode.
Flags: needinfo?(bugs)
With initializing KeyboardEvent::InitKeyEvent(), keyCode, charCode and which are computed as trusted event with its type value. E.g., even if setting charCode as non-zero for "keydown", the charCode of the initialized instance returns 0. I think that there is no reason to change the behavior of legacy method for compatibility with our current and older builds.

However, this behavior isn't good for event ctor. Ctor should be set each attribute value independently (I believe so). Therefore, this patch creates new patch in KeyCode(), CharCode() and Which(). They return set value directly.

FYI: The legacy attributes, keyCode, charCode and which, are now defined by D3E as a part of constructor.
https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#KeyboardEvent-supplemental-interface
Attachment #8405031 - Flags: review?(bugs)
This patch implements KeyboardEvent.initKeyboardEvent() which is defined in D3E.

The arguments are: type, canBubble, cancelable, view, detail, key, location, modifiersList, repeat.

IE supports |.locale| and it takes a value for it after the argument for repeat.
http://msdn.microsoft.com/en-us/library/ie/ff975297%28v=vs.85%29.aspx

I.e., IE's initKeyboardEvent() takes one more argument. Fortunately, as I wrote in the automated tests, adding the locale argument at calling doesn't cause any problems. Therefore, this patch's initKeyboardEvent() works fine with the code written for IE.

FYI #1: initKeyboardEvent() cannot set keyCode, charCode and which. Therefore, there is no compatibility problem with IE.

FYI #2: WebKit/Blink has initKeyboardEvent() too but its are arguments are not same as current draft. Perhaps, they are defined in old D3E draft.
http://trac.webkit.org/browser/trunk/Source/WebCore/dom/KeyboardEvent.idl#L59
Attachment #8405039 - Flags: review?(bugs)
Attachment #8405031 - Flags: review?(bugs) → review+
Comment on attachment 8405039 [details] [diff] [review]
part.2 Implement KeyboardEvent.initKeyboardEvent()

tiny bit risky, if some code path expects Gecko to have similar
method to webkit/blink.
Attachment #8405039 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 8405039 [details] [diff] [review]
> part.2 Implement KeyboardEvent.initKeyboardEvent()
> 
> tiny bit risky, if some code path expects Gecko to have similar
> method to webkit/blink.

Yeah. It might be safer if we never support initKeyboardEvent, though. But anyway, I have no idea to use synthesized key event on usual web applications since we don't allow it to change editor content.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7c4c0d0f613
https://hg.mozilla.org/integration/mozilla-inbound/rev/220e9d8b1454

Or, if we could define overload method by webidl, we could support WebKit/Blink's initKeyboardEvent(), though.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #23)
> Or, if we could define overload method by webidl, we could support
> WebKit/Blink's initKeyboardEvent(), though.

We could.
Hmm, looks like that WebKit/Blink's initKeyboardEvent() initializes non-standard attribute "keyIdentifier" which must be a prototype of "key" attribute. So, even if our webidl supports overload method, we need to implement it... I think that it's not worthwhile to do it since all internal key handlers which supports untrusted events shouldn't check the non-standard attribute only for the compatibility of this...
https://hg.mozilla.org/mozilla-central/rev/c7c4c0d0f613
https://hg.mozilla.org/mozilla-central/rev/220e9d8b1454
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Summary: Implement DOM KeyboardEvent constructor of UI Events → Implement D3E KeyboardEvent constructor
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: