Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require support or non-support for legacy init*Event #112

Open
zcorpan opened this issue Nov 1, 2016 · 19 comments
Open

Require support or non-support for legacy init*Event #112

zcorpan opened this issue Nov 1, 2016 · 19 comments
Labels

Comments

@zcorpan
Copy link
Member

zcorpan commented Nov 1, 2016

See web-platform-tests/wpt#4117

I would like the spec to say which methods must be supported, and which must not be supported. Saying that they are optional doesn't help with reaching interop.

It seems Gecko does not support initKeyboardEvent, but WebKit/Chromium/Edge 13 do.

http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4630

Should Gecko add it, or should the other browsers drop it?

@ayg
Copy link
Contributor

ayg commented Nov 1, 2016

Gecko tried to add it, but it caused compatibility issues, so it dropped support again:

https://bugzilla.mozilla.org/show_bug.cgi?id=999645

tldr -- before Firefox added the event, everything was fine because sites sniffed other ways of doing things (presumably initKeyEvent). Then when Firefox added support for initKeyboardEvent, it made all arguments mandatory, which was consistent with the spec and IE, but not Chrome. Then something broke because it tried to use initKeyboardEvent and omitted some arguments. The fix was to remove initKeyboardEvent again.

What should we do in these cases where there were two legacy ways of doing something?

  1. Require implementations to support both?
  2. Pick one as the standard one and leave the other as de facto optional?
  3. Pick one as standard and prohibit supporting the other?
  4. Specify both but say implementations need only support one of the two?

It's not actually clear to me. I think we usually wind up doing (2), but that leaves browsers not actually interoperable (they're potentially following different code paths). (3) is nice in theory but not necessarily practical, because it will break pages in the browser that drops support for its way of doing things. (1) requires the most work from specifiers and implementers. (4) requires the least work from implementers, but also doesn't necessarily result in real interop for legacy pages.

I think we're agreed that "5. Pretend legacy pages don't exist and don't properly spec behavior for them" is not a great option, though.

@zcorpan
Copy link
Member Author

zcorpan commented Nov 1, 2016

Thanks @ayg.

@RByers @foolip do you think initKeyboardEvent could be dropped from Chromium? I see there's a use counter at https://www.chromestatus.com/metrics/feature/timeline/popularity/868 (has it reached stable?)

If we find that non-Gecko browsers can't remove it for Web compat, we have the option to describe state this in the spec world using https://html.spec.whatwg.org/multipage/webappapis.html#concept-navigator-compatibility-mode (but in my opinion this option should not be used unless it is demonstrated that neither side is able to change).

@Ms2ger
Copy link

Ms2ger commented Nov 1, 2016

Perhaps a simpler approach: require support for initKeyboardEvent, with optional arguments.

@foolip
Copy link
Member

foolip commented Nov 1, 2016

Yeah. The init*Event methods in Blink have all optional arguments for some historical reason. In general, I think it'd make sense if arguments were made optional where their corresponding init dict member is not required, which for the most part would mean that init*Event methods have 1 required argument.

@RByers
Copy link

RByers commented Nov 1, 2016

Yes, the use counter has been in stable for awhile. The usage is frighteningly high, so we'd need to do some more serious analysis to try to prove that sites do generally have good fallbacks rather than, for example, relying on UA sniffing to decide which code path to take. Whether that's worth doing for blink is probably @dtapuska's call. Certainly the fact that Firefox is getting along more-or-less fine without initKeyboardEvent suggests we may have an opportunity here (which we'd likely loose if Gecko adds initKeyboardEvent).

But I suspect there's a decent change we'd find non-trivial breakage and so would prefer to just standardize what the web appears to rely on today. Is / how long has the KeyboardEvent constructor been interoperable between latest shipped Safari, Edge, Firefox and Chrome?

@ayg
Copy link
Contributor

ayg commented Nov 1, 2016

The only reason Firefox gets along without it is because it has initKeyEvent. I doubt it would be web-compatible to have neither, so the spec should require one or the other. The question is how best to do that, particularly given that it might be risky for Firefox to drop initKeyEvent.

@bzbarsky What do you think?

@bzbarsky
Copy link

bzbarsky commented Nov 1, 2016

For event stuff you want @smaug----

@dtapuska
Copy link
Contributor

dtapuska commented Nov 1, 2016

I was hoping that we could remove initKeyboardEvent entirely. Mostly what I was seeing people is broken in chrome today anyways. They were typically dispatching keyboard events from javascript to do something. Which isn't allowed due to trusted event support. Secondly initKeyboardEvent doesn't support key or code events so you can't initialize anything useful on them these days with Chrome dropping support for keyIdentifier.

@smaug----
Copy link

I think I agree with @Ms2ger here. That sounds like the most likely to be web compatible.

I don't really care if initKeyboardEvent can't be used for anything useful. It is a legacy thing, but used often so can't be removed, and shouldn't be changed too much. Making param optional should be still fine.

@foolip
Copy link
Member

foolip commented Nov 2, 2016

Concretely, this is what Blink would look like if making only the first argument required:

void initKeyboardEvent(DOMString type,
                       optional boolean bubbles = false,
                       optional boolean cancelable = false,
                       optional Window? view = null,
                       optional DOMString keyIdentifier = "",
                       optional unsigned long location = 0,
                       optional boolean ctrlKey = false,
                       optional boolean altKey = false,
                       optional boolean shiftKey = false,
                       optional boolean metaKey = false);

Would that work for Gecko?

@garykac
Copy link
Member

garykac commented Jan 25, 2017

Is most of the initKeyboardEvent usage in libraries? If it's only a few places generating most of the usage, then we can have them switch to "new KeyboardEvent" and then deprecate.

When I've attempted to use initKeyboardEvent in Chrome, it didn't work as I expected. I couldn't set keyCode or charCode and I needed to fall back to using "new Event" and setting the fields manually.

I'd prefer to deprecate initKeyboardEvent if possible, but if we can agree on un-deprecating then I'm fine with that.

@foolip
Copy link
Member

foolip commented Jan 25, 2017

@garykac, I did a query in https://bigquery.cloud.google.com:443/savedquery/762219082167:7284a4c19ede4b468fc2c5ac9cb137b4 and here is a list of pages with the string "initKeyboardEvent": initKeyboardEvent.txt

To get some idea about the usage, could you take 10 of these at random and see how initKeyboardEvent() is used in each? If it looks promising, it might be worth doing a more comprehensive analysis of the usage. This is unfortunately the only way that I know of to make progress in cases like this where usage is a bit too high for comfort.

@dtapuska
Copy link
Contributor

dtapuska commented Jan 25, 2017 via email

@Ms2ger
Copy link

Ms2ger commented Jan 25, 2017

Do people actually update dojo?

Either case, if dojo is representative, it might be sufficient to require initKeyEvent and forbid initKeyboardEvent.

@foolip
Copy link
Member

foolip commented Jan 25, 2017

The Dojo code is:

has.add("events-keypress-typed", function(){ // keypresses should only occur a printable character is hit
	var testKeyEvent = {charCode: 0};
	try{
		testKeyEvent = document.createEvent("KeyboardEvent");
		(testKeyEvent.initKeyboardEvent || testKeyEvent.initKeyEvent).call(testKeyEvent, "keypress", true, true, null, false, false, false, false, 9, 3);
	}catch(e){}
	return testKeyEvent.charCode == 0 && !has("opera");
});

Given that exceptions are caught, seems like just removing both initKeyboardEvent and initKeyEvent should still result in returning true here.

If Dojo is the main user of initKeyboardEvent, then it may be necessary to do an exhaustive analysis of httparchive to see if it's obscuring other usages that would break in a bad way. If it and initKeyEvent can be removed with little risk that's great, but since we're not getting rid of all init*Event methods it'd also be fine to find some compatible intersection and call it a day.

@annevk
Copy link
Member

annevk commented Mar 14, 2017

Note that we ended up changing DOM to make arguments like this optional. Does that mean Gecko can add the method now?

@ayg
Copy link
Contributor

ayg commented Aug 6, 2017

@foolip So your argument list doesn't actually match Edge or the spec, correct? Do you want to change to match the spec and Edge, or should the spec change to match you?

https://msdn.microsoft.com/en-us/library/ff975297(v=vs.85).aspx
https://w3c.github.io/uievents/#dom-keyboardeveng-initkeyboardevent

@ayg
Copy link
Contributor

ayg commented Aug 6, 2017

https://bugzilla.mozilla.org/show_bug.cgi?id=1387828

Patch submitted. I matched the spec/IE documentation for argument list, not the Chrome version mentioned here in an earlier comment. (However, the locale argument is ignored in my patch.)

@foolip
Copy link
Member

foolip commented Aug 8, 2017

@foolip So your argument list doesn't actually match Edge or the spec, correct? Do you want to change to match the spec and Edge, or should the spec change to match you?

That's right, we have a TODO to that effect. I don't think I've ever looked hard into what the differences are or the likely compat implications of changing it, but obviously I don't mind resolving it as in #153. Getting some input from Edge folks would be good, in case they already know of reasons they're unable to change. @travisleithead?

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 15, 2017
w3c/uievents#112

This is supported by all other UAs.  In the past we had compatibility
problems when trying to add support, but it seems these might be fixed
if we make all arguments optional beyond the first.

The interface chosen for the method is from the spec, which has been
updated to match Chrome.  This is also very similar to WebKit, but the
final four arguments are different from IE.

MozReview-Commit-ID: 36AeX1JwJTt

--HG--
extra : rebase_source : 28b298d370f0f9a5ab4090a71a2aae91f1d90025
freesamael pushed a commit to freesamael/gecko-unified that referenced this issue Aug 15, 2017
w3c/uievents#112

This is supported by all other UAs.  In the past we had compatibility
problems when trying to add support, but it seems these might be fixed
if we make all arguments optional beyond the first.

The interface chosen for the method is from the spec, which has been
updated to match Chrome.  This is also very similar to WebKit, but the
final four arguments are different from IE.

MozReview-Commit-ID: 36AeX1JwJTt
aethanyc pushed a commit to aethanyc/gecko-dev that referenced this issue Aug 15, 2017
w3c/uievents#112

This is supported by all other UAs.  In the past we had compatibility
problems when trying to add support, but it seems these might be fixed
if we make all arguments optional beyond the first.

The interface chosen for the method is from the spec, which has been
updated to match Chrome.  This is also very similar to WebKit, but the
final four arguments are different from IE.

MozReview-Commit-ID: 36AeX1JwJTt
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 1, 2019
w3c/uievents#112

This is supported by all other UAs.  In the past we had compatibility
problems when trying to add support, but it seems these might be fixed
if we make all arguments optional beyond the first.

The interface chosen for the method is from the spec, which has been
updated to match Chrome.  This is also very similar to WebKit, but the
final four arguments are different from IE.

MozReview-Commit-ID: 36AeX1JwJTt

UltraBlame original commit: 8ee32af87bd4ce492f0882184ab558a5a722dcad
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 1, 2019
w3c/uievents#112

This is supported by all other UAs.  In the past we had compatibility
problems when trying to add support, but it seems these might be fixed
if we make all arguments optional beyond the first.

The interface chosen for the method is from the spec, which has been
updated to match Chrome.  This is also very similar to WebKit, but the
final four arguments are different from IE.

MozReview-Commit-ID: 36AeX1JwJTt

UltraBlame original commit: 8ee32af87bd4ce492f0882184ab558a5a722dcad
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 1, 2019
w3c/uievents#112

This is supported by all other UAs.  In the past we had compatibility
problems when trying to add support, but it seems these might be fixed
if we make all arguments optional beyond the first.

The interface chosen for the method is from the spec, which has been
updated to match Chrome.  This is also very similar to WebKit, but the
final four arguments are different from IE.

MozReview-Commit-ID: 36AeX1JwJTt

UltraBlame original commit: 8ee32af87bd4ce492f0882184ab558a5a722dcad
@garykac garykac added the Legacy label Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

10 participants