1619186 - macOS "Look Up" spawns popup in the wrong place when the page is zoomed in with apz.allow_zooming=true

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[hujq]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:75.0) Gecko/20100101 Firefox/75.0

Steps to reproduce:

1. Set apz.allow_zooming=true
2. Open any page with text and pinch to zoom in with touchpad
3. Try to use "Look Up" on a word by moving the mouse cursor to that word and force-clicking on touchpad or pressing the Cmd-Ctrl-D shortcut

Actual results:

The "Look Up" window does not appear at all or opens with a different word, which seems to be the one on that position before zooming.

Expected results:

The "Look Up" window should show up for the word that's under the mouse cursor when the page at the current zoom level.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Botond Ballo [:botond]]

We should re-test this once bug 1556556 lands, as that may fix it.

Comment 3

[Botond Ballo [:botond]]

Kats, would you be able to re-test this on a Mac with bug 1556556 in the tree?

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Screen Shot 2020-05-08 at 04.20.15.png

Yeah, it's still happening.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Actually, I should clarify: the word that gets selected is the right one - it's the one under the cursor post-zoom. However the popup and word highlight appears in the wrong place. So this might actually get fixed by one of the other bugs about popup windows spawning in the wrong place.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

This code may or may not be relevant: https://searchfox.org/mozilla-central/rev/3d39d3b7dd1b2be30692d4541ea681614e34c786/toolkit/actors/ControllersParent.jsm#69-85 (I just came across it during unrelated investigations)

Comment 7

[Timothy Nikkel (:tnikkel)]

That code seems okay, it's dealing in visual coords in the parent process. This code seems closer to the problem

https://searchfox.org/mozilla-central/rev/3d39d3b7dd1b2be30692d4541ea681614e34c786/dom/base/nsGlobalWindowCommands.cpp#1009-1029

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: fixlookup

This is a minimal patch that fixes this bug for me. There is probably a lot of other coord issues like this in this file (dom/events/ContentEventHandler.cpp), those issues might affect things like IME or whatever else functions in this file are used for.

Comment 9

[Timothy Nikkel (:tnikkel)]

If anyone else wants this feel free to take it.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Created attachment 9158388 [details] [diff] [review]
fixlookup

This is a minimal patch that fixes this bug for me. There is probably a lot of other coord issues like this in this file (dom/events/ContentEventHandler.cpp), those issues might affect things like IME or whatever else functions in this file are used for.

Yeah, the coordinates are used to specify position of IME own windows. In other words, if current ContentEventHandler is not APZ-aware, IME's window is shown at wrong position too.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This might cause something wrong on Android if same zooming code is used.

Comment 12

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #11)

This might cause something wrong on Android if same zooming code is used.

Yes. https://bugzilla.mozilla.org/show_bug.cgi?id=1597897

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Masayuki sent me some instructions on how to exercise different codepaths in ContentEventHandler.cpp and indeed there other things that will need updating for APZ zoom. i.e. the patch on this bug doesn't fix all the problems. I'll spin off separate bugs with STRs.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1619186 - Refactoring to avoid use FromUnknownRect. r?masayuki

No functional changes, just some cleanup.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1619186 - Apply the layout-to-visual transform when querying text/caret rects. r?masayuki,botond

Depends on D86982

Comment 16

[Kartikaya Gupta (email:[e-mail])]

After the patches in this bug and the one on bug 1658927, there's still a conversion in QueryContentRect that looks like it should have this LayoutToVisual conversion, but it's not clear to me what user-visible effect that might have. :masayuki if you know, please let me know so I can test that.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff0cfe6047ed
Refactoring to avoid use FromUnknownRect. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/b6c0086812a8
Apply the layout-to-visual transform when querying text/caret rects. r=masayuki,botond

Comment 18

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/ff0cfe6047ed
https://hg.mozilla.org/mozilla-central/rev/b6c0086812a8