Once more, I'm away till August 15, so if anyone wants this landed before then, they should land it themselves. This applies on top of bug 1269209 and bug 1271115 (possibly specifically in that order). https://treeherder.mozilla.org/#/jobs?repo=try&revision=284c43e04b2e&exclusion_profile=false

Fixed for try failures, but now test_contenteditable_text_input_handling.html crashes when dispatching a key event -- which with this patch is done using SpecialPowers.wrap() to make the event trusted. But now it gives me "Assertion failure: mNativeKeyCommandsValid", from bug 993714, which you reviewed, so hopefully you know what the problem is (I couldn't understand what was going on).

Sorry for the long delay to review it. I'll separate the patch to each test and review them perhaps next month.

Oops, sorry for the delay. I'll try to review this later.

Comment on attachment 8750102 [details] [diff] [review] 0005-Bug-1271125-Port-editor-mochitests-that-depend-on-Ch.patch Oh, I'm confused with bug 1269209. This is enough small to review without separating patch nor MozReview. >diff --git a/editor/libeditor/tests/test_contenteditable_text_input_handling.html b/editor/libeditor/tests/test_contenteditable_text_input_handling.html >index c02304f7..0eba60c 100644 >--- a/editor/libeditor/tests/test_contenteditable_text_input_handling.html >+++ b/editor/libeditor/tests/test_contenteditable_text_input_handling.html > function runTests() > { >- var fm = Components.classes["@mozilla.org/focus-manager;1"]. >- getService(Components.interfaces.nsIFocusManager); >+ var fm = SpecialPowers.Cc["@mozilla.org/focus-manager;1"] >+ .getService(SpecialPowers.Ci.nsIFocusManager); SpecialPowers.focusManager is available now. https://dxr.mozilla.org/mozilla-central/rev/24763f58772d45279a935790f732d80851924b46/testing/specialpowers/content/specialpowersAPI.js#1648 > > var listener = { > handleEvent: function _hv(aEvent) > { > aEvent.preventDefault(); // prevent the browser default behavior > } > }; >- var els = Components.classes["@mozilla.org/eventlistenerservice;1"]. >- getService(Components.interfaces.nsIEventListenerService); >+ var els = SpecialPowers.Cc["@mozilla.org/eventlistenerservice;1"] >+ .getService(SpecialPowers.Ci.nsIEventListenerService); > els.addSystemEventListener(window, "keypress", listener, false); SpecialPowers.addSystemEventListener() is available now. https://dxr.mozilla.org/mozilla-central/rev/24763f58772d45279a935790f732d80851924b46/testing/specialpowers/content/specialpowersAPI.js#1577

(In reply to :Aryeh Gregor (working until September 2) from comment #2) > Created attachment 8750102 [details] [diff] [review] > 0005-Bug-1271125-Port-editor-mochitests-that-depend-on-Ch.patch > > Fixed for try failures, but now > test_contenteditable_text_input_handling.html crashes when dispatching a key > event -- which with this patch is done using SpecialPowers.wrap() to make > the event trusted. But now it gives me "Assertion failure: > mNativeKeyCommandsValid", from bug 993714, which you reviewed, so hopefully > you know what the problem is (I couldn't understand what was going on). Oh, the patch still has the problem? Dispatched KeyboardEvent shouldn't be handled by editor. Now, EditorEventListener listens only trusted keyboard events: https://dxr.mozilla.org/mozilla-central/rev/24763f58772d45279a935790f732d80851924b46/editor/libeditor/EditorEventListener.cpp#164-166 However, if you dispatch keyboard event from chrome script, the event becomes trusted event but it doesn't have proper mWidget and native key binding information (e.g., Linux and Mac specific keyboard shortcuts which depend on system settings are performed with the information). Therefore, when PuppetWidget::ExecuteNativeKeyBinding() is called, the keyboard event is assumed that it was dispatched from widget, not from JS. There are some choices to fix the crash: 1. Does nothing if WidgetKeyboardEvent::mWidget is nullptr in PuppetWidget::ExecuteNativeKeyBinding() This is the simplest fix, but this may cause some incompatible issues of add-ons. 2. EditorBase::IsAcceptableInputEvent() should require mWidget of eKeyPress https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/EditorBase.cpp#5042,5068,5081 However, this is perhaps the worst choice because add-ons cannot perform anything with dispatching DOM KeyboardEvent even from chrome script to an editor. 3. PuppetWidget::ExecuteNativeKeyBindings() should request native key bindings to the parent process I think that we should take this. See PuppetWidget::DispatchEvent(). When PuppetWidget receives a native eKeyPress event from the parent process, it requests native shortcut key list with calling RequestNativeKeyBindings(). https://dxr.mozilla.org/mozilla-central/rev/24763f58772d45279a935790f732d80851924b46/widget/PuppetWidget.cpp#318,328-333 Please check if WidgetKeyboardEvent::mWidget is nullptr when you call RequestNativeKeyBinding() from ExecuteNativeKeyBindings(). If it's not a nullptr, we have a bug like this crash.

Still fails the same assert.

> if (aEvent.mFlags.mIsSynthesizedForTests && !mNativeKeyCommandsValid) { aEvent.mFlags.mIsSynthesizedForTests is true only when the event is generated with EventUtils.synthesizeKey(). You should check: if (!aEvent.mWidget && !mNativeKeyCommandsValid) { MOZ_ASSERT(!aEvent.mFlags.mIsSynthesizedForTests); // Shouldn't refer native information with untrusted keyboard event since // allowing to do it causes leaking the system settings. if (NS_WARN_IF(!aEvent.IsTrusted())) { return false; } mTabChild->RequestNativeKeyBindings(&autoCache, &aEvent); }

Oops, sorry for the bug spam, please ignore my previous comment if you don't hit the assertion anymore. Thunderbird on my environment failed to receive new bugmail. Therefore, I read and reply to comment 7.

Comment on attachment 8784378 [details] [diff] [review] 0001-Bug-1271125-Port-editor-mochitests-that-depend-on-Ch.patch So this causes a failure I don't understand: https://treeherder.mozilla.org/logviewer.html#?job_id=26348497&repo=try#L10159 (The failures in other files are not caused by this change.)

Yeah, really odd. Does this really a new regression of the patch for this bug? I.e., not reproduced only with the patch for bug 1271115? And rebasing with newer tree doesn't solve it? Anyway, I'll check more... # I think that you should separate the patch for checking which part of the patch causes the orange...

Here's the problem: https://hg.mozilla.org/try/rev/7b53162c7c00315b9c8c2c10da673add33f105e8#l8.12 - var dataTransfer = event.dataTransfer; + var dataTransfer = SpecialPowers.wrap(event.dataTransfer); This is needed to make plain mochitests work, but it breaks chrome mochitests. Joel, what should I do to fix this?

we should be able to detect which harness we are running in and put a condition in there to use different methods. is Chrome the only harness that has problems? not browser-chrome, a11y, jetpack? It might make sense to add a variable to Simpletest that when we launch the harness it takes input of the harness we are running and stores it in Simpletest- we would have the check parent/widow.opener/etc. to ensure we get that- maybe it gets stored in specialpowers. Another unique thing about Chrome is that it has full access, so we don't really need specialpowers, we could check a restricted operation that requires specialpowers and if we don't need it ignore specialpowers.wrap

What condition would you suggest adding, and where?

hmm, I didn't have anything specific in mind, just that it could be possible. we currently pass a url to the browser to self start, for browser-chrome it loads a json file with the config issue, for other harnesses it passes information in via urlparams. I would imagine we could pass in the harness that way and then we can make smarter decisions in specialpowers.

Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93b94646fa4920c68c51e412bb6908c379b14e6b (The apparent consistent failures in functional tests and reftests seem to be in a base revision, because they appear in all my patches and are totally unrelated to any of my changes.)

Comment on attachment 8805809 [details] Bug 1271125 part 1 - wrap() only for non-chrome; https://reviewboard.mozilla.org/r/89456/#review88726

Comment on attachment 8805810 [details] Bug 1271125 part 2 - Port editor mochitests that depend on ChromeUtils.js to mochitest-plain; https://reviewboard.mozilla.org/r/89458/#review88774 Although, I don't think that it's worthwhile to run test_contenteditable_text_input_handling.html in e10s mode because the changes in editor will notified to native IME via IPC and it don't emulate the actual path. However, it's okay for now because it's green on tryserver.

One last (I hope) failure that I can't figure out: https://treeherder.mozilla.org/logviewer.html#?job_id=30172590&repo=try#L5642 It's only on OS X and Windows, and my development machine is Linux, so I didn't try to reproduce locally. If I don't get it sorted out before I get a green try run for the rest, I'll check in everything else and leave this for a follow-up.

(In reply to Aryeh Gregor (:ayg) (working until November 1) from comment #23) > One last (I hope) failure that I can't figure out: > > https://treeherder.mozilla.org/logviewer.html#?job_id=30172590&repo=try#L5642 > > It's only on OS X and Windows, and my development machine is Linux, so I > didn't try to reproduce locally. If I don't get it sorted out before I get > a green try run for the rest, I'll check in everything else and leave this > for a follow-up. It's stopped at dispatching key events. The test dispatches trusted Backspace key events even without editable element having focus. Then, the original test prevents its default with window's keypress event listener. However, it's changed to plain now. I *guess* that the event listener is now added after the event handler for "History Back". How about to add event listener to the document or document.documentElement instead of window? If they don't work, I think that we should make the test keep in chrome tests.

Comment on attachment 8805810 [details] Bug 1271125 part 2 - Port editor mochitests that depend on ChromeUtils.js to mochitest-plain; https://reviewboard.mozilla.org/r/89458/#review89084 ::: editor/libeditor/tests/test_contenteditable_text_input_handling.html:206 (Diff revision 1) > const nsIDOMKeyEvent = Components.interfaces.nsIDOMKeyEvent; > dispatchKeyEvent(nsIDOMKeyEvent.DOM_VK_BACK_SPACE, 0, aTarget); > dispatchKeyEvent(nsIDOMKeyEvent.DOM_VK_BACK_SPACE, 0, aTarget); > dispatchKeyEvent(nsIDOMKeyEvent.DOM_VK_BACK_SPACE, 0, aTarget); Could you rewrite them with KeyboardEvent.DOM_VK_BACK_SPACE? We don't need to refer nsIDOOMKeyEvent anymore.

Pushed by ayg@aryeh.name: https://hg.mozilla.org/integration/mozilla-inbound/rev/0f35e2e5b575 part 1 - wrap() only for non-chrome; r=jmaher https://hg.mozilla.org/integration/mozilla-inbound/rev/53516fd74efb part 2 - Port editor mochitests that depend on ChromeUtils.js to mochitest-plain; r=masayuki

Greenish try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe56506f5253 The failing tests on Android were disabled for the inbound push. If I get a green try run for test_contenteditable_text_input_handling.html following the strategy given in comment 24, I'll push that separately.

Comment on attachment 8805810 [details] Bug 1271125 part 2 - Port editor mochitests that depend on ChromeUtils.js to mochitest-plain; https://reviewboard.mozilla.org/r/89458/#review89084 > Could you rewrite them with KeyboardEvent.DOM_VK_BACK_SPACE? We don't need to refer nsIDOOMKeyEvent anymore. I pushed this before I saw the comment. (Did you not give r+ to this commit and I got confused? Or did you give r+ and then later add a new issue?) Do you want a followup?

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #24) > It's stopped at dispatching key events. > > The test dispatches trusted Backspace key events even without editable > element having focus. Then, the original test prevents its default with > window's keypress event listener. However, it's changed to plain now. I > *guess* that the event listener is now added after the event handler for > "History Back". How about to add event listener to the document or > document.documentElement instead of window? If they don't work, I think that > we should make the test keep in chrome tests. Didn't work with document instead of window: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6dd179bc191&selectedJob=30237796 So I guess I'll leave it alone.

https://hg.mozilla.org/mozilla-central/rev/0f35e2e5b575 https://hg.mozilla.org/mozilla-central/rev/53516fd74efb