Find TabParent to dispatch to by LayersId attached to the event

RESOLVED FIXED in Firefox 67

Status

()

defect
P2
normal
RESOLVED FIXED
5 months ago
2 months ago

People

(Reporter: hsivonen, Assigned: hsivonen)

Tracking

(Blocks 1 bug)

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Fission Milestone:M2, firefox67 fixed)

Details

(Whiteboard: [fission-event-m1])

Attachments

(1 attachment)

Assignee

Description

5 months ago

In EventStateManager::DispatchCrossProcessEvent(), when about to dispatch a cross-process event, if the event has an attached LayersId (in the field added in bug 1524226), use that LayersId to obtain a TabParent via TabParent::GetTabParentFromLayersId() and dispatch to that TabParent (as opposed to a TabParent obtained from nsSubDocumentFrame).

If the event carries a transform matrix in the field added in bug 1524226, apply the transform to the event coordinates before dispatching the event (or should the recipient apply the transform since the matrix is remotable anyway?).

Priority: -- → P2
Whiteboard: [fission-event-m1]
Assignee

Updated

4 months ago
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Assignee

Comment 1

4 months ago

(In reply to Henri Sivonen (:hsivonen) from comment #0)

If the event carries a transform matrix in the field added in bug 1524226, apply the transform to the event coordinates before dispatching the event (or should the recipient apply the transform since the matrix is remotable anyway?).

It appears that in e10s, the transform is applied by the parent:
https://searchfox.org/mozilla-central/source/dom/ipc/TabParent.cpp#1051

Currently, the transform is characterized by a point:
https://searchfox.org/mozilla-central/source/dom/ipc/TabParent.cpp#1980

The main problem is that the coordinates on the event are of type LayoutDeviceIntPoint. Yet, with out-of-process iframes, a content process isn't guaranteed to be merely translated but could have an arbitrary transform matrix applied to it.

I think this leaves 3 options:

  1. Convert ints to floats, transform, convert back to ints. Works for normal cases where iframes are merely translated. Loss of coordinate precision when iframes are scaled and rotated.

  2. Make all processes receive events in screen coordinates. Make processes aware of their top-level transform relative to the screen. Rely on whatever mechanism layout presently has for dealing with rotated and scaled in-process iframes.

  3. Change events to have float-typed coordinates. Looks very invasive.

I'm tempted to go with option #1, but first I'd like to know what kind of coordinate handling out-of-process iframes currently have. rhunt, any advice?

Also, does WebRender composite chrome and the top level of content the same way as out-of-process iframes? I.e. is the top-level child process offset already baked into WebRender's matrices or is the offset between chrome and top-level content a special case outside WebRender?

Flags: needinfo?(rhunt)
Assignee

Comment 2

4 months ago

Option #1 is the closest to the plan, but when writing the plan, I mistakenly thought that event coordinates were already floats.

Assignee

Comment 3

4 months ago

Furthermore, it seems that only mRefPoint of the event is translated to the content process coordinate space in e10s and mLastRefPoint is never translated. It looks like mLastRefPoint is only used for DOM movementX/movementY values, which appear to be in screen space scaled to CSS pixels. Superficially, it looks like movementX/movementY are computed in the content process, yet only one input to the computation (mRefPoint) is translated. smaug, what mechanism makes the movementX/movementY correct when mRefPoint is translated to the content process coordinate space but mLastRefPoint is not?

Flags: needinfo?(bugs)
Assignee

Comment 4

4 months ago

https://hg.mozilla.org/mozilla-central/rev/58e9c32d5e53 suggests that our code is already known-wrong when there's a transform that's not strictly a translation.

Assignee

Comment 5

4 months ago

Also, when the transform is not a mere translate, the "LayoutDevice" type part of LayoutDeviceIntPoint becomes a lie. :-(

(In reply to Henri Sivonen (:hsivonen) from comment #2)

Option #1 is the closest to the plan, but when writing the plan, I mistakenly thought that event coordinates were already floats.

Note that currently when we do a hit-test on the main thread across a transform, we should be maintaining precision to the nearest app unit - see code here. So if instead start rounding to nearest LayoutDevice pixel that will probably be a regression of sorts. Not sure if it will be user-detectable but I'm guessing yes.

(In reply to Henri Sivonen (:hsivonen) from comment #3)

Furthermore, it seems that only mRefPoint of the event is translated to the content process coordinate space in e10s and mLastRefPoint is never translated. It looks like mLastRefPoint is only used for DOM movementX/movementY values, which appear to be in screen space scaled to CSS pixels. Superficially, it looks like movementX/movementY are computed in the content process, yet only one input to the computation (mRefPoint) is translated. smaug, what mechanism makes the movementX/movementY correct when mRefPoint is translated to the content process coordinate space but mLastRefPoint is not?

It looks like the mLastRefPoint is updated in EventStateManager from the mRefPoint value. Presumably this happens in both the UI process and the content process. When it happens in the content process, the mRefPoint value being used is the translated one, so mLastRefPoint will also be in the translated coordinate space.

Comment 7

4 months ago

Yeah this code is pretty broken with oop-iframe's.

I just did some light reading through the code, not an expert on all the requirements. I agree that (1) seems like a good option for standing things up, but I'd also be concerned about precision.

I've got to go away for a bit, so I'll keep the ni? and try and think more about it.

Assignee

Comment 8

4 months ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)

(In reply to Henri Sivonen (:hsivonen) from comment #2)

Option #1 is the closest to the plan, but when writing the plan, I mistakenly thought that event coordinates were already floats.

Note that currently when we do a hit-test on the main thread across a transform, we should be maintaining precision to the nearest app unit - see code here. So if instead start rounding to nearest LayoutDevice pixel that will probably be a regression of sorts. Not sure if it will be user-detectable but I'm guessing yes.

Even though the type currently says LayoutDevicePixel, the rounding would be to integer in the coordinate space of the out-of-process iframe. I assume that if the iframe has a scale transform applied to it, the integer rounding wouldn't actually correspond to rounding to device pixel precision.

But I really don't know properly how the top-level coordinate space of an out-of-process iframe works at present.

(In reply to Henri Sivonen (:hsivonen) from comment #3)

Furthermore, it seems that only mRefPoint of the event is translated to the content process coordinate space in e10s and mLastRefPoint is never translated. It looks like mLastRefPoint is only used for DOM movementX/movementY values, which appear to be in screen space scaled to CSS pixels. Superficially, it looks like movementX/movementY are computed in the content process, yet only one input to the computation (mRefPoint) is translated. smaug, what mechanism makes the movementX/movementY correct when mRefPoint is translated to the content process coordinate space but mLastRefPoint is not?

It looks like the mLastRefPoint is updated in EventStateManager from the mRefPoint value. Presumably this happens in both the UI process and the content process. When it happens in the content process, the mRefPoint value being used is the translated one, so mLastRefPoint will also be in the translated coordinate space.

Thanks. Clearing ni for smaug.

(In reply to Ryan Hunt [:rhunt] from comment #7)

Yeah this code is pretty broken with oop-iframe's.

I just did some light reading through the code, not an expert on all the requirements. I agree that (1) seems like a good option for standing things up, but I'd also be concerned about precision.

I've got to go away for a bit, so I'll keep the ni? and try and think more about it.

Thanks. I'll proceed with option 1.

Flags: needinfo?(bugs)
Assignee

Updated

4 months ago
Depends on: 1529594

Updated

4 months ago
Fission Milestone: --- → M2

Comment 11

4 months ago
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e5162aed5ec0
Find TabParent to dispatch to by LayersId attached to the event. r=nika

Comment 12

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Component: Event Handling → User events and focus handling
Assignee

Updated

3 months ago
Regressions: 1541088

Updated

2 months ago
Flags: needinfo?(rhunt)
You need to log in before you can comment on or make changes to this bug.