1785759 - Action bar of selection is overlapped with text selection when using Fission

Status: RESOLVED FIXED

(GeckoView :: Sandboxing, defect, P2)

Description

[Makoto Kato [:m_kato]]

Attachment: screenshot with fission

Step

1. set fission.autostart=true or ./mach run --enable-fission
2. Browse https://wontfix.net/bugs/iframe1.html
3. Zoom content
4. Select text in <textbox> into <iframe>

Result

Action bar is overlapped with selected text.

Expected Result

Same as non-fission or non-xorigin content (https://wontfix.dev/bugs/iframe1.html)

Comment 1

[Makoto Kato [:m_kato]]

https://searchfox.org/mozilla-central/rev/f3616b887b8627d8ad841bb1a11138ed658206c5/mobile/android/actors/SelectionActionDelegateChild.jsm#315,323-330 doesn't seem to be fission compatible.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

It appears that the _getFrameOffset function (https://searchfox.org/mozilla-central/rev/30ac44262374be08ccd4262ee36f0716152868d3/mobile/android/actors/SelectionActionDelegateChild.jsm#191-236) is written in such a way that it tries to traverse across process boundaries when running with Fission, and doesn't correctly respect the process-root

I don't know off the top of my head what the best solution is for this, though I know that coordinates in nested iframes are particularly fun (thanks to things like transforms on the frame element themselves). I'll ni? :emilio who probably has more context on this, but it might be possible to copy/share the logic which Select uses for this (https://searchfox.org/mozilla-central/rev/1f9b94dc3046fb5db1fa604f977292541b35c156/toolkit/modules/LayoutUtils.jsm#11-48) and get something which works cross-process.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

When writing tests for this, it might also be good to have a test where the cross-process iframe element is distorted using CSS or similar as well.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Yes, this should just use the LayoutUtils code, probably refactored like:

getElementBoundingScreenRect(aElement) {
  return this.boundingRectToScreenRect(aElement.getBoundingClientRect(), aElement.ownerGlobal);
}

boundingRectToScreenRect(aRect, aGlobal) {
  // current code.
}

That would account for the layout to visual viewport offset in the parent process, which is what's not getting accounted for here... I can take this I think.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: WIP: Bug 1785759 - wip

Comment 6

[Emilio Cobos Álvarez (:emilio)]

So the attached patch works and gives consistent coordinates within fission / non-fission, but the coordinate space conversion in java is off, and I'm not familiar with the gazillion coordinate spaces there. Makoto, could you help with that?

Comment 7

[Makoto Kato [:m_kato]]

Sorry for too late. As long as I test, it works well.

Also, when change API, we have to update the following files, too.

• Update https://searchfox.org/mozilla-central/source/mobile/android/geckoview/api.txt
• Adding https://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md

If you add utility methods (getElementBoundingScreenRect) in LayoutUtils.jsm, I can update other parts.

Comment 8

[Makoto Kato [:m_kato]]

emilio, do you continue to work on this?

Comment 9

[Makoto Kato [:m_kato]]

If you don't have more time to work this, I will take this since I need LayoutUtils's code in other bug to support fission on GV.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

I'm a bit busy with other work so if you need it please go ahead. Happy to review or what not :)

Comment 11

[Makoto Kato [:m_kato]]

I take this.

Comment 12

[Makoto Kato [:m_kato]]

Attachment: Bug 1785759 - Part 1. Add LayoutUtils.rectToScreenRect. r=emilio

I would like screen coordinate version by screen pixel for
getElementBoundingScreenRect for GeckoView to support fission since it
returns CSS pixel.

Comment 13

[Makoto Kato [:m_kato]]

Attachment: Bug 1785759 - Part 2. Make BasicSelecitonDelegate fission compatible. r=#geckoview-reviewers

SelectionActionDelegate.Selection.clientRect isn't fission compatible. It is
better to use screen coordinate for selection data, instead of client
coordinate since it has no easy way to calculate client coordinate with
fission.

If we can have screenRect, mTempMatrix and mTempRect are unnecessary, so
we should be marked as deprecated too.

Depends on D161415

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c74803d53178
Part 1. Add LayoutUtils.rectToScreenRect. r=emilio
https://hg.mozilla.org/integration/autoland/rev/69a91cebaad5
Part 2. Make BasicSelecitonDelegate fission compatible. r=geckoview-reviewers,calu

Comment 15

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/c74803d53178
https://hg.mozilla.org/mozilla-central/rev/69a91cebaad5

Comment 16

[Andrej Glavic (:andrej)]

== Change summary for alert #36207 (as of Mon, 28 Nov 2022 18:42:19 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
15%	microsoft loadtime	macosx1015-64-shippable-qr	cold fission webrender	317.46 -> 268.42
15%	microsoft loadtime	macosx1015-64-shippable-qr	cold fission webrender	309.44 -> 261.75

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=36207