1484597 - ascii.cl - Cut and Paste broken

Status: VERIFIED FIXED

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

Description

[Karl Dubost💡 :karlcow]

1. Go to https://ascii.cl/htmlcodes.htm
2. Try select text in the latest column


Attempted moz-regression
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b62d582090e11edcdd01c2c707e0b07b33ad0b12&tochange=ecafdbe899a7b2e574922250e356f8610788ad75

Comment 1

[Botond Ballo [:botond]]

This does seem to be broken pretty badly. The text selection handles appear as expected after a long tap, but:

  - The Cut/Copy/Share popup sometimes appears in the wrong
    place or not at all (presumably it's somewhere offscreen)

  - Trying to drag one of the selection handles results in
    unexpected scrolling

I will investigate.

Comment 2

[Botond Ballo [:botond]]

I've been investigating the first issue (mispositioning of the Cut/Copy/Share popup, called the "floating toolbar" in the code).

This toolbar is positioned by the Android system, but the Android system queries the Firefox app for the bounds of the selection [1] and positions it close to (but not overlapping) the selection.

These bounds originate in the "bounding client rect" field of the 'mozcaretstatechanged' message [2] that AccessibleCaretManager dispatches when the text selection handles appear.

This rect is interpreted as being relative to the browser's content area (i.e. relative to the screen origin + browser / OS chrome offsets).

The value computed for the rect is relative to the ViewportFrame. Until bug 1423011, it used to be the case that the ViewportFrame origin corresponded to the origin of the browser's content area in most cases (*).  Bug 1423011 changed that, making the computed value of the rect different from what's expected if the visual viewport has scrolled away from the layout viewport origin.

(*) I say in most cases, because the ViewportFrame origin could be different from the origin of the browser's content area even before bug 1423011, on pages that are overflow:hidden and you've zoomed in. And indeed, the mispositioning occurs on such pages, even before bug 1423011.

[1] https://searchfox.org/mozilla-central/rev/37663bb87004167184de6f2afa6b05875eb0528e/mobile/android/base/java/org/mozilla/gecko/text/FloatingActionModeCallback.java#69
[2] https://searchfox.org/mozilla-central/rev/de7676288a78b70d2b9927c79493adbf294faad5/layout/base/AccessibleCaretManager.cpp#1450

Comment 3

[Botond Ballo [:botond]]

Attachment: Reduced testcase

Here is a reduced testcase that triggers the mispositioning of the floating toolbar.

Comment 4

[Botond Ballo [:botond]]

Attachment: Variation that's buggy even before bug 1423011

Here's a variation of the reduced testcase that exhibits the mispositioning of the floating toolbar even before bug 1423011.

Comment 5

[Botond Ballo [:botond]]

(Another way of describing the problem is that the code handling the 'mozcaretstatechanged' message is interpreting the bounding client rect as being relative to the visual viewport, while AccessibleCaretManager is passing in a rect relative to the layout viewport.)

Comment 6

[Botond Ballo [:botond]]

This patch fixes the caret mispositioning issue:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39894319f0497fad2ab72a3973268e5eead5fa52

The second issue (unexpected scrolling when dragging a selection caret) remains to be investigated.

Comment 7

[Botond Ballo [:botond]]

The second issue is related to the "scroll selection into view" operation that dragging a caret triggers after modifying the selection.

It can be reproduced on the same test page, as follows:

  * Zoom in the page
  * Scroll so that part of the text is offscreen
  * Select an onscreen word in the text
  * Try to extend the selection to include the offscreen
    part of the text by dragging one of the carets

Comment 8

[Botond Ballo [:botond]]

So part of the problem is that ScrollToShowRect() is performing its computation using a visible rect composed of the layout viewport offset and the visual viewport size [1], which no longer makes sense with bug 1423011 in place.

However, there seems to be a remaining issue related to the APZ callback transform as well, which I haven't quite pinned down yet.

[1] https://searchfox.org/mozilla-central/rev/99cbc0aec3e1c0b65ff9052523fb5c181b248f57/layout/base/PresShell.cpp#3452

Comment 9

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #8)
> However, there seems to be a remaining issue related to the APZ callback
> transform as well, which I haven't quite pinned down yet.

I think I understand this issue now. I will try to explain my understanding of it.

First, a bit of background:

  - The ability for the layout viewport origin to diverge from
    the visual viewport origin has been around for quite a
    while (it was added in bug 975962), although we didn't use
    those terms to describe it. However, it was limited to 
    cases when an overflow:hidden page was zoomed in.

  - Bug 1423011 extended this ability to any page when zoomed
    in.

  - The way this divergence is implemented is that for most
    purposes, the main thread is permitted to continue assuming
    that the two origins coincide. The difference between them
    is tracked by APZ, and APZ adjusts information it sends to
    the main thread (e.g., the coordinates of input events) to
    make the main thread's assumption valid.

    Notable among these adjustments is the "APZ callback 
    transform" added in bug 981029, which basically reflects
    the offset between the two viewports, and is applied to the
    coordinates of input events before being dispatched through
    main thread event handling code.

Now, the problem:

  - One assumption behind the APZ callback transform is that
    the offset between the two viewports is only changed by APZ.
    Since APZ updates the callback transform when it makes such
    a change, the callback transform will always be up to date
    at the time it's queried.

  - However, this assumption is not always valid. When the main
    thread originates a scroll offset update for the RCD-RSF, 
    the effect is that APZ resets the visual viewport offset to 
    match the new layout viewport offset, thereby collapsing any
    difference between the two. This is duly followed up by an
    update to the APZ callback transform to reflect the new lack
    of difference.

  - If, however, the main thread queries the callback transform
    after it has changed its scroll offset, but before APZ has
    updated the callback transform, and it combines the old
    callback transform (reflecting a nonzero offset between the
    viewports) with its new scroll offset (where the origins of
    the viewports now coincide), it can come to incorrect
    conclusions about the position of the layout viewport offset
    relative to the screen.

My proposed solution is for the main thread to clear the APZ callback transform when it originates a scroll offset update. We know APZ will end up clearing it, but it's important for the new (zero) value to be there from the get-go.

With this change, together with the changes from comment 6 and comment 8, the page here (both the reduced testcase and the original) behave well for me.

Comment 10

[Botond Ballo [:botond]]

Attachment: Bug 1484597 - Expose visual viewport information more conveniently in Layout. r?mstange

Comment 11

[Botond Ballo [:botond]]

Attachment: Bug 1484597 - Keep the bounding client rect field of the 'mozcaretstatechanged' event relative to the visual viewport. r?mstange

Depends on D6074

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1484597 - Use the visual viewport offset in ScrollToShowRect(). r?mstange

Depends on D6075

Comment 13

[Botond Ballo [:botond]]

Attachment: Bug 1484597 - Clear the APZ callback transform when originating a main thread scroll offset update for the RCD-RSF. r?kats

Depends on D6076

Comment 14

[Botond Ballo [:botond]]

The first patch contains no functionality changes. The second and third patches are the fixes for the issues described in comment 6, and comment 8, respectively - they seem fairly straightforward.

The last patch is the fix for the issue described in comment 9. I'm less certain about this fix than the others, as it touches the tricky scroll position syncing code. That said, it does seem to fix this bug for me.

Comment 15

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

Comment on attachment 9009743 [details]
Bug 1484597 - Clear the APZ callback transform when originating a main thread scroll offset update for the RCD-RSF. r?kats

Kartikaya Gupta (email:[e-mail]) (parental leave) has approved the revision.

Comment 16

[Markus Stange [:mstange]]

Comment on attachment 9009740 [details]
Bug 1484597 - Expose visual viewport information more conveniently in Layout. r?mstange

Markus Stange [:mstange] has approved the revision.

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 9009741 [details]
Bug 1484597 - Keep the bounding client rect field of the 'mozcaretstatechanged' event relative to the visual viewport. r?mstange

Markus Stange [:mstange] has approved the revision.

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 9009742 [details]
Bug 1484597 - Use the visual viewport offset in ScrollToShowRect(). r?mstange

Markus Stange [:mstange] has approved the revision.

Comment 19

[Botond Ballo [:botond]]

Attachment: Bug 1484597 debugging

Depends on D6549

Comment 20

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

Comment on attachment 9009741 [details]
Bug 1484597 - Keep the bounding client rect field of the 'mozcaretstatechanged' event relative to the visual viewport. r?mstange

:Nika Layzell has approved the revision.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/262dc187bc8e
Expose visual viewport information more conveniently in Layout. r=mstange
https://hg.mozilla.org/integration/autoland/rev/8b5b04264da1
Keep the bounding client rect field of the 'mozcaretstatechanged' event relative to the visual viewport. r=mstange,nika
https://hg.mozilla.org/integration/autoland/rev/595b2c7f7d85
Use the visual viewport offset in ScrollToShowRect(). r=mstange
https://hg.mozilla.org/integration/autoland/rev/bc99ca10fe66
Clear the APZ callback transform when originating a main thread scroll offset update for the RCD-RSF. r=kats

Comment 22

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/262dc187bc8e
https://hg.mozilla.org/mozilla-central/rev/8b5b04264da1
https://hg.mozilla.org/mozilla-central/rev/595b2c7f7d85
https://hg.mozilla.org/mozilla-central/rev/bc99ca10fe66

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Is it possible to land an automated test for this?

Comment 24

[Botond Ballo [:botond]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #23)
> Is it possible to land an automated test for this?

Yes, one is planned in bug 1492194.

Comment 25

[Botond Ballo [:botond]]

Comment on attachment 9009740 [details]
Bug 1484597 - Expose visual viewport information more conveniently in Layout. r?mstange

Approval Request Comment
[Feature/Bug causing the regression]:
  Bug 1423011

[User impact if declined]:
  When zoomed in, text selection can be impaired in two ways:

    * The floating toolbar (with the Cut/Copy/Share commands)
      may not be shown

    * Dragging the text selection handles can result in
      unexpected scrolling, possibly to far away from the
      text

[Is this code covered by automated tests?]:
  The code being touched has some test coverage.
  This specific scenario does not have an automated test,
  although one is planned in bug 1490102.

[Has the fix been verified in Nightly?]:
  Yes, by me.

[Needs manual test from QE? If yes, steps to reproduce]: 
  No.

[List of other uplifts needed for the feature/fix]:
  No.

[Is the change risky?]:
  Moderately risky.

[Why is the change risky/not risky?]:
  The last patch touches code that has historically proven
  to be fairly tricky.
  The code touched by the third patch could also use some 
  more test coverage in zoomed-in scenarios.

[String changes made/needed]:
  None.

Comment 26

[Botond Ballo [:botond]]

Note: the approval request above applies to all 4 patches together.

Comment 27

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #25)
> [Is this code covered by automated tests?]:
>   The code being touched has some test coverage.
>   This specific scenario does not have an automated test,
>   although one is planned in bug 1490102.

Correction: that should be bug 1492194.

Comment 28

[Pascal Chevrel:pascalc]

Comment on attachment 9009740 [details]
Bug 1484597 - Expose visual viewport information more conveniently in Layout. r?mstange

Needed for bug 1492288 which is  a P2 and has been on nightly for several days without reported regressions, , uplift accepted for 63 beta 11, thanks.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3b9941572ef1
https://hg.mozilla.org/releases/mozilla-beta/rev/f22d1ffd83bc
https://hg.mozilla.org/releases/mozilla-beta/rev/fae09b7d51ad
https://hg.mozilla.org/releases/mozilla-beta/rev/eaf11802a534

Comment 30

[u554753]

Verified with the following specs:

Windows x10 x64, Mac OS X 10.11, Ubuntu 18.10 x64
Version: 64.0a1
Build: 20181003100127

Version: 63.0b11
Build: 20181001131022