Use matrix instead of offset for converting between chrome and content process event coordinates in TabParent

RESOLVED FIXED in Firefox 67

Status

()

enhancement
RESOLVED FIXED
4 months ago
3 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

4 months ago

Various TabParent::Recv* methods, such as dictionary lookup, need to transform points from the coordinate space of the out-of-process iframe process to the coordinate space of the chrome process. To avoid bouncing via all the processes associated with the ancestors of the iframe in the Web hierarhy, we should probably get this info from APZ.

Assignee

Updated

4 months ago
Depends on: 1529239

Updated

4 months ago
Component: XUL → DOM: Content Processes
Assignee

Comment 1

4 months ago

Rescoping to cover TabParent::Sent* methods, too.

Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Summary: Correctly untransform the coordinates for messages received from oop-iframe TabChild → Use matrix instead of offset for converting between chrome and content process event coordinates in TabParent
Assignee

Updated

4 months ago
Blocks: 1528937
Assignee

Comment 3

4 months ago

I suppose it's possible for an out-of-process to have a non-invertible matrix applied to it. Is there a good way to compute an approximate inverse that e.g. for a rectangle collapsed to a line still transforms points so that they appear on the line?

Flags: needinfo?(botond)

Hmm, good question. Looking through APZ code, there do seem to be places where we invert a matrix that can include CSS transforms (and thus could potentially not be invertible), without checking for invertibility (e.g. here).

It's not clear to me whether we are missing checks for invertibility, or if there is some invariant that ensures we don't even get to that code (e.g., we don't even build a layer) if a CSS transform isn't invertible.

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

Is there a good way to compute an approximate inverse that e.g. for a rectangle collapsed to a line still transforms points so that they appear on the line?

Matt, do you know the answer to this?

Flags: needinfo?(botond) → needinfo?(matt.woodrow)

(In reply to Botond Ballo [:botond] from comment #4)

Hmm, good question. Looking through APZ code, there do seem to be places where we invert a matrix that can include CSS transforms (and thus could potentially not be invertible), without checking for invertibility (e.g. here).

It's not clear to me whether we are missing checks for invertibility, or if there is some invariant that ensures we don't even get to that code (e.g., we don't even build a layer) if a CSS transform isn't invertible.

We do try to avoid building layers for transforms with non-invertible matrices, but async animations can mean that a transform changes to be non-invertible at a later point. We should make sure we handle this on the APZ side.

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

Is there a good way to compute an approximate inverse that e.g. for a rectangle collapsed to a line still transforms points so that they appear on the line?

Matt, do you know the answer to this?

I'm not sure I understand the question entirely, but if a content process has a non-invertible matrix applied to it, then it won't be visible at all. No coordinates in the parent coordinate space map to anything in the content process, so we can just detect a non-invertible matrix and report that hit testing didn't find anything.

Flags: needinfo?(matt.woodrow)
Assignee

Comment 6

4 months ago

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)

I'm not sure I understand the question entirely, but if a content process has a non-invertible matrix applied to it, then it won't be visible at all. No coordinates in the parent coordinate space map to anything in the content process, so we can just detect a non-invertible matrix and report that hit testing didn't find anything.

This isn't about hit testing. This is about content saying to chrome that something happened that requires chrome to display some UI at the right location, so the inverse matrix transforms from the content coordinate space from the chrome space. We need something more sensible than crashing if the matrix isn't invertible.

One option is that if the matrix isn't invertible, we generate a point at the top left corner of the content area.

Assignee

Comment 7

4 months ago

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

One option is that if the matrix isn't invertible, we generate a point at the top left corner of the content area.

The latest just uses the top left corner of chrome coordinate space.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=71dc28300c5dc5fbf04f131b08de1efbf8b5440f

Whiteboard: [fission-event-m1]

Updated

4 months ago
Fission Milestone: --- → M2

Comment 8

4 months ago
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e544d9948446
Use matrix instead of offset for converting between chrome and content process event coordinates in TabParent. r=kats

Comment 9

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)

(In reply to Botond Ballo [:botond] from comment #4)

Hmm, good question. Looking through APZ code, there do seem to be places where we invert a matrix that can include CSS transforms (and thus could potentially not be invertible), without checking for invertibility (e.g. here).

It's not clear to me whether we are missing checks for invertibility, or if there is some invariant that ensures we don't even get to that code (e.g., we don't even build a layer) if a CSS transform isn't invertible.

We do try to avoid building layers for transforms with non-invertible matrices, but async animations can mean that a transform changes to be non-invertible at a later point. We should make sure we handle this on the APZ side.

(Just to close the loop here: the transforms we invert in APZ don't include OMTA transforms, only CSS transforms and APZ transforms. So, if only OMTA transforms can be non-invertible, we should be fine.)

Assignee

Updated

3 months ago
Duplicate of this bug: 1529239
You need to log in before you can comment on or make changes to this bug.