See bug 1245921 comments 59 to 68.

(In reply to Tim Nguyen :ntim from comment #3) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=2ed47e210b839ab8fb9a3d6e33a8b9edf180dec6 So the reason for the mass devtools failures is that keyboard shortcuts do not work anymore. Without useContentEventHandling, the keypresses were propagated from child to parent. Now that's no longer the case since content event handling doesn't propagate events from child to parent.

(In reply to Tim Nguyen :ntim from comment #4) > (In reply to Tim Nguyen :ntim from comment #3) > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=2ed47e210b839ab8fb9a3d6e33a8b9edf180dec6 > > So the reason for the mass devtools failures is that keyboard shortcuts do > not work anymore. Toolbox wide shortcuts don't work anymore. Pressing `Esc` will only trigger the keypress event for the focused iframe, and it won't propagate to the toolbox. Looks like a possible fix here is to propagate the keypresses manually using JS to the toolbox. We could also special case "keypress" in the platform code, but that would a very odd behaviour for the platform code.

The "HTML"Tooltip also relies on the chrome event behaviour. Looks like I need to fix that too.

(In reply to Tim Nguyen :ntim from comment #10) > The "HTML"Tooltip also relies on the chrome event behaviour. Looks like I > need to fix that too. Actually, I don't think it does. Seems like switching hosts does something strange to the event handling. I need to look closer into it.

Since switching hosts uses swapFrameLoaders, it might have something to do with this: [0]: https://dxr.mozilla.org/mozilla-central/source/dom/base/nsFrameLoader.cpp#1684 "Save off the tree owners, frame elements, chrome event handlers", mainly the event handlers part.

Comment on attachment 8840710 [details] Bug 1342297 - Add support for <iframe useContentEventHandling=true>. I think there's still stuff missing in the platform patch. I'll have to check for all event-related stuff here: https://dxr.mozilla.org/mozilla-central/search?q=nsIDocShellTreeItem%3A%3AtypeChrome&redirect=false

Comment on attachment 8840710 [details] Bug 1342297 - Add support for <iframe useContentEventHandling=true>. https://reviewboard.mozilla.org/r/115144/#review116876 ::: dom/base/nsFrameLoader.cpp:1832 (Diff revision 4) > ourDocshell->SetChromeEventHandler(otherChromeEventHandler); > otherDocshell->SetChromeEventHandler(ourChromeEventHandler); > // Restore the correct treeowners > // (and also chrome event handlers for content frames only). > SetTreeOwnerAndChromeEventHandlerOnDocshellTree(ourDocshell, otherOwner, > - ourType == nsIDocShellTreeItem::typeContent ? otherChromeEventHandler.get() : nullptr); > + ourType == nsIDocShellTreeItem::typeContent Could you made the method return failure if only one of the elements have useContentEventHandling set. So, either both should have it set or neither. Make that check early in the method. So, something like NS_ENSURE_STATE(ourFrameElement->AttrValueIs(kNameSpaceID_None, nsGkAtoms::useContentEventHandling, nsGkAtoms::_true, eCaseMatters) == otherFrameElement->AttrValueIs(kNameSpaceID_None, nsGkAtoms::useContentEventHandling, nsGkAtoms::_true, eCaseMatters)); around line 1712 ::: dom/base/nsFrameLoader.cpp:1834 (Diff revision 4) > // Restore the correct treeowners > // (and also chrome event handlers for content frames only). > SetTreeOwnerAndChromeEventHandlerOnDocshellTree(ourDocshell, otherOwner, > - ourType == nsIDocShellTreeItem::typeContent ? otherChromeEventHandler.get() : nullptr); > + ourType == nsIDocShellTreeItem::typeContent > + || !ourFrameElement->AttrValueIs(kNameSpaceID_None, nsGkAtoms::useContentEventHandling, > + nsGkAtoms::_true, eCaseMatters) ? otherChromeEventHandler.get() : nullptr); This all is hard to read. Add a helper variable bool wantsContentEventHandling = ourType == nsIDocShellTreeItem::typeContent || !ourFrameElement->AttrValueIs(kNameSpaceID_None, nsGkAtoms::useContentEventHandling, nsGkAtoms::_true, eCaseMatters); and use that in SetTreeOwnerAndChromeEventHandlerOnDocshellTree call. ::: dom/base/nsGlobalWindow.cpp:3780 (Diff revision 4) > > nsCOMPtr<Element> element = GetOuterWindow()->GetFrameElementInternal(); > nsIDocShell* docShell = GetDocShell(); > - if (element && GetParentInternal() && > - docShell && docShell->ItemType() != nsIDocShellTreeItem::typeChrome) { > + if (element && GetParentInternal() && docShell && > + (docShell->ItemType() != nsIDocShellTreeItem::typeChrome > + || element->AttrValueIs(kNameSpaceID_None, nsGkAtoms::useContentEventHandling, || should be in the previous line

Olli, thanks for the review! New try looks better: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5737cc1ceed

Comment on attachment 8841186 [details] Bug 1342297 - Fix inplace-editor autocomplete. https://reviewboard.mozilla.org/r/115494/#review117010 You need similar fixes for : - inspector-search.js - jsterm.js - rules.js & computed.js for the filter input (#ruleview-searchbox) - fonts.js for #font-preview-text-input (and maybe others?) Clearing review flag based on that. Another solution would be to skip some events in setIframeKeypressListener in toolbox.js (based on the event target?). For instance we could decide on a classname that we set on all inputs for which keypress events should not be forwarded. (Probably not linked to this particular changeset, but I noticed that the split console shortcut no longer works.)

Comment on attachment 8840732 [details] Bug 1342297 - Propagate keypress events from the child frames to the toolbox. https://reviewboard.mozilla.org/r/115174/#review117070 Did you tried listening for key* events on capture? addEventListener("keypress", function () {}, true); Or play with with wantsUntrusted argument? http://searchfox.org/mozilla-central/source/dom/interfaces/events/nsIDOMEventTarget.idl#76 This event forwarding feels weak.

Should this resolved as a duplicate of bug 1420130? It looks like there are still a few places where events are duplicated, which we're currently working around with some `stopPropgation` calls. Is that good enough, or are we still interested in the approach here which would opt into content style event handling consistenly?

It all depends... @Alex: Is the amount of work involved here justified over us adding a few stopPropagation calls as we currently do? If it isn't worth the work we should close this now that the monkeypatch has been removed.

ntim approach is surely more complex as it involves patching native code. But it also looks less prone to errors. From what I understood of the event issue, adding these stopPropagation calls is unnatural. Also using content style behavior would help us support launchpad setup (by running the whole toolbox instead of just one tool) and running the tools in unprivileged frames (I.e. content ones). It may also help us running UI in a content process. Now, none of this is an immediate goal...

Moving this inactive P2 to the backlog (P3)