1780093 - Text selection magnifier missing

Status: RESOLVED FIXED

(GeckoView :: Core, defect, P1)

Description

[Kevin Brosnan [Ex-Mozilla]]

From github: https://github.com/mozilla-mobile/fenix/issues/26066.

Steps to reproduce

I remember text selection magnifier was added few months ago but since last few weeks I am observing this is not May I know why the feature removed?

Expected behaviour

On selecting text magnifier should show

Actual behaviour

Magnifier no longer showing

Device name

asus rog 3

Android version

android 12

Firefox release type

Firefox Nightly

Firefox version

latest

Device logs

No response

Additional information

No response

┆Issue is synchronized with this Jira Task

Change performed by the Move to Bugzilla add-on.

Comment 1

[Makoto Kato [:m_kato]]

I guess that turning on GPU process causes this issue.

Comment 2

[Chris Peterson [:cpeterson]]

(In reply to Makoto Kato [:m_kato] from comment #1)

I guess that turning on GPU process causes this issue.

Makoto, is this a bug we need to fix in GeckoView? Or should I assign this bug to the Graphics team?

If this bug is a regression caused by the GPU process, then Fenix versions >= 101 will be affected. Uplifting a fix to Beta 104 would be good.

Comment 3

[Jamie Nicol [:jnicol]]

Can we get a regression range? Disabling the GPU process doesn't fix this for me (though of course it is possible that a GPU process related change broke this even when it's disabled)

Comment 4

[Chris Peterson [:cpeterson]]

Lorand, you said (on GitHub) that were able to you reproduce this bug in Nightly 104.

Can you please try testing Fenix Release (not Nightly) 100 and 101? If you can reproduce the bug in 100, then we will know this isn't a GPU process bug (because the GPU process shipped in 101). You should test Release instead of Nightly because the GPU process was enabled in Nightly 100 and earlier, so comparing Nightly 100 versus 101 won't help us identify if this is a GPU process bug.

If you know how to use mozregression and adb to bisect GeckoView Nightly builds (using something like mach mozregression --app gve), that would be even more help than test Release 100.

Comment 5

[Makoto Kato [:m_kato]]

Makoto, is this a bug we need to fix in GeckoView? Or should I assign this bug to the Graphics team?

I don't know what change of rendering issue. It is better to investigate gfx team.

Comment 6

[Makoto Kato [:m_kato]]

It may be same issue as https://source.chromium.org/chromium/chromium/src/+/942ed34df733303441ae36484433ab804f87bf52

Comment 7

[Makoto Kato [:m_kato]]

regression range: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=fda1bbc3a74afe6042ef5c5d9bc2ce2424c68926&tochange=1d1a02444b51b409294441c04e27c763cc3f97ff

Comment 8

[Jamie Nicol [:jnicol]]

Right, that makes much more sense than the GPU process, and indeed looks the same as the Chrome issue. Will investigate, thanks!

Comment 9

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

Set release status flags based on info from the regressing bug 1767128

Comment 10

[Chris Peterson [:cpeterson]]

Tracking for 105

No need for Lorand to bisect since Makoto found the regression.

Comment 11

[Jamie Nicol [:jnicol]]

Attachment: Bug 1780093 - Disable SurfaceControl compositing path when selection is active. r?#geckoview-reviewers

On Android we previously added a rendering path using the
SurfaceControl API, in order to work around an Android OS bug when
recovering from a GPU process crash. Unfortunately the Magnifier
widget (shown when moving a text selection caret) does not work when
rendering using SurfaceControl.

This patch makes it so that we temporarily disable the SurfaceControl
path when a text selection is active, allowing the Magnifier to work.
Unfortunately this means that if a GPU process crash occurs while
there is an active selection we will be unable to recover. Hopefully
this turns out to be a relatively rare occurence.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b9103141824
Disable SurfaceControl compositing path when selection is active. r=geckoview-reviewers,calu

Comment 13

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/6b9103141824

Comment 14

[Jamie Nicol [:jnicol]]

Reopening because in bug 1783542 we ended up effectively reverting this change, as the bug it caused was more severe. Will try to find another solution

Comment 15

[Chris Peterson [:cpeterson]]

(In reply to Jamie Nicol [:jnicol] from comment #14)

Reopening because in bug 1783542 we ended up effectively reverting this change, as the bug it caused was more severe. Will try to find another solution

In that case, I'll reset the status-firefox105 flag to affected.

Comment 16

[Jamie Nicol [:jnicol]]

Attachment: Bug 1780093 - Remove code for enabling/disabling SurfaceControl rendering path. r?#geckoview-reviewers

In order to fix the magnifier widget being broken, the previous patch
in this bug added a mechanism to disable and enable the SurfaceControl
rendering path. This caused some glitches to occur, so we removed the
calls to that code in bug 1783542, but the code remained.

As we now have an alternative solution to fix the magnifier widget, we
no longer require this code. This patch therefore reverts the original
patch, to lead the way for the new solution in the next patch.

Comment 17

[Jamie Nicol [:jnicol]]

Attachment: Bug 1780093 - Use custom SurfaceView which allows magnifier widget to work. r?#geckoview-reviewers

The android magnifier widget does not work when using our
SurfaceControl rendering path, as we no longer render in to the
Surface provided by the SurfaceView, but instead into a child Surface
we have created and attached the SurfaceControl.

To fix this, we create a SurfaceView subclass, MagnifiableSurfaceView,
which allows us to set an override Surface to be used by the
magnifier. This class works by overriding getHolder() to return a
custom SurfaceHolder, which returns our override Surface rather than
the default one when called by the Magnifier class.

Depends on D157308

Comment 18

[Chris Peterson [:cpeterson]]

107

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cba4d2198c60
Remove code for enabling/disabling SurfaceControl rendering path. r=geckoview-reviewers,owlish
https://hg.mozilla.org/integration/autoland/rev/29044dfc66e8
Use custom SurfaceView which allows magnifier widget to work. r=geckoview-reviewers,owlish

Comment 20

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/cba4d2198c60
https://hg.mozilla.org/mozilla-central/rev/29044dfc66e8

Comment 21

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

The patch landed in nightly and beta is affected.
:jnicol, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.Also, don't forget to request an uplift for the patches in the regression caused by this fix.
• If no, please set status-firefox106 to wontfix.

For more information, please visit auto_nag documentation.

Comment 22

[Jamie Nicol [:jnicol]]

I think we better let this one bake on nightly, it's a wee bit of a hack!