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

VERIFIED FIXED in Firefox 65

Status

()

enhancement
P1
normal
VERIFIED FIXED
4 months ago
2 months ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({dev-doc-complete})

65 Branch
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 unaffected, firefox65+ verified, firefox66 wontfix)

Details

(Whiteboard: [webcompat:p1])

Attachments

(1 attachment)

[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.

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.

Attachment #9037171 - Flags: review?(bugs)

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.

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

Flags: needinfo?(masayuki)

Particularly around bug 354358 and bug 968056.

(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.

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.

Updated

4 months ago
Attachment #9037171 - Flags: review?(bugs) → review+

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

Flags: needinfo?(masayuki)
Attachment #9037171 - Flags: approval-mozilla-beta?
Whiteboard: [webcompat:p1]
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.
Attachment #9037171 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 10

4 months ago
uplift
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
No longer blocks: 1508200

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+

Updated

4 months ago
Depends on: 1522089

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

Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.