Closed Bug 1255555 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox for Android :: Text Selection, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 50
Tracking Status
firefox47 --- unaffected
firefox48 + verified
firefox49 + verified
fennec 48+ ---
firefox50 + verified

People

(Reporter: liuche, Assigned: kats)

References

(Blocks 1 open bug, )

Details

Attachments

(3 files)

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.
Is this a dupe of this: Bug 1252802 ?
No, this is something new ... looking further.
Blocks: gecko-carets
Blocks: text-selection-mvp
No longer blocks: gecko-carets
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
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?
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*)
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
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 (?)
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/
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/
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
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.
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
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/
tracking-fennec: --- → ?
I can reproduce this on my personal GS5 on Nightly, FF50 – we should take care of this before it hits release.
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?
Flags: needinfo?(markcapella)
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 :-)
Flags: needinfo?(markcapella)
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.
Attached file 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
Flags: needinfo?(tlin)
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
Flags: needinfo?(bugs)
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?
Flags: needinfo?(bugs)
That works beautifully, thanks!
Assignee: nobody → bugmail
Component: General → Text Selection
Nit, use EmptyString() and not NS_LITERAL_STRING("")
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 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.
Attachment #8768546 - Flags: review?(tlin) → review+
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
Flags: needinfo?(bugs)
for *native* anonymous element, yes.
Flags: needinfo?(bugs)
[Tracking Requested - why for this release]: This is a new feature we are shipping in 48 and this would be a noticeable regression
tracking-fennec: ? → 48+
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.
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
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 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/
Attachment #8768546 - Attachment description: Bug 1255555 - Ensure the dummy touchstart listener on the AccessibleCaret persists across clones. → Bug 1255555 - When a frame reconstruction triggers caret elements to be cloned, ensure the dummy touch listeners are re-registered on the clones.
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.
Attachment #8768546 - Flags: review+ → review?(tlin)
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.
Attachment #8768546 - Flags: review?(tlin) → review+
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.)
Yeah I verified that the mDummyTouchListeners get cleaned up appropriately.
Pushed by kgupta@mozilla.com:
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
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).
Whiteboard: [NO_UPLIFT]
https://hg.mozilla.org/mozilla-central/rev/509ef85e1007
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Nice :-D
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
Attachment #8768546 - Flags: approval-mozilla-beta?
Attachment #8768546 - Flags: approval-mozilla-aurora?
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.
Attachment #8768546 - Flags: approval-mozilla-beta?
Attachment #8768546 - Flags: approval-mozilla-beta+
Attachment #8768546 - Flags: approval-mozilla-aurora?
Attachment #8768546 - Flags: approval-mozilla-aurora+
This needs to be verified in 48 beta 8.
Flags: qe-verify+
Track this as this affects user experiences.
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'
Apparently the patch is not ok. See comment 41.
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.
Flags: needinfo?(bugmail)
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)
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.