1520756 - Put off to ship Window.event, Event.returnValue and setting same keyCode/charCode value to keypress events

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, enhancement, P1)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

[Tracking Requested - why for this release]: Due to bug 1514940.

We need to put of to ship "setting same keyCode/charCode value to keypress events" (implemented by bug 1479964).

However, this blocks Window.event (bug 1479964) and Event.returnValue (bug 1510985) too.

So, we need to land a patch to disable them only on 65 Beta.

Note that "stop dispatching non-printable keypress events" (bug 968056) and "start to dispatch keydown/keyup events even during composition" (bug 354358) should be shipped 65 as planned because these changes cannot be detected with any API. So, some web pass may have already changed their behavior with version number check.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for Beta 65

In bug 1496288, we decided to ship the following new features:

• Windows.event
• Setting keyCode and charCode of "keypress" events to same value
• Stop dispatching non-printable "keypress" events
• Start to dispatch "keydown" and "keyup" events even during composition

However, the 2nd change breaks editor of Confluence. For making the shipment
of 65 safer, we should cancel shipping the 2nd change.

Then, we need to disable Window.event and Event.returnValue too because
they are blocked by the 2nd (bug 1479964 and bug 1478102).

On the other hand, perhaps, we should ship the last 2 changes as planned
because web apps cannot detect these changes from any API and also we've
already introduced these changes. So, some web apps have already started
to check version number of Firefox. We shouldn't break such web apps.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Try pushes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc76b300d51bd8b38fe26d6ad8e34c3b7b2a8259
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3278882d7654b084c81aadac449a631b923d5dce
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed71dd234031d9427985717bfd92420208d5b637

Comment 3

[Ryan VanderMeulen [:RyanVM]]

This is really late in the cycle to be making this call - today's the last scheduled Beta for 65 and the RC is next week.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

IIUC, this is just a rebased version of bug 1510985, correct? What sort of web compat fallout should we expect in that case?

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Particularly around bug 354358 and bug 968056.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #4)

IIUC, this is just a rebased version of bug 1510985, correct?

Unfortunately, not only bug 1510985. The patch additionally backing out part of bug 1496288.

What sort of web compat fallout should we expect in that case?

If some web apps have already been changed for new behavior of 65, and they check version number of us for new behavior even for the disabling features, such web apps will be broken.

Comment 7

[Mike Taylor [:miketaylr]]

Sorry this is so late in the beta cycle. We've learned that if we don't disable this, we're going to break parts of Confluence (including Mana), which is unfortunately a hosted intranet thing. We think we have a plan to fix Bug 1514940 going forward, but it seems better to land this now than to have to chemspill over a known issue next week.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9037171 [details] [diff] [review]
Patch for Beta 65

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1496288

User impact if declined: Users cannot type "." in editor of Confluence (bug 1514940).

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: 1-1. Open https://bug1478102.bmoattachments.org/attachment.cgi?id=9020774
1-2. Type numbers
1-3. Then, numbers should be appear in the editor.

2-1. Open https://w3c.github.io/uievents/tools/key-event-viewer.html
2-2. Type "a"
2-3. Then, keyCode of "keypress" should be 0.

3-1. Open mana
3-2. Type some characters and type "." (at end of the editor)
3-3. Check whether "." is typed.

List of other uplifts needed: None

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): If there are some web apps which have already been changed for Firefox 65, and they check our version number for those features. Then, such apps will be broken (although, those features should be detected with feature detection). But keep shipping the features would break at least Confluence which is used as intranet app like mana.

String changes made/needed: none

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9037171 [details] [diff] [review]
Patch for Beta 65

[Triage Comment]
Sounds like we're between a rock and a hard place on this one. However, the risks of intranet bustage by shipping this feature seem to outweigh the risks of sites having to re-adjust to our previously-stated intent to ship in 65, so let's go with this. Approved for 65.0b12.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6881825b5cd7

Comment 11

[Catalin Sasca, Desktop Test Engineering [:csasca]]

I successfully verified the fixes on Firefox 65.0b12 under Windows 10 (x64), macOS 10.12, and Ubuntu 18.04 (x64) using the STR from Comment 8. I encountered no issues while testing.

Comment 12

[Chris Mills [:cmills]]

I'm setting this to ddc, because we updated the current state of support for these in https://bugzilla.mozilla.org/show_bug.cgi?id=1496288#c26