1255555 - Can't add text selection up or down because page scrolls

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[Chenxia Liu [:liuche]]

Attachment: Video: Scrolling instead of highlighting

On Nightly 3/10, I'm unable to move the selection carets up or down to select lines above or below a selected word for several news sites, such as vox.com and theatlantic.com. support.mozilla.org.

I'm seeing this on N7, N5, and a Moto X.

Comment 1

[Randall Barker [:rbarker]]

Is this a dupe of this: Bug 1252802 ?

Comment 2

[Mark Capella [:capella]]

No, this is something new ... looking further.

Comment 3

[Mark Capella [:capella]]

Using testcases:

Normal Page that WORKS :
   https://www.lawfareblog.com/aclu-takes-first-step-towards-prepublication-review-reform

Interesting Page that FAILS / exhibits this issue : 
   http://www.vox.com/2016/3/15/11243056/anita-alvarez-kim-foxx-cook-county-prosecutor

The only quick thing I notice where the code paths diverge, is for the FAIL pages, during initial MotionEvent.ACTION_DOWN on caret drag, there's a difference in behaviour in InputQueue::MaybeRequestContentResponse() ... pages that FAIL, have TargetConfirmed(), route through [0] and don't trigger a  ScheduleMainThreadTimeout().

Additionally for the FAILS, AccessibleCaretManager is indeed seeing: OnScrollStart/End/PositionChanged events ...

The actual scroll events originate in AsyncPanZoomController::RequestContentRepaint() -> ... -> APZCCallbackHelper::ScrollFrameTo() -> ScrollFrameHelper::ScrollToWithOrigin() ... (Instant Scroll's) from [1]

cc:ing kats as this is deeper than I understand atm into APZ


[0] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/InputQueue.cpp?rev=fd968c710a39&mark=414-414#405
[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp?rev=8f609479ba37&mark=2181-2186#2151

Comment 4

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

Yeah it sounds like for the FAIL pages APZ isn't waiting for touch listeners, because it doesn't think there are any at the touch-down point. Does the caret register a touch listener?

Comment 5

[Mark Capella [:capella]]

Yes, there's a DummyTouchListener patterned after the one in nsRangeFrame, for "touchstart".

I've just started chatting with TYLin about it's design, as I don't understand how it's to work w/o doing any preventDefaults, and more importantly why logging I add to the HandleEvent() method doesn't show any events (neither "touchstart" nor others I add) flowing through it ever.

I've also had a brief discussion with noit in irc regarding failures of anonymous content targets of eventListeners, related to style attributes, that he in turn, previously discussed with nmaiers. (May be a long shot, but *meh*)

Comment 6

[Mark Capella [:capella]]

Status update, in the case of Vox, it seems there's a <script> they include [0] that causes this.

Pull the <script>, page works fine.

[0] https://cdn0.vox-cdn.com/javascripts/vox_head.v940ffb85b3a4f04b.js

Comment 7

[Mark Capella [:capella]]

Additionally, I don't see this in theatlantic.com or support.mozilla.org as mentioned above. The hit box for the carets is tight, so you may be fat-fingering and getting a page-scroll, vs a target-drag (?)

Comment 8

[Chenxia Liu [:liuche]]

I'm not seeing this on SUMO, but I am still seeing it on any Atlantic article. It's definitely not fat-fingering, because the carets are changing their selection left-right while the page is scrolling. Try this article? http://www.theatlantic.com/magazine/archive/2016/04/the-obama-doctrine/471525/

Comment 9

[Mark Capella [:capella]]

Ah, thanks! That one does fail, and I'll look further. I was poking around earlier at [0], which seems to function normally.

[0] http://www.theatlantic.com/notes/2016/03/what-were-following-this-morning/475785/

Comment 10

[Mark Capella [:capella]]

Similar issue as mentioned in comment #6, CDN <script> named [0] ... remove script, issue gone.

:-/

[0] http://cdn.theatlantic.com/assets/static/b/theatlantic/js/head.min.29383a723a06.js

Comment 11

[Mark Capella [:capella]]

The code employed that passes along scroll events is different for both scripts, but probably designed for a similar (unknown to me) reason.

In any case, I find on both sites, the issue is more pronounced if you try to tap-and-drag the caret quickly.

If you tap-and-firmly-hold the caret briefly, then dragging seems to work properly for me on both.

Comment 12

[bull500]

Hey

This also happens on Rap Genius with the latest Nightly (25-04-2016)  
http://genius.com/Kanye-west-ultralight-beam-lyrics#note-8663047
Try selecting the text below the lyrics. 
Its starts scrolling when you try to select more lines. 

One way to select more lines is to correctly place finger on the selection caret and give it a hard scroll down.  

Note-less link - http://genius.com/Kanye-west-ultralight-beam-lyrics

Comment 13

[Mark Capella [:capella]]

Right, I see this testing on my phone 5x device. I'm longPress selecting the word "strong" in the phrase "This track establishes strong parallels for The Life of Pablo".

This one is caused by interactions from the page including the /New Relic/ "Performance monitoring platform" script ... [0]

As seen on other unrelated pages, and mentioned in comment #11, If you tap-and-firmly-hold the caret briefly, dragging then seems to work properly

[0] http://newrelic.com/

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I can reproduce this on my personal GS5 on Nightly, FF50 – we should take care of this before it hits release.

Comment 15

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

I don't understand how the text selection code is working at all. I added some printf_stderr calls in the DummyTouchListener code in AccessibleCaret and they're not getting hit at all. Are we sure the AccessibleCaret code is the one being used? In about:config I do see layout.accesssiblecaret.enabled set to true but maybe it's not taking effect somehow?

Comment 16

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

^

Comment 17

[Mark Capella [:capella]]

I saw this with DummyTouchListene also, back in comment 5, but never explained it after I realized the scripts on the pages were involved, and started digging in there.

Was about to ?NI ping over to tylin, but you just added :-)

Comment 18

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

Ok, so it looks like AccessibleCaretEventHub has the code responsible for actually dealing with the events and manipulating the caret. That all looks ok. The only that needs to be done is to register a dummy listener properly. I do actually see the DummyTouchListener code getting fired, a lot, but only during page load. I don't know what the AccessibleCaret is or what it's used for, but it doesn't seem to represent an actual selection caret because there's a billion of them getting created and destroyed during page load and that doesn't seem right.

Right now as far as I can tell, the only problem is that APZ isn't waiting for the AccessibleCaretEventHub to handle the events and determine if they are preventDefault or not. The behaviour in comment 11 ("long-tap on the caret makes it work properly") is because APZ gets a preventDefault on the contextmenu event, and so then stops scrolling. That's basically accidental. We need to have the AccessibleCaret properly register a touch listener on the caret areas when the carets are visible, and then everything should work just fine.

Comment 19

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

Attachment: Backtrace to cloneNode

Ok, so I dug into this. It looks like most of the code is actually fine. The billions of carets that I saw getting created and destroyed were because the vox page creates a ton of iframes, each of which gets its own pair of AccessibleCaret instances. The normal flow is:
- AccessibleEventHub is created, which creates a pair of AccessibleCaret instances
- Each AccessibleCaret instance creates a caret element [1] and inserts it as anonymous content into the tree [2]. (This clones the element)
- Then the dummy touch listener is registered on the anonymous content [3]

The problem is that on the vox page, I see the anonymous content getting cloned *again* at some point during page load. When it gets cloned, the touch listener is not carried over to the new clone, and so it's as if it was never there. I stuck a breakpoint in the nsINode::CloneNode function to see what was triggering it, and I got the attached backtrace. Basically, the page is doing something that causes a frame reconstruction of the entire document, and that results in cloning of the anonymous content as well [4].

[1] http://searchfox.org/mozilla-central/rev/ffcc65db736dd64dc3cbc49592f9edac77bf65ad/layout/base/AccessibleCaret.cpp#208
[2] http://searchfox.org/mozilla-central/rev/ffcc65db736dd64dc3cbc49592f9edac77bf65ad/layout/base/AccessibleCaret.cpp#195
[3] http://searchfox.org/mozilla-central/rev/ffcc65db736dd64dc3cbc49592f9edac77bf65ad/layout/base/AccessibleCaret.cpp#203
[4] http://searchfox.org/mozilla-central/rev/ffcc65db736dd64dc3cbc49592f9edac77bf65ad/layout/generic/nsCanvasFrame.cpp#147

Comment 20

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

smaug, quick summary here is that we are creating anonymous elements in the DOM for the touch selection carets. We also register a dummy touchstart listener on them so that they become "APZ aware" and APZ knows to wait for the main-thread to handle touch events on them. However, the webpage is doing something that triggers a frame reconstruction, and so the anonymous elements get cloned. This causes the dummy touchstart listeners to get lost, and stuff breaks. Do you know if there's a good way to solve this problem?

I don't know if there's a way to ensure the listeners are copied over to the new anonymous content nodes, or if there's some way to register a callback for when nodes are cloned so that we can re-register the listener. The comment I found at [1] seems to imply there's no easy way to do this?

[1] http://searchfox.org/mozilla-central/rev/ffcc65db736dd64dc3cbc49592f9edac77bf65ad/toolkit/modules/FinderHighlighter.jsm#714

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

anonymous elements get cloned?

Where do we add the dummy listener? Could one use a dummy event handler (ontouchstart attribute) to trigger registration whenever there are relevant elements alive?

Comment 22

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

That works beautifully, thanks!

Comment 23

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

Attachment: Bug 1255555 - When a frame reconstruction triggers caret elements to be cloned, ensure the dummy touch listeners are re-registered on the clones.

Review commit: https://reviewboard.mozilla.org/r/62726/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62726/

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Nit, use EmptyString() and not NS_LITERAL_STRING("")

Comment 25

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

Comment on attachment 8768546 [details]
Bug 1255555 - When a frame reconstruction triggers caret elements to be cloned, ensure the dummy touch listeners are re-registered on the clones.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62726/diff/1-2/

Comment 26

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

^ updated with comment 24.

Comment 27

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8768546 [details]
Bug 1255555 - When a frame reconstruction triggers caret elements to be cloned, ensure the dummy touch listeners are re-registered on the clones.

https://reviewboard.mozilla.org/r/62726/#review59644

kats, thank you for digging into this isuue! This looks good to me.

Comment 28

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

The try push is showing a few failures in dom/security/test/csp/ tests [1], I think because the ontouchstart attribute on the div is triggering the CSP before the script that the test uses even gets to run.

From what I can tell by looking at the code, this is actually a problem because if the CSP is in effect, it will prevent the listener from ever getting registered [2], and so the "dummy touch listener" will not have the desired effect of making the anonymous content element APZ-aware.

smaug, do you know if it's ok to skip the CSP check on anonymous elements?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=7123a276def2&selectedJob=23451714
[2] http://searchfox.org/mozilla-central/rev/a7c8e9f3cc323fd707659175a46826ad12899cd1/dom/events/EventListenerManager.cpp#861

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

for *native* anonymous element, yes.

Comment 30

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: This is a new feature we are shipping in 48 and this would be a noticeable regression

Comment 31

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69154876f642&group_state=expanded

Comment 32

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

I've been having trouble getting this working, because it looks like when a node is cloned it loses the "is native anonymous" flag, so it still ends up violating the CSP check. I can work around it in the AccessibleCaret::CreateCaretElement cloning step (when it gets inserted as anonymous content) but if a frame reconstruction happens later, it could end up violating CSP there.

And it doesn't look safe to propagate the "is native anonymous" flag to arbitrary clones, either.

Comment 33

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

After a discussion with smaug we came up with a couple of options to explore. One (which seems safer to me) is to take advantage of the init call at [1] to re-register the dummy touch listener on the cloned elements, after the frame reconstruction happens.

The other is to somehow use a script runner to delay the adding of the attribute until after the anonymous flag is set on the clones [2]. I have a harder time picturing how this will work though - if we add the ontouchstart attribute once, it will get copied into any subsequent clones, and trip up the CSP check. We could make the carets work, but I think we'd still get spurious CSP warnings on pages that are affected by it.

Anyhow I implemented the first thing, try push at [3]. It seems to work locally.

[1] http://searchfox.org/mozilla-central/rev/f43c9e0ffa92e72dbdbcbf57eecf04a43d46da63/layout/generic/nsCanvasFrame.cpp#115
[2] http://searchfox.org/mozilla-central/rev/f43c9e0ffa92e72dbdbcbf57eecf04a43d46da63/layout/base/nsCSSFrameConstructor.cpp#4217,4234
[3 https://treeherder.mozilla.org/#/jobs?repo=try&revision=624c044be54c

Comment 34

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

Above try push was busted, needed rebasing on m-c. New try push with the rebase is green, at https://treeherder.mozilla.org/#/jobs?repo=try&revision=67693f1418cb&group_state=expanded

I'll put the new patch up for review.

Comment 35

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

Comment on attachment 8768546 [details]
Bug 1255555 - When a frame reconstruction triggers caret elements to be cloned, ensure the dummy touch listeners are re-registered on the clones.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62726/diff/2-3/

Comment 36

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

Comment on attachment 8768546 [details]
Bug 1255555 - When a frame reconstruction triggers caret elements to be cloned, ensure the dummy touch listeners are re-registered on the clones.

Re-flagging for review as the patch has changed significantly from the one previously reviewed.

Comment 37

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8768546 [details]
Bug 1255555 - When a frame reconstruction triggers caret elements to be cloned, ensure the dummy touch listeners are re-registered on the clones.

https://reviewboard.mozilla.org/r/62726/#review60230

Looks good to me.  However, I do have a question.  When the anonymous element gets cloned, will the mDummyTouchListener be removed properly from the old element?  We have to ensure the ref-count of mDummyTouchListener is correct.  If not, the current code might already get the ref-count wrong when the frame reconstruction happends because the RemoveEventListener() in AccessibleCaret::RemoveCaretElement() is called upon the new element.

::: layout/base/AccessibleCaret.cpp:198
(Diff revision 3)
> +  // If the caret element was cloned, the listener might have been lost. So
> +  // if that's the case we register a dummy listener if there isn't one on
> +  // the element already.
> +  if (!CaretElement()->IsApzAware()) {
> +    CaretElement()->AddEventListener(NS_LITERAL_STRING("touchstart"),
> +                                    mDummyTouchListener, false);

Nit: Please correct mDummyTouchListener indentation.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Adding an event listener to an event target addrefs the listener (target's EventListenerManager keeps a strong reference to the listeners)
Listeners are automatically removed when the target is about to die. 
(And DummyListener doesn't need to be cycle collectable since it doesn't participate in any cycle.)

Comment 39

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

Yeah I verified that the mDummyTouchListeners get cleaned up appropriately.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/509ef85e1007
When a frame reconstruction triggers caret elements to be cloned, ensure the dummy touch listeners are re-registered on the clones. r=tylin

Comment 41

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

This will need to be rebased for uplift to 49 and 48, because IsApzAware() doesn't exist on those branches, we have to use HasApzAwareListeners() instead. Flagging as NO_UPLIFT so that I don't forget to do this during uplift (and so that the sheriffs don't auto-uplift the unrebased version after I request approval).

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/509ef85e1007

Comment 43

[Mark Capella [:capella]]

Nice :-D

Comment 44

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

Comment on attachment 8768546 [details]
Bug 1255555 - When a frame reconstruction triggers caret elements to be cloned, ensure the dummy touch listeners are re-registered on the clones.

Approval Request Comment
[Feature/regressing bug #]: new selection carets in 48
[User impact if declined]: on some pages, trying to drag the selection carets scrolls the page instead
[Describe test coverage new/current, TreeHerder]: tested locally
[Risks and why]: should be pretty low-risk, relatively small patch. there might be interactions with other DOM stuff that I'm not aware of and that might cause the patch to not be 100% effective but I don't anticipate new regressions from this.
[String/UUID change made/needed]: none

Comment 45

[Gerry Chang [:gchang]]

Comment on attachment 8768546 [details]
Bug 1255555 - When a frame reconstruction triggers caret elements to be cloned, ensure the dummy touch listeners are re-registered on the clones.

This patch fixes a regression. Take it in 48 beta 8 and aurora. This should be fixed in fennec 48 beta 8.

Comment 46

[Gerry Chang [:gchang]]

This needs to be verified in 48 beta 8.

Comment 47

[Gerry Chang [:gchang]]

Track this as this affects user experiences.

Comment 48

[Frank-Rainer Grahl (:frg)]

Is the patch for m-a ok. I am now hitting an error compiling suite in c-a:

>> mozmake[4]: Leaving directory 'c:/seabuild/release/comm-aurora-15/obj-x86_64-pc-mingw32/mailnews/mime/cthandlers/vcard'
>> c:/seamonkey/comm-aurora/mozilla/layout/base/AccessibleCaret.cpp(196): error C2039: 
>> 'IsApzAware': is not a member of 'mozilla::dom::Element'
>> c:\seabuild\release\comm-aurora-15\obj-x86_64-pc-mingw32\dist\include
>> \mozilla/StyleAnimationValue.h(31): note: see declaration of 'mozilla::dom::Element'

Comment 49

[Frank-Rainer Grahl (:frg)]

Apparently the patch is not ok. See comment 41.

Comment 50

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/de766fceaddc

Comment 51

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=2997032&repo=mozilla-aurora

Comment 52

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

The bustage was expected, see comment 41. Landed with the necessary changes made:

https://hg.mozilla.org/releases/mozilla-aurora/rev/385340f31f4d

I'll wait for that to go green before uplifting to beta, just in case.

Comment 53

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

https://hg.mozilla.org/releases/mozilla-beta/rev/1748a233dae0

Comment 54

[u549602]

Verified as fixed on all branches(48.0b9, Aurora 49.0a2/2016-07-18 and Nightly 50.0a1/2016-07-18) on a Samsung Galaxy S6 Edge (Android 6.0.1) and on a Samsung Galaxy Tab S2(Android 5.0.2)