Closed Bug 1024864 Opened 5 years ago Closed 5 years ago

For renaming some key values of KeyboardEvent.key on 33, we should warn it on the Console

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox32 - ---

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(1 file)

We'll change some key values which have been modified by DOM Level 3 KeyboardEvent Key Values spec in current cycle.

Therefore, it should be notified to web developers whose application access .key attribute of KeyboardEvent.

I.e., this bug just make DOM KeyboardEvent.key will warn the message in the console and it should be uplift to Aurora.
Attached patch PatchSplinter Review
%s isn't available with nsIDocument::WarnOnceAbout() since it's hard to store whether same message has already been used per document.

This adds a lot of warning messages. However, I think that it's OK because this patch will be backed out from Nightly soon after landed on Aurora.
Attachment #8439817 - Flags: review?(bugs)
Attachment #8439817 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/3bb907654de0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
I have two issues with this patch:
* can we please stop hard-coding "/en-US/" in links to MDN?
* why 25 strings instead of one with placeholders? The only things changing are oldKey and newKey, and they're not supposed to be localized.
(In reply to Francesco Lodolo [:flod] from comment #5)
> I have two issues with this patch:
> * can we please stop hard-coding "/en-US/" in links to MDN?

Where's the good reference of them? The document have never been translated to any other languages yet.

> * why 25 strings instead of one with placeholders? The only things changing
> are oldKey and newKey, and they're not supposed to be localized.

See comment 1. It's the limitation of nsIDocument::WarnOnceAbout().
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> (In reply to Francesco Lodolo [:flod] from comment #5)
> > I have two issues with this patch:
> > * can we please stop hard-coding "/en-US/" in links to MDN?
> 
> Where's the good reference of them? The document have never been translated
> to any other languages yet.

The point is: when linking to SUMO or MDN, just drop the en-US part, it's not necessary. 
English users will be directed anyway to en-US, other locales will redirect to the right page if available, en-US if not.


(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> See comment 1. It's the limitation of nsIDocument::WarnOnceAbout().

Too bad, these strings are a nightmare of copy and paste, with a very high risk of making a mess.
(In reply to Francesco Lodolo [:flod] from comment #7)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> > (In reply to Francesco Lodolo [:flod] from comment #5)
> > > I have two issues with this patch:
> > > * can we please stop hard-coding "/en-US/" in links to MDN?
> > 
> > Where's the good reference of them? The document have never been translated
> > to any other languages yet.
> 
> The point is: when linking to SUMO or MDN, just drop the en-US part, it's
> not necessary. 
> English users will be directed anyway to en-US, other locales will redirect
> to the right page if available, en-US if not.

Okay, thanks. However, looks like that other resources also link to MDN written in English.
http://mxr.mozilla.org/mozilla-central/source/dom/locales/en-US/chrome/dom/dom.properties#143

I think that if you believe they are bad manner, all of them should be fixed at one time.

> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> > See comment 1. It's the limitation of nsIDocument::WarnOnceAbout().
> 
> Too bad, these strings are a nightmare of copy and paste, with a very high
> risk of making a mess.

Agreed. However, fortunately, the patch will be backed out from m-c after landing the patch onto Aurora and landing renaming patches onto m-c.
I try to file bugs when I see them (see a similar example, bug 896256, for devtools).

With this patch we move from having 3 of them (1 with a pending bug), to 28.
http://transvision.mozfr.org/?recherche=%2Fen-US%2F&repo=central&sourcelocale=en-US&locale=it&search_type=strings
Comment on attachment 8439817 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not a regression.
User impact if declined: This patch warns that KeyboardEvent.key value will be changed for conforming the latest DOM Level 3 KeyboardEvent Key value spec (Some our key values were renamed in the D3E WG). If this patch isn't uplift to Aurora, we need to wait for renaming them about 5 weeks or rename them without warnings.
Testing completed (on m-c, etc.): Landed on m-c.
Risk to taking this patch (and alternatives if risky): Probably, nothing.
String or IDL/UUID changes made by this patch: 24 warning strings are added to dom.properties. They will be backed out from m-c after uplift is granted.

FYI: See the number of blocking bugs. If this is not granted, we need to discard the patches which were already reviewed.
Attachment #8439817 - Flags: approval-mozilla-aurora?
Having 25 strings like this on l10n-central is painful but acceptable, but that's not really OK to land on mozilla-aurora (string frozen).
(In reply to Francesco Lodolo [:flod] from comment #11)
> Having 25 strings like this on l10n-central is painful but acceptable, but
> that's not really OK to land on mozilla-aurora (string frozen).

If it's not acceptable, I believe that we should just backout the patch from m-c and perform the key value changes without warnings on 32. But smaug perhaps disagree with this. But I strongly don't want to waste a lot of weeks by this.
Hmm, was I wrong... I was pretty sure we have started to warn about deprecated features on Aurora in
some cases.

Masayuki, what is the reason this stuff can't just go with normal cycles, and have one cycle with
warnings and the next one with new .key values?
(In reply to Olli Pettay [:smaug] from comment #13)
> Masayuki, what is the reason this stuff can't just go with normal cycles,
> and have one cycle with
> warnings and the next one with new .key values?

I think that we should land them as far as possible because IE, Chrome and Safari may implement them in the near future. At *developing* them, our implementation may be a good reference. Our implementation is open and enough cross platform, so, for the compatibility between browsers in the future, I believe improving our implementation is very important.

If our some mappings are wrong, it should be fixed as soon as possible. Then, all other browsers must not share such buggy mapping. (If buggy mapping is shared, it becomes to change the mapping more difficult)

Additionally, I have a lot of patches in my queue which must be rejected if the renaming patches are not applied. So, I need to recreate patches which add new key values in the latest draft and remapping some existing keys in this cycle and I'll need to recreate the renaming patches again. That's very annoying...

Anyway, I want to improve .key value improvement at same cycle. This must make web developers happy.
Comment on attachment 8439817 [details] [diff] [review]
Patch

With string changes and not really being a critical issue (may implement in other browsers is not compelling enough) this should ride the trains.
Attachment #8439817 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Okay, thank you, Lukas.

Smaug. I'd like to back it out from m-c. And landing rename patches. How about you?

We've already changed some values without warnings. E.g., at internationalizing keyCode values, at changing some IME key value mapping. If we had to warn every change at least a cycle, we would need long time for modifying .key value mapping or something similar changes in the future. I don't think that it's good developing style. I think that modifying MDN document and Firefox XX for developers are enough for publicity of changes. We can write the document at least 12 weeks before the release.
Flags: needinfo?(bugs)
I doubt devs notice warnings in MDN. Console is better place.

The next uplift will happen in about a month. What all would it delay, if we renamed keys during the
next cycle?

But if you think that 4 weeks delays stuff too much, fine, back this out.
(Though, going through the key names still once to ensure the best possible stability in the spec would
be good.)
Flags: needinfo?(bugs)
Okay, anyway, we've already lost a week. I'll request reviews of remaining patches. If the all bugs blocking bug 900372 will be ready to land and not so late for 33, I'll back this out and land them at same time. Does this sound good to you?
Sounds ok.
You need to log in before you can comment on or make changes to this bug.