I've emailed again, asking if we might get access to some unminified source code (one can dream...), in the meanwhile, Tom can you try to track down exactly where things are going wrong? To reproduce, go to any random Mana page and try to leave a comment. The period character won't work until you toggle dom.keyboardevent.keypress.set_keycode_and_charcode_to_same_value

Actually setting ni?... see Comment #16.

I checked if preventDefault was being called on any key events in Mana's comment-editor, and sure enough, I found this stack trace:

prevent https://mana.mozilla.org/wiki/s/e324fe67f5f092e7ca80f0eeff5ac2f1-CDN/en_GB/7601/6017a6228c95ac4ab2c23e1488007fb69b3151df/61d5fe8aace5ec5ee3b4dc9add3a1285/_/download/contextbatch/js/editor-v3,editor,macro-browser,-_super,-atl.general,-viewcontent,-main,-page,-atl.comments/batch.js?analytics-enabled=true&analytics-uploadable=true&build-number=7601&confluence.table.resizable=true&highlightactions=true&hostenabled=true&is-server-instance=true&locale=en-GB&nps-acknowledged=true:13921
x https://mana.mozilla.org/wiki/s/e324fe67f5f092e7ca80f0eeff5ac2f1-CDN/en_GB/7601/6017a6228c95ac4ab2c23e1488007fb69b3151df/61d5fe8aace5ec5ee3b4dc9add3a1285/_/download/contextbatch/js/editor-v3,editor,macro-browser,-_super,-atl.general,-viewcontent,-main,-page,-atl.comments/batch.js?analytics-enabled=true&analytics-uploadable=true&build-number=7601&confluence.table.resizable=true&highlightactions=true&hostenabled=true&is-server-instance=true&locale=en-GB&nps-acknowledged=true:26173
dispatch https://mana.mozilla.org/wiki/s/e324fe67f5f092e7ca80f0eeff5ac2f1-CDN/en_GB/7601/6017a6228c95ac4ab2c23e1488007fb69b3151df/61d5fe8aace5ec5ee3b4dc9add3a1285/_/download/contextbatch/js/editor-v3,editor,macro-browser,-_super,-atl.general,-viewcontent,-main,-page,-atl.comments/batch.js?analytics-enabled=true&analytics-uploadable=true&build-number=7601&confluence.table.resizable=true&highlightactions=true&hostenabled=true&is-server-instance=true&locale=en-GB&nps-acknowledged=true:10242
b https://mana.mozilla.org/wiki/s/e324fe67f5f092e7ca80f0eeff5ac2f1-CDN/en_GB/7601/6017a6228c95ac4ab2c23e1488007fb69b3151df/61d5fe8aace5ec5ee3b4dc9add3a1285/_/download/contextbatch/js/editor-v3,editor,macro-browser,-_super,-atl.general,-viewcontent,-main,-page,-atl.comments/batch.js?analytics-enabled=true&analytics-uploadable=true&build-number=7601&confluence.table.resizable=true&highlightactions=true&hostenabled=true&is-server-instance=true&locale=en-GB&nps-acknowledged=true:17473
e https://mana.mozilla.org/wiki/s/e324fe67f5f092e7ca80f0eeff5ac2f1-CDN/en_GB/7601/6017a6228c95ac4ab2c23e1488007fb69b3151df/61d5fe8aace5ec5ee3b4dc9add3a1285/_/download/contextbatch/js/editor-v3,editor,macro-browser,-_super,-atl.general,-viewcontent,-main,-page,-atl.comments/batch.js?analytics-enabled=true&analytics-uploadable=true&build-number=7601&confluence.table.resizable=true&highlightactions=true&hostenabled=true&is-server-instance=true&locale=en-GB&nps-acknowledged=true:13860

The culprit is this function in this script, which according to comment in the minified file is part the module file tinymce3/plugins/confcursortarget/editor_plugin_src.js:

function x(a, b) {
  var c = b.keyCode;
  if (c !== m.BACKSPACE && c !== m.DELETE) return !0;
  a.selection.isCollapsed() ? c === m.BACKSPACE && E(a.selection.getNode(), a.selection.getRng(F)) ? i.dom.Event.prevent(b)  : c === m.DELETE && D(a.selection.getNode(), a.selection.getRng(F)) && i.dom.Event.prevent(b)  :
  n(a.getBody());
  return !0
}

This turns out to be a keydown handler, which is working around old Firefox key behavior differences by canceling all keydowns except backspace and delete, and relying on the same events still happening in the keypress handler. Sure enough, if I add mana.mozilla.org to the about:config options dom.keyboardevent.keypress.hack.dispatch_non_printable_keys and dom.keyboardevent.keypress.hack.use_legacy_keycode_and_charcode, then the handlers get what they expect and things seem to work fine.

Fixing this on the Mana instance just involved having the page not add those Gecko-specific handlers. They are added a bit further down the same script in this chunk of (minifed) code:

, i.isGecko && e.onKeyDown.add(function (a, b) {
      (b.keyCode === m.ENTER || b.keyCode === m.BACKSPACE || b.keyCode === m.DELETE) && a.selection.normalize()
}), e.onKeyDown.add(x), e.onKeyUp.add(x), i.isGecko && e.onKeyPress.add(x), e.onPaste.add(r));

Changing that so these only apply to Gecko versions that change our key behavior should do the trick; something like this:

, (isGecko65down = i.isGecko && parseInt(navigator.userAgent.match(/Firefox\/(\d+)/)[1]) < 66), i.isGecko45down && e.onKeyDown.add(function (a, b) {
      (b.keyCode === m.ENTER || b.keyCode === m.BACKSPACE || b.keyCode === m.DELETE) && a.selection.normalize()
}), e.onKeyDown.add(x), e.onKeyUp.add(x), i.isGecko45down && e.onKeyPress.add(x), e.onPaste.add(r));

This is working fine for me on Mana, though I don't know which specific version of Firefox would be best to sniff for. Confluence and other sites using tinymce3 could likely take this fix, assuming the sniff is good enough (or maybe we now have a better sniff than a version-check?)

Testing tinymce online at http://fiddle.tinymce.com, versions 4 and 5 are working as expected but their tool is refusing to load v3. I wonder if this is an issue for tinymce 3, or just this specific plugin -- when I try the tinymce3 examples locally, they work as expected: http://archive.tinymce.com/download/older.php (at least, 3.5.8).

Tom, can you try to find out what version of tinymce3 in running on Mana? I'll email our contact and ask about getting ahold of an unminified tinymce3/plugins/confcursortarget/editor_plugin_src.js

The web console gives me these values on the Mana page:

tinyMCE
> { majorVersion: "3", minorVersion: "4.9-atlassian-11", releaseDate: "2012-03-06" }

new tinymce.plugins.ConfluenceVersionComment().getInfo()
>{ longname: "Confluence Version Comment", author: "Atlassian", authorurl: "http://www.atlassian.com", version: "3.4.9-atlassian-11" }

Confluence.getBuildNumber()
> "7601"

Here's a link to the non-minified inymce3/plugins/confcursortarget/editor_plugin_src.js (i hope it's OK to share...?):

https://gist.github.com/miketaylr/48b4fa64f0e8ce03701b8308e43e7c17

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(sick) from comment #14)

If there are good information in the DOM tree, we could switch behavior on
old Confluence. But we need to be careful for the performance in other
(most) web apps.

Given the above info (in Comments #20 and #21), maybe we could do something hacky similar to:

try {
  let {author, version} = new tinyMCE.plugins.CursorTargetPlugin().getInfo()
  if (author == "Atlassian" && version == "1.0") {
    applyConfluenceHack()
  } 
} catch(e) {}

What do you think Masayuki?

(forgot ni?)

Not that my proposed fix for Confluence is complicated, but here is how it would look de-minified (according to the link in comment 7):

    var isGecko65down = tinymce.isGecko && parseInt(navigator.userAgent.match(/Firefox\/(\d+)/)[1]) < 66;

    // ATLASSIAN - CONFDEV-5541 - Fix cursor position if enter / return / delete / backspace is pressed
    // on Firefox 65 and earlier.
    if(isGecko65down) {
        ed.onKeyDown.add(function(ed, e) {
            if ((e.keyCode === KEY_CODE.ENTER || e.keyCode === KEY_CODE.BACKSPACE || e.keyCode === KEY_CODE.DELETE)) {
                // Enter may be broken if the cursor is in the wrong position - fix first
                ed.selection.normalize();
            }
        });
    }

    [omitting unrelated code]

    // Fx65 and earlier will fire all three key events for backspace and delete. If you register this on
    // IE and Chrome then you get CONFDEV-4062 which is fun.
    isGecko65down && ed.onKeyPress.add(deleteAndBackspaceKeyHandling);

(In reply to Mike Taylor [:miketaylr] from comment #22)

(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(sick) from comment #14)

If there are good information in the DOM tree, we could switch behavior on
old Confluence. But we need to be careful for the performance in other
(most) web apps.

Given the above info (in Comments #20 and #21), maybe we could do something hacky similar to:

try {
  let {author, version} = new tinyMCE.plugins.CursorTargetPlugin().getInfo()
  if (author == "Atlassian" && version == "1.0") {
    applyConfluenceHack()
  } 
} catch(e) {}

What do you think Masayuki?

I don't think that we can get tinyMCE object from C++ code without complicated code. So, I don't think that we can switch our behavior only on Confluence... Any ideas, smaug?

Yeah, looking at the particular JS file and some info there gets at least slow, if not complicated.
I don't yet have good ideas.

If we can figure out how to detect this thing, we would only have to do for editable documents, which makes it a bit less expensive. I wonder if our system addon could detect the JS, send a message to Gecko which would flip the pref? We would have to invent some kind of Web Extensions experiment API to do that, I guess.

I have a suggestion for how to apply the Confluence hack without incurring a ton of performance penalty.

Looking at the Confluence setup on https://mana.mozilla.org, I see that the contenteditable <body> element is inside an iframe embedded in the main page. There, the tinyMCE object is being embedded inside the parent page, for example it is accessed from the onload handler like |window.parent.tinyMCE.get('wysiwygTextarea').onLoad.dispatch();|. Let's assume that in order to apply the confluence hack with code similar to what we have in comment 22, we would need to run such code both inside the frame where the editable content lives, and its parent frame if one exists.

Based on that, my suggestion is to add the following logic to nsHTMLDocument::EditingStateChanged() around https://searchfox.org/mozilla-central/rev/c21d6620d384dfb13ede6054015da05a6353b899/dom/html/nsHTMLDocument.cpp#2436:

• If it's the first time we're moving away from mEditingState being eOff, dispatch a task to the main thread to raise a CustomEvent called MaybeApplyConfluenceHack on both our document and the parent document.
• In https://searchfox.org/mozilla-central/rev/c21d6620d384dfb13ede6054015da05a6353b899/browser/components/nsBrowserGlue.js#16, register a new actor called ConfluenceHackActor with allFrames: true to handle that event.
• Create a JSM file for it, and inside the JSM code provide a handleEvent() function that responds to "MaybeApplyConfluenceHack", does the author and version check in comment 22 and calls applyConfluenceHack() if needed.
• Ensure that we have some telemetry which captures how many times we are performing the author/version check against how many times applyConfluenceHack() is being called so that we can use it to determine when to remove this hack.

This shouldn't be too bad performance-wise, since nsHTMLDocument::EditingStateChanged() is already quite expensive and making it a little bit more expensive by adding this delayed cost should hopefully not be prohibitive in order to give us a working solution.

Olli, Masayuki, do you think this setup would be a suitable workaround?

As for the actual site bug I think it's actually here:

tinymce.isGecko && ed.onKeyPress.add(deleteAndBackspaceKeyHandling);

non-Gecko (and Gecko...) gets this handler bound to keydown and keyup:

https://gist.github.com/miketaylr/48b4fa64f0e8ce03701b8308e43e7c17#file-atlassian-plugin-thing-js-L658-L659

// Webkit will fire keyDown and keyUp for backspace and delete (even if one is suppressed).
ed.onKeyDown.add(deleteAndBackspaceKeyHandling);
ed.onKeyUp.add(deleteAndBackspaceKeyHandling);

And Gecko gets it for keypress as well.

function deleteAndBackspaceKeyHandling(ed, e) {
    var keyCode = e.keyCode;
    if (keyCode !== KEY_CODE.BACKSPACE && keyCode !== KEY_CODE.DELETE) {
        return true;
    }

    if (!ed.selection.isCollapsed()) {
        addCursorTargetParagraphsToContent(ed.getBody());
    } else if (keyCode === KEY_CODE.BACKSPACE && shouldCancelBackspace(ed.selection.getNode(), ed.selection.getRng(W3C_RANGE))) {
        tinymce.dom.Event.prevent(e);
    } else if (keyCode === KEY_CODE.DELETE && shouldCancelDelete(ed.selection.getNode(), ed.selection.getRng(W3C_RANGE))) {
        tinymce.dom.Event.prevent(e);
    }

    return true;
}

And earlier, var KEY_CODE = $.ui.keyCode;.

KEY_CODE.BACKSPACE is 46. 46 is the value for PERIOD in keypress, but BACKSPACE in keydown/keyup. So it thinks its seeing a backspace and prevents the default action.

So yeah, the actual fix here for Confluence would be to remove that extra duplicate keypress handler, but only once we can ship this (like Tom points out in Comment #24).

Ehsan:

Thanks for your suggestion! Sounds like that it's a reasonable approach.

The keyCode changes were reverted for Fx65 via bug 1520756. We should definitely try to verify that b12 behaves better.

This issue was covered while verifying bug 1520756, everything works as expected on Fx 65.0b12.

Based on Comment #29, let's move this to DOM to see if we can't fix this there.

Trying to create the hack suggested by Ehsan.

Oddly, the new JSM file isn't loaded in the tab (i.e., cannot see it in debugger of Browser Content Toolbox). I don't understand why it's not loaded like ClickHandleChild.jsm...

Any ideas?

Based on my understanding of this code (which is incomplete) these actors respond to CustomEvents. Any reason why you aren't using one?

(In reply to :Ehsan Akhgari from comment #36)

Based on my understanding of this code (which is incomplete) these actors respond to CustomEvents. Any reason why you aren't using one?

nsHTMLDocument dispatches a custom event for the actor with targeting the document node itself. I.e., aEvent.target.setKeyPressEventModel(model); is the response.

On the other hand, it seems that the actor's handleEvent() never called. Additionally, looks like that the actor is not loaded into the content since I don't see the file name in debugger of Browser Content Toolbox. I tried to write the actor with looking ClickHandlerChild implementation. I see it in the debugger, but I don't see my new actor in the same list (nor everywhere).

I'd like to know why I failed registering the actor...

I meant CustomEvent as in https://searchfox.org/mozilla-central/source/dom/events/CustomEvent.cpp...

(In reply to :Ehsan Akhgari from comment #38)

I meant CustomEvent as in https://searchfox.org/mozilla-central/source/dom/events/CustomEvent.cpp...

Oh, I was thinking that eUnidentifiedEvent is mapped to dom::CustomEvent, but looks like not so...
https://searchfox.org/mozilla-central/source/dom/events/EventNameList.h

In this case, I don't need any special attribute which the actor wants to receive. So, I think that dom::Event is enough. But if it causes some problems, I'll try to use it instead.

I have a question, you said

If it's the first time we're moving away from mEditingState being eOff, dispatch a task to the main thread to raise a CustomEvent called MaybeApplyConfluenceHack on both our document and the parent document.

Why do we need to dispatch the event even on the parent document? In my understanding, the editor is in an <iframe> element and we need to use the old behavior only in it.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #39)

If it's the first time we're moving away from mEditingState being eOff, dispatch a task to the main thread to raise a CustomEvent called MaybeApplyConfluenceHack on both our document and the parent document.

Why do we need to dispatch the event even on the parent document? In my understanding, the editor is in an <iframe> element and we need to use the old behavior only in it.

See the second paragraph of comment 28. In mana.mozilla.org, the TinyMCE object lives in the parent frame, so that's why I suggested dispatching the event on the parent document. But I wasn't 100% sure whether this setup is exactly the same way in all Confluence installations. Perhaps there are cases where the editable element is in the same document as the TinyMCE object, which is why I suggested dispatching another event in the current document just to be safe. There is a chance that the second event handler won't find a TinyMCE object and won't do anything useful...

(In reply to :Ehsan Akhgari from comment #40)

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #39)

If it's the first time we're moving away from mEditingState being eOff, dispatch a task to the main thread to raise a CustomEvent called MaybeApplyConfluenceHack on both our document and the parent document.

Why do we need to dispatch the event even on the parent document? In my understanding, the editor is in an <iframe> element and we need to use the old behavior only in it.

See the second paragraph of comment 28. In mana.mozilla.org, the TinyMCE object lives in the parent frame, so that's why I suggested dispatching the event on the parent document.

Thanks! That means that we can dispatch only one event. Then, the actor can check both the document and its parent document.

I don't know the reason though, I see the actor in Browser Content Toolbox now.

However, it seems that actor cannot access properties which are set by content script directly. I.e., aWindow.tinyMCE is always undefined. I guess that I need to use something special API to access actual window which is referred by content script. Does somebody know what can I use?

Okay, I found XPCNativeWrapper.unwrap() and it works as expected for me.

Sigh... The event listener is detected as "leak" in some tests...

It seems that we don't need allFrames: true.

Although, today's tree is really annoying to me, I cannot run mochitest locally :-(

I'll request data collection review after getting r+ for the patches.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #41)

(In reply to :Ehsan Akhgari from comment #40)

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #39)

If it's the first time we're moving away from mEditingState being eOff, dispatch a task to the main thread to raise a CustomEvent called MaybeApplyConfluenceHack on both our document and the parent document.

Why do we need to dispatch the event even on the parent document? In my understanding, the editor is in an <iframe> element and we need to use the old behavior only in it.

See the second paragraph of comment 28. In mana.mozilla.org, the TinyMCE object lives in the parent frame, so that's why I suggested dispatching the event on the parent document.

Thanks! That means that we can dispatch only one event. Then, the actor can check both the document and its parent document.

No, I suggested two events intentionally to ensure your event/actor setup will be Fission-compatible. In the post-Fission world, you cannot assume your document and its parent will be in the same process any more. :-)

(In reply to :Ehsan Akhgari from comment #50)

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #41)

(In reply to :Ehsan Akhgari from comment #40)

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #39)

If it's the first time we're moving away from mEditingState being eOff, dispatch a task to the main thread to raise a CustomEvent called MaybeApplyConfluenceHack on both our document and the parent document.

Why do we need to dispatch the event even on the parent document? In my understanding, the editor is in an <iframe> element and we need to use the old behavior only in it.

See the second paragraph of comment 28. In mana.mozilla.org, the TinyMCE object lives in the parent frame, so that's why I suggested dispatching the event on the parent document.

Thanks! That means that we can dispatch only one event. Then, the actor can check both the document and its parent document.

No, I suggested two events intentionally to ensure your event/actor setup will be Fission-compatible. In the post-Fission world, you cannot assume your document and its parent will be in the same process any more. :-)

Then, I think that it's safe to use only one event because the src attribute of <iframe> is javascript URI and the load event of the sub-document's <body> element accesses window.parent.tinyMCE. I.e., if we'd put process boundary here, Confluence won't work anyway.

Old Confluence does not aware of conflated model keypress event (see UI Events
spec, https://w3c.github.io/uievents/#determine-keypress-keyCode).
Additionally, Confluence can be hosted with any domains. Therefore, we cannot
use blacklist to disable the conflated model keypress event only on it.

This patch checks whether current or parent document is Confluence with JS
module, called KeyPressEventModelCheckerChild. For kicking this module,
nsHTMLDocument dispatches an custom event, CheckKeyPressEventModel, when it
becomes editable only first time. Finally, if it's a Confluence instance, the
module let PresShell know that we need to use split model keypress event in it.

We need to collect how many Confluence instances are loaded and how percentage
of that is old versions which require split keypress event model.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #51)

(In reply to :Ehsan Akhgari from comment #50)

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #41)

(In reply to :Ehsan Akhgari from comment #40)

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #39)

If it's the first time we're moving away from mEditingState being eOff, dispatch a task to the main thread to raise a CustomEvent called MaybeApplyConfluenceHack on both our document and the parent document.

Why do we need to dispatch the event even on the parent document? In my understanding, the editor is in an <iframe> element and we need to use the old behavior only in it.

See the second paragraph of comment 28. In mana.mozilla.org, the TinyMCE object lives in the parent frame, so that's why I suggested dispatching the event on the parent document.

Thanks! That means that we can dispatch only one event. Then, the actor can check both the document and its parent document.

No, I suggested two events intentionally to ensure your event/actor setup will be Fission-compatible. In the post-Fission world, you cannot assume your document and its parent will be in the same process any more. :-)

Then, I think that it's safe to use only one event because the src attribute of <iframe> is javascript URI and the load event of the sub-document's <body> element accesses window.parent.tinyMCE. I.e., if we'd put process boundary here, Confluence won't work anyway.

Hah, you are right, great point! :-)

Do you mind writing a test case for this too, with some JS which creates a fake tinyMCE object which mimics the plugins.CursorTargetPlugin().getInfo() API to verify that we successfully can modify the keypress event model when we find such an object?

Well, if there is, it's better, however, I don't know how to wait the new event which is posted into the queue without its event listener. The event is always stopped the propagation in the message manager first to save the runtime propagation cost. Therefore, synthesizeKey() will run before the event. Do you know how to flush the event queue simply?

In order to spin the event loop you can do something like this for example: https://searchfox.org/mozilla-central/rev/c07aaf12f13037c1f5a343d31f8291549e57373f/toolkit/content/widgets/browser-custom-element.js#1825. This code is waiting for a specific event and is setting up a timeout of 1 second to detect the case when the event doesn't happen in the expected time frame.

For your use case you may not have a specific event you're waiting for, but if you pass false to processNextEvent it will only look at what's currently in the event queue and will not wait for new events to arrive, so perhaps that would achieve what you need?

(In reply to :Ehsan Akhgari from comment #58)

For your use case you may not have a specific event you're waiting for, but if you pass false to processNextEvent it will only look at what's currently in the event queue and will not wait for new events to arrive, so perhaps that would achieve what you need?

Unfortunately, and oddly, it does not work.

I tried to just call processNextEvent(false), like this, with/without Promse. However, according to the Browser Content Toolbox's debugger, actor runs after SimpleTest.finish() is called...

Should I give up stop the propagation for test? Or should I give up the test for runtime cost?

Ah, or, if we could post an event into the queue from mochitest, it should be dispatched after the event. Then, we could guarantee to run the actor.

Oh, looks like that AppConstants.DEBUG is useful in the actor.

Okay, let's allow to propagate the event only in debug builds for the automated test.

Only Part 2 (attachment 9039713 [details]) adds telemetry probe.

Category 3, Web Activity.

Hmm...

Result: datareview- for being Category 3 and defaulting to collecting on all channels.

If we couldn't collect most users' data, we might unship the hack even when it'd still be important...

updated the data review form. and now the new patch uses Scalar instead of Histogram.

hsinyi:

Do you think only the fix (i.e., without Telemetry probe) should be landed first? This bug may have made some testers leave.

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #69)

hsinyi:

Do you think only the fix (i.e., without Telemetry probe) should be landed first? This bug may have made some testers leave.
Hi Masayuki,
I think landing the fix first is the good call here with ensuring that we still need telemetry follow-up.
Thanks for the great efforts!!!

Hmm, why is this considered category 3 data? Based on my understanding of the criteria described in https://wiki.mozilla.org/Firefox/Data_Collection, category 3 data is data that can reveal specific or categories of pages visited by users. In this case, the data can reveal the software that the web server has been running, but it cannot reveal where the user has encountered the web server, or anything specific to the browsing of the user. To me it sounds more like category 2 data, matching questions like "Has the user encountered an old Confluence installation in this session?", "Has the user pressed a specific key combination in this session?", "Has the user clicked more than 10 times in this session?", etc.

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1496288

User impact if declined

Cannot type "." at end of Confluence editor.

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

No

Needs manual test from QE?

Yes

If yes, steps to reproduce

1. Load a Confluence editor (e.g., mana of Mozilla)
2. Type something into the editor.
3. Type "."

Then, "." should be inputted.

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

Because this disables new behavior (bug 1479964) only when we detect the editor parent is old Confluence.

String changes made/needed

None.

https://hg.mozilla.org/mozilla-central/rev/25625eaa999d

Oddly, a lot of tests are orange in tryserver... But it looks like not a regression of the patch since even without the patch, I see same oranges. https://treeherder.mozilla.org/#/jobs?repo=try&revision=78fff291ff1818bcc4f4c9e7c0eefa144806cd1e

Aryx, what do you think? Should we back this out?

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #78)

Aryx, what do you think? Should we back this out?

If this is based on comment 77, I meant that I tested the patch with Beta branch on tryserver. But oddly, even without the patch, I see a lot of oranges and I cannot confirm the result in Beta.

Those failures on Try are due to a too old base revision. Some certificates expires around 2019-02-04 00:00 UTC and cause e.g. https://www.example.com to not load in tests. This has been fixed on central, beta and release. Bug 1525191 for more details.

I managed to reproduce the issue using an older version of Nightly from 2018-12-17 on Windows 10 x64.
I retested everything on latest Nightly 67.0a1 on macOS 10.12, Ubuntu 18.04 x64 and Windows 10 x64. The bug is not reproducing anymore.

I don't think this will help at all at this point, but I thought I would mention it. On beta 66.0b6 I noticed that if you press the "Backspace" button a few times then you can write ".", but as soon as the field is out of focus and then in focus again, the bug is reproducing. Also, even if "." is not present, the undo button is activated.

:lizzard , there are some conflicts, could you please take a look?

grafting 524548:25625eaa999d "Bug 1514940 - part 1: Forcibly disable new keyCode/charCode value of keypress events if the document is Confluence r=smaug,Ehsan,kmag"
merging dom/webidl/HTMLDocument.webidl
merging layout/base/PresShell.cpp
merging layout/base/PresShell.h
merging layout/base/nsIPresShell.h
merging toolkit/actors/moz.build
merging toolkit/modules/ActorManagerParent.jsm
merging xpcom/ds/StaticAtoms.py
warning: conflicts while merging layout/base/PresShell.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging toolkit/actors/moz.build! (edit, then use 'hg resolve --mark')

There's a rebased for Beta Part 1 patch attached to the bug.

https://hg.mozilla.org/releases/mozilla-beta/rev/59b0981f244c

Thank you Ryan, I`ve imported that patch

responding to chutten's needinfo...As long as we are recording that the user visited A Confluence editing page and not collecting data on what confluence editing page it is, this seems to fall in to a gray area between category 2 and category 3, one that creates little or no privacy risk. Given that, default on seems acceptable to me so long as the probe expires. If I understand the use for the data correctly, the permanent collection strikes me as excessive.

Thank you, Merwin. I think that the current patch is enough safe for the telemetry rules.

https://hg.mozilla.org/mozilla-central/rev/92d9228ac82d