1733025 - fix macOS Lookup coords for fission

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1733025. Convert coords correctly when sending cmd_lookUpDictionary (macOS lookup) to child process. r?hiro,botond

this.browser in toolkit/actors/ControllersParent.jsm is the top level browser, ie the browser holding the root content document. So the conversion that happens in that file converts the coordinates to be relative to the root content document, but they need to be relative to the root of whichever child process we are sending the event to.

The best way I found out how to do this was to pass the coords down to the child process still relative to the parent process widget and then in the child process use the child to parent transform matrix to make them relative to the root widget in the child process.

I needed a new nsIDOMWindowUtils functions because I don't think there is anything existing to do this.

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1733025. Make sure the highlight text for macOS lookup feature has the right size with fission. r?botond,hiro

If we pinch zoomed with an oop iframe the font would not take into account the pinch zoom because GetCumulativeResolution in the child process was 1.

This fix also takes into account the scale from css transforms. This is notable because in non-fission cases (fission turned off, or not in an oop iframe) we do not take into account css transforms, even after this patch. (We target the correct word, but the highlight that is drawn can be at the wrong place and have the wrong size.)

Depends on D126861

Comment 3

[Timothy Nikkel (:tnikkel)]

Here's a little write up on how this feature works for my future reference or anyone else who finds it useful.

To activate this feature you press on the touchpad over a word, you'll feel one click, hold you finger there and then press harder, you'll feel a second click when the feature activates. A yellow highlight of the word, over top the word, is drawn by the OS, and a popup window with a definition of the word is shown by the OS.

This is initiated in the parent process at https://searchfox.org/mozilla-central/rev/8db61933e64b13c4a0ae456bcaccbd86a519ccc5/widget/cocoa/nsChildView.mm#3731 by a call from the OS. We take the current mouse position and put it on an event of type eContentCommandLookUpDictionary and dispatch it.

This ends up in EventStateManager.cpp https://searchfox.org/mozilla-central/rev/8db61933e64b13c4a0ae456bcaccbd86a519ccc5/dom/events/EventStateManager.cpp#6015 where we take the refpoint of the event and call commandController->DoCommandWithParams.

This gets to ControllersParent.jsm https://searchfox.org/mozilla-central/rev/8db61933e64b13c4a0ae456bcaccbd86a519ccc5/toolkit/actors/ControllersParent.jsm#63 (I'm a little fuzzy on how that happens exactly). We send a message to ControllersChild with the refpoint. ControllersChild just calls nsDocShell::DoCommandWithParams, which gets to nsLookUpDictionaryCommand::DoCommandParams which actually does the work. https://searchfox.org/mozilla-central/rev/8db61933e64b13c4a0ae456bcaccbd86a519ccc5/dom/base/nsGlobalWindowCommands.cpp#957

The work that it does consists of sending two "query content" events. These are a special type of event where when they are handled by content, content fills in a "reply" on the event, and the dispatcher of the event can look at the reply. It's almost like a function call, we pass the type of the event and the refpoint, we get back a rect or some other result. Except the target of the function call is determined based on the coords. We use two "query content" events to get the font size and rect of the full word under the mouse cursor. The code that is responsible for filling in the reply is in https://searchfox.org/mozilla-central/rev/8db61933e64b13c4a0ae456bcaccbd86a519ccc5/dom/events/ContentEventHandler.cpp

Now that we have the required information we call widget->LookUpDictionary on the puppet widget. This sends the LookUpDictionary call to the BrowserParent which in turn calls LookUpDictionary on it's widget, which is the original nsChildView, and finally the OS performs the Lookup for us.

When we pass down the mouse cursor location from the parent to the child we need to convert it to be relative to the child process, and this is where the bug is: ControllersParent translates the coords to the root content document no matter which child process we are sending the lookup to.

When we pass the rect of the word under the mouse cursor to the parent process from the child process we do so as a point of the top left, a fontsize, and the raw text of the word, this is enough to draw the word at the correct place and size. We need to translate the point from child relative coords to parent relative coords, that happens in BrowserParent::RecvLookUpDictionary. This is done correctly currently, there is no problem with this. The font size however does not take into account any resolution of css transform scale in ancestor processes, and that is what the second patch on this bug fixes.

Comment 4

[Timothy Nikkel (:tnikkel)]

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

When we pass down the mouse cursor location from the parent to the child we need to convert it to be relative to the child process, and this is where the bug is: ControllersParent translates the coords to the root content document no matter which child process we are sending the lookup to.

The patch I uploaded does the conversion in the child process (in ControllersChild) using the child to parent matrix. The child to parent matrix is on both the browser parent and browser child so the patch could theoretically be rewritten to do the conversion in the parent (in ControllersParent) with a similar structure (adding a function to nsDOMWindowUtils).

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9060eae95f29
Convert coords correctly when sending cmd_lookUpDictionary (macOS lookup) to child process. r=hiro
https://hg.mozilla.org/integration/autoland/rev/2f1a1c9216e8
Make sure the highlight text for macOS lookup feature has the right size with fission. r=botond

Comment 6

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/9060eae95f29
https://hg.mozilla.org/mozilla-central/rev/2f1a1c9216e8

Comment 7

[Chris Peterson [:cpeterson]]

Should we uplift this fix to Beta 94? We plan to ship Fission in Release 94. Dictionary lookup seems like an uncommon feature (and this bug was found by code audit, not a user filing a bug report), so I assume this fix can just ride the trains with 95.

Tentatively setting status-firefox94=wontfix and status-firefox93=disabled because Fission is disabled in 93.

Comment 8

[Timothy Nikkel (:tnikkel)]

Yes, I agree, since it was found by audit it's not very common.