STR

1. Run Firefox Nightly with apz.allow_zooming=true on a desktop platform
2. Load https://planet.mozilla.org/
3. Pinch-zoom out as far as possible.
4. Right-click on the menu to show the context menu

Expected results

The context menu is positioned such that its top left or bottom left corner is where the mouse click was.

Actual results

The context menu position is offset from the mouse click position. The discrepancy grows as the mouse click position moves away from the viewport origin.

The WIP patches for bug 1556556 do mostly fix this, although the context menu is still mispositioned during the async zooming phase (when we haven't repainted at the new resolution yet). I'll use this bug to track that issue.

Unassigning myself for now as the remaining issue is of lower priority.

I retested this with bug 1556556 in the tree, and found that context menu popups are mispositioned when zoomed even after a repaint, not just during the async zooming phase. This should probably block desktop-zoom-nightly.

The eContextMenu widget mouse event gets passed to content, ContextMenuChild.jsm gets the screenX and Y from it and passes that to the parent and the parent opens it at those coords. The problem comes when MouseEvent::GetScreenCoords converts from visual to layout

https://searchfox.org/mozilla-central/rev/dc4560dcaafd79375b9411fdbbaaebb0a59a93ac/dom/events/Event.cpp#553

then the point that ContextMenuChild gets via screenX/Y is no longer in screen coords.

Hmm, I'm not even sure it makes sense to adjust the screen coords of a mouse event on mobile: the values that come out don't seem useful. Unless somewhere undoes that visual to layout transform somewhere to balance it out and make the coords reasonable again.

Removing the visual to layout transform definitely makes geckoview_example misbehave, so it is needed.

Can you double check my thinking here?

In Event::GetScreenCoords

https://searchfox.org/mozilla-central/rev/fa52bedc4b401c12251513fa1c9df1753a29abb2/dom/events/Event.cpp#553

we convert from visual to layout coords. Event::GetScreenCoords is used for MouseEvent::ScreenX/Y and other things that seem like they would want screen (and hence visual) coords and not layout coords.

Doing that was added in https://hg.mozilla.org/mozilla-central/rev/22c0d6e4df2c I'm not really able to find an explanation in that bug.

Removing the VisualToLayout call in Event::GetScreenCoords results in a geckoview example that doesn't work: long tapping after zooming in doesn't highlight the expected word (or any word), and sending events to content doesn't seem to work as expected either (mouse move listeners don't get triggered for taps after zooming in).

Another problem I found that might or might not be related is in FindFrameTargetedByInputEvent at

https://searchfox.org/mozilla-central/rev/fa52bedc4b401c12251513fa1c9df1753a29abb2/layout/base/PositionedEventTargeting.cpp#527

We are adjusting the refpoint of a widget event and we use the viewport type of the passed in RelativeTo, which is layout some of the time. Widget refpoints should be in visual coords, so this seems wrong. Changing that to always be visual (and the GetScreenCoords change) doesn't fix long tapping etc.

(In reply to Timothy Nikkel (:tnikkel) from comment #7)

In Event::GetScreenCoords

https://searchfox.org/mozilla-central/rev/fa52bedc4b401c12251513fa1c9df1753a29abb2/dom/events/Event.cpp#553

we convert from visual to layout coords. Event::GetScreenCoords is used for MouseEvent::ScreenX/Y and other things that seem like they would want screen (and hence visual) coords and not layout coords.

Doing that was added in https://hg.mozilla.org/mozilla-central/rev/22c0d6e4df2c I'm not really able to find an explanation in that bug.

I agree that conceptually, screenX/Y should be in visual coordinates.

Bug 1556556 added the VisualToLayout() call to preserve the existing RemoveResolution() behaviour, but I also don't recall why that was added in the first place. Possibly there were some tests that made assertions about screenX/Y without taking resolution into account, and it was a convenient way to keep them working?

Removing the VisualToLayout call in Event::GetScreenCoords results in a geckoview example that doesn't work: long tapping after zooming in doesn't highlight the expected word (or any word), and sending events to content doesn't seem to work as expected either (mouse move listeners don't get triggered for taps after zooming in).

I don't have a theory off the top of my head for why removing that VisualToLayout() call would have these effects. Investigating that further may reveal another reason why some callers of GetScreenCoords() want layout coordinates.

Another problem I found that might or might not be related is in FindFrameTargetedByInputEvent at

https://searchfox.org/mozilla-central/rev/fa52bedc4b401c12251513fa1c9df1753a29abb2/layout/base/PositionedEventTargeting.cpp#527

We are adjusting the refpoint of a widget event and we use the viewport type of the passed in RelativeTo, which is layout some of the time. Widget refpoints should be in visual coords, so this seems wrong. Changing that to always be visual (and the GetScreenCoords change) doesn't fix long tapping etc.

The aViewportType parameter of nsLayoutUtils::TranslateViewToWidget() indicates the coordinate system of the input point. The output point is always in visual coordinates, and thus suitable for assinging to mRefPoint. (The implementation performs LayoutToVisual() if necessary to ensure the output is in visual coords).

In the case of the FindFrameTargetedByInputEvent call site, the input point may indeed be either in visual or layout coordinates.

(In reply to Timothy Nikkel (:tnikkel) from comment #7)

Removing the VisualToLayout call in Event::GetScreenCoords results in a geckoview example that doesn't work: long tapping after zooming in doesn't highlight the expected word (or any word), and sending events to content doesn't seem to work as expected either (mouse move listeners don't get triggered for taps after zooming in).

Although the geckoview example I was testing in was broken the problem was pre-existing, and seems to be caused by something that landed after May 3, bug 1556556 is the obvious first candidate to look consider. I will file a new bug for this.

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

The aViewportType parameter of nsLayoutUtils::TranslateViewToWidget() indicates the coordinate system of the input point. The output point is always in visual coordinates, and thus suitable for assinging to mRefPoint.

Of course! Simple mistake, thanks for finding it.

(In reply to Timothy Nikkel (:tnikkel) from comment #9)

(In reply to Timothy Nikkel (:tnikkel) from comment #7)

Removing the VisualToLayout call in Event::GetScreenCoords results in a geckoview example that doesn't work: long tapping after zooming in doesn't highlight the expected word (or any word), and sending events to content doesn't seem to work as expected either (mouse move listeners don't get triggered for taps after zooming in).

Although the geckoview example I was testing in was broken the problem was pre-existing, and seems to be caused by something that landed after May 3, bug 1556556 is the obvious first candidate to look consider. I will file a new bug for this.

New bug -> bug 1637113.

Screen coords are not layout coords.

https://hg.mozilla.org/mozilla-central/rev/715ea15fc504