Closed Bug 1785759 Opened 2 years ago Closed 2 years ago

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

Categories

(GeckoView Graveyard :: Sandboxing, defect, P2)

All
Android

Tracking

(firefox106 wontfix, firefox109 fixed)

RESOLVED FIXED
109 Branch
Tracking Status
firefox106 --- wontfix
firefox109 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fission:android:m2])

Attachments

(4 files)

Attached image 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)

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.

Flags: needinfo?(emilio)

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.

Severity: -- → S3
Priority: -- → P2
Whiteboard: [fission:android:m2]

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.

Assignee: nobody → 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?

Assignee: emilio → nobody
Flags: needinfo?(emilio) → needinfo?(m_kato)
Assignee: nobody → emilio
Status: NEW → ASSIGNED
Assignee: emilio → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(m_kato)
Attachment #9291628 - Flags: feedback+
Assignee: nobody → emilio
Status: NEW → ASSIGNED

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

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

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

Assignee: emilio → nobody
Status: ASSIGNED → NEW

emilio, do you continue to work on this?

Flags: needinfo?(emilio)

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.

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

Flags: needinfo?(emilio)

I take this.

Assignee: nobody → m_kato
Flags: needinfo?(m_kato)

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

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

Flags: needinfo?(m_kato)
Blocks: 1801615
Pushed by m_kato@ga2.so-net.ne.jp:
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
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 109 Branch

== 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

Product: GeckoView → GeckoView Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: