Closed Bug 1514940 Opened 5 years ago Closed 5 years ago

Can't type a period in Confluence comment editor

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Version:
65 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 + disabled
firefox66 + fixed
firefox67 --- verified

People

(Reporter: kbrack, Assigned: masayuki)

References

Details

(Keywords: regression, Whiteboard: [webcompat:p1])

Attachments

(4 files, 4 obsolete files)

Bug 1514940 - part 1: Forcibly disable new keyCode/charCode value of keypress events if the document is Confluence
47 bytes, text/x-phabricator-request
Details | Review
Bug 1514940 - part 2: Add telemetry probe to decide when we can remove the hack for Confluence data-review=liuche,
47 bytes, text/x-phabricator-request
Details | Review
Part 1 for Beta
21.66 KB, patch
lizzard
: approval-mozilla-beta+
Details | Diff | Splinter Review
bug1514940.txt
1.81 KB, text/plain
liuche
: data-review+
Details 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

Open a page in confluence on either Firefox 65b2 or Nightly. Attempt to write a comment at the bottom, or create an inline comment. Try to type a '.'.


Actual results:

Nothing. No text is added to the text box.


Expected results:

A '.' should have appeared.
Every other character I tested works correctly. I would assume this is a confluence bug, but it works as expected in stable Firefox (64) and in chrome.
Flags: needinfo?(masayuki)
Hmm, how can I test it? Yes, I know there is a trial demo for 7 days. But that must not be enough for me to investigate, fix and verify...

Could you check if disabling one of the following prefs in about:config fixes the bug?
- dom.keyboardevent.keypress.set_keycode_and_charcode_to_same_value
- dom.keyboardevent.keypress.dispatch_non_printable_keys_only_system_group_in_content
- dom.keyboardevent.dispatch_during_composition
- dom.window.event.enabled

If it does not fix this bug, could you investigate which changeset caused this regression with mozregresson?
https://mozilla.github.io/mozregression/install.html
Flags: needinfo?(masayuki) → needinfo?(kbrack)
Thanks for the quick reply! Disabling dom.keyboardevent.keypress.set_keycode_and_charcode_to_same_value does in fact fix the issue.
Flags: needinfo?(kbrack)
With that clue, it seems like this might be a duplicate of https://bugzilla.mozilla.org/show_bug.cgi?id=1497518 . Feel free to close if that's the case.
No, we need to contact Atlassian.

Mike, do we have connection with them?
Blocks: 1479964
Component: Untriaged → Desktop
Flags: needinfo?(miket)
Keywords: regressionwindow-wanted
OS: Unspecified → All
Product: Firefox → Tech Evangelism
Hardware: Unspecified → All
Version: 65 Branch → Firefox 65
Before the fix of bug 1479964, "." key's keypress event is { keyCode: 0, charCode: 0x2E }. However, currently it's changed to { keyCode: 0x2E, charCode: 0x2E }. keyCode 0x2E means "Delete" key if it's "keydown" or "keyup" event. I guess that there is Gecko only path and/or keydown/keypress events are handled by same function(s) and they check keyCode value without checking event type.
(Adam and I are trying to get a good contact that works on Confluence, leaving ni?)
Flags: needinfo?(miket)
Priority: -- → P1
Harald, do you have a contact who works on Confluence?
Flags: needinfo?(hkirschner)
Status: UNCONFIRMED → NEW
status-firefox-esr60: --- → unaffected
tracking-firefox65: --- → +
tracking-firefox66: --- → +
Ever confirmed: true
If we get this fixed upstream, will that fix propagate to any self-hosted Confluence instances (is that a thing?)? If not, is there a way to do something Confluence-specific here (since I imagine doing it URL-based with a pref will be hard for any self-hosted or intranet-type instances)?
Flags: needinfo?(miket)
Flags: needinfo?(masayuki)
Reached out, with Mike cc'd.
Flags: needinfo?(hkirschner)
(In reply to Andrew Overholt [:overholt] from comment #8)
> If we get this fixed upstream, will that fix propagate to any self-hosted
> Confluence instances (is that a thing?)? If not, is there a way to do
> something Confluence-specific here (since I imagine doing it URL-based with
> a pref will be hard for any self-hosted or intranet-type instances)?

This is a good question. Harald's contact (a dev on Confluence) was unable to reproduce the issue as reported in Confluence Server (self hosted, version 6.14.0-m66) and Confluence Cloud. So that's at least good. I'm following up to ask if there's ways we can detect older versions.
Flags: needinfo?(miket)
Hi kbrack, could you run the following in the browser console of the affected Confluence instance and let us know what it returns? Thanks:

AJS.Meta.get("version-number”)
Flags: needinfo?(kbrack)
Sure thing:

>> AJS.Meta.get('version-number')
"1000.0.0-09b635b7104"
Flags: needinfo?(kbrack)
Awesome, thanks. I'll pass that along. According to our contact:

> Version numbers starting with “1000.0” are Cloud. Version numbers starting with anything else (eg, 4, 5, 6) are Server.
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.
Flags: needinfo?(masayuki)
Our contact is able to reproduce now and filed a confluence bug here: https://jira.atlassian.com/browse/CONFCLOUD-65359

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.

Flags: needinfo?(twisniewski)

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?)

Flags: needinfo?(twisniewski)

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

Flags: needinfo?(twisniewski)

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"
Flags: needinfo?(twisniewski)

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?)

Flags: needinfo?(masayuki)

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?

Flags: needinfo?(masayuki) → needinfo?(bugs)

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.

Flags: needinfo?(bugs)

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.

status-firefox65: affected → disabled
Flags: needinfo?(cornel.ionce)

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

Flags: needinfo?(cornel.ionce)

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

Component: Desktop → DOM
Product: Tech Evangelism → Core
Version: Firefox 65 → 65 Branch

Trying to create the hack suggested by Ehsan.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attached patch Patch (WIP) (obsolete) — — Splinter Review

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

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

Flags: needinfo?(ehsan)

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

Flags: needinfo?(ehsan)

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

Attachment #9039032 - Attachment is obsolete: true

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?

Flags: needinfo?(ehsan)

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?

Flags: needinfo?(ehsan)
Flags: webcompat+
Whiteboard: [webcompat:p1]

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

Flags: needinfo?(ehsan)

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.

Flags: needinfo?(ehsan)
Attached file bug1514940.txt (obsolete) —

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

Attachment #9039479 - Attachment is obsolete: true
Attachment #9040323 - Flags: review?(chutten)
Comment on attachment 9040323 [details]
bug1514940.txt

Preliminary note:

I recommend setting the expiry on the probe even though we're not sure when we'll be able to remove it. That way we're forced to make a decision twice a year about whether we can get rid of the code yet or not. We also won't accidentally leave it in place and forget about it.

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes. This collection is Telemetry so is documented in its definitions file ([Histograms.json](https://hg.mozilla.org/mozilla-central/file/tip/toolkit/components/telemetry/Histograms.json)) and the [Probe Dictionary](https://telemetry.mozilla.org/probe-dictionary/).

    Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

    If the request is for permanent data collection, is there someone who will monitor the data over time?

No. This collection is set to never expire but no one is identified as being responsible for its ongoing collection.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 3, Web Activity. This probe measures whether someone visits a confluence editing page. I'm not sure if this is a problem, but it seems Cat-3 which means I'll need to pass this up to Legal & Trust for review.

    Is the data collection request for default-on or default-off?

Default on for all channels.

    Does the instrumentation include the addition of any new identifiers?

No.

    Is the data collection covered by the existing Firefox privacy notice?

...not 100% sure, since it appears to be Category 3 data.

    Does there need to be a check-in in the future to determine whether to renew the data?

Yes. According to the review request it needs to be removed at some unspecified later time.

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


Marshall, this probe is measuring a very specific plugin version that is known to only be used by Confluence instances. By collecting this we can infer that the user visited a Confluence editing page.

Does that make it Category 3 data? Does that make it ineligible for default-on collection in release?
Flags: needinfo?(merwin)
Attachment #9040323 - Flags: review?(chutten) → review-

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

Attached file bug1514940.txt (obsolete) —

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

Attachment #9040323 - Attachment is obsolete: true
Attachment #9041386 - Flags: review?(chutten)

hsinyi:

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

Flags: needinfo?(htsai)

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

Flags: needinfo?(htsai)
Comment on attachment 9041386 [details]
bug1514940.txt

Clearing review flag until Marshall has a chance to decide whether we can go ahead with this Category 3 collection being default-on on release.
Attachment #9041386 - Flags: review?(chutten)
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/25625eaa999d
part 1: Forcibly disable new keyCode/charCode value of keypress events if the document is Confluence r=smaug,Ehsan,kmag

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.

Attached patch Part 1 for Beta — — Splinter Review

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.

Attachment #9041701 - Flags: approval-mozilla-beta?

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?

Flags: needinfo?(aryx.bugmail)

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

Flags: needinfo?(aryx.bugmail)
Whiteboard: [webcompat:p1] → [webcompat:p1][qa-triaged]

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.

status-firefox67: --- → verified
Flags: qe-verify+
Comment on attachment 9041701 [details] [diff] [review]
Part 1 for Beta

Fix for a new regression, verified in nightly, includes new tests.
Let's uplift for beta 7.
Attachment #9041701 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

: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')

Flags: needinfo?(masayuki)

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

Flags: needinfo?(masayuki) → needinfo?(nbeleuzu)

Thank you Ryan, I`ve imported that patch

Flags: needinfo?(nbeleuzu)
QA Whiteboard: [qa-triaged]
Whiteboard: [webcompat:p1][qa-triaged] → [webcompat:p1]

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.

Flags: needinfo?(merwin)
Attached file bug1514940.txt —

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

Attachment #9041386 - Attachment is obsolete: true
Attachment #9050200 - Flags: data-review?(chutten)
Comment on attachment 9050200 [details]
bug1514940.txt

Because of the review comment of Jan-Erik, I change data-reviewer.
Attachment #9050200 - Flags: data-review?(chutten) → data-review?(liuche)
Component: DOM → DOM: Core & HTML
Comment on attachment 9050200 [details]
bug1514940.txt

data-review+
Only one clarification required on the documentation, commented in the patch.

# Data Review Form 

1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate? 
Yes, in Scalars.yaml

2) Is there a control mechanism that allows the user to turn the data collection on and off? 
Yes, Firefox data controls

3) If the request is for permanent data collection, is there someone who will monitor the data over time?**
No, expires in 71

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **
Type 2/3, but with low risk on a specific use case on a type of page.

5) Is the data collection request for default-on or default-off?
default on for release

6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?
No

7) Is the data collection covered by the existing Firefox privacy notice?
Yes - not collecting specific editing page, and cleared with Merwin.

8) Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No) (If yes, set a todo reminder or file a bug if appropriate)**
Expires in 71

9) Does the data collection use a third-party collection tool?
no
Attachment #9050200 - Flags: data-review?(liuche) → data-review+
Attachment #9039713 - Attachment description: Bug 1514940 - part 2: Add telemetry probe to decide when we can remove the hack for Confluence → Bug 1514940 - part 2: Add telemetry probe to decide when we can remove the hack for Confluence data-review=liuche,
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/92d9228ac82d
part 2: Add telemetry probe to decide when we can remove the hack for Confluence data-review=liuche, r=smaug,janerik

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

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
See Also: → 1540158
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: