Closed Bug 1342297 Opened 7 years ago Closed 5 years ago

Remove ReactDOM event system patch

Categories

(DevTools :: Framework, defect, P3)

Product:
DevTools
Component:
Framework
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED DUPLICATE of bug 1539979

People

(Reporter: ntim, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

Bug 1342297 - Add support for <iframe useContentEventHandling=true>.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1342297 - Fix inplace-editor autocomplete.
59 bytes, text/x-review-board-request
Details 
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.
Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
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
Attachment #8840710 - Flags: review?(bugs)
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
Attachment #8840710 - Flags: review?(bugs) → review+
Olli, thanks for the review!

New try looks better: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5737cc1ceed
Summary: Remove ReactDOM patch → Remove ReactDOM event system patch
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.)
Attachment #8841186 - Flags: review?(jdescottes)
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.
Assignee: ntim.bugs → nobody
Status: ASSIGNED → NEW
See Also: → 1420130
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?
Flags: needinfo?(mratcliffe)
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.
Flags: needinfo?(mratcliffe) → needinfo?(poirot.alex)
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...
Flags: needinfo?(poirot.alex)
Product: Firefox → DevTools
Moving this inactive P2 to the backlog (P3)
Priority: P2 → P3

@ntim: is this still something you are planning to work on?

Honza

Flags: needinfo?(ntim.bugs)

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #33)

@ntim: is this still something you are planning to work on?

No, but I believe jdescottes is picking up this work in different bugs for Fission.

Flags: needinfo?(ntim.bugs) → needinfo?(jdescottes)

Unless this is a blocker for fission I don't plan on picking this up. Nevertheless, I will block it on my current work from Bug 1539979.

Depends on: 1539979
Flags: needinfo?(jdescottes)
Attachment #8840730 - Attachment is obsolete: true
Attachment #8840730 - Flags: review?(poirot.alex)
Attachment #8840731 - Attachment is obsolete: true
Attachment #8840731 - Flags: review?(poirot.alex)
Attachment #8840732 - Attachment is obsolete: true
Attachment #8840732 - Flags: review?(poirot.alex)

(In reply to Julian Descottes [:jdescottes] from comment #35)

Unless this is a blocker for fission I don't plan on picking this up. Nevertheless, I will block it on my current work from Bug 1539979.

Since the actual patching was removed in bug 1420130, and using content iframes is covered by bug 1539979, I think it probably makes sense to dupe to one of the two bugs.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: