Closed
Bug 1255555
Opened 9 years ago
Closed 8 years ago
Can't add text selection up or down because page scrolls
Categories
(Firefox for Android Graveyard :: Text Selection, defect)
Tracking
(firefox47 unaffected, firefox48+ verified, firefox49+ verified, fennec48+, firefox50+ verified)
VERIFIED
FIXED
Firefox 50
People
(Reporter: liuche, Assigned: kats)
References
()
Details
Attachments
(3 files)
(deleted),
video/mp4
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
text/x-review-board-request
|
TYLin
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
|
Details |
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•9 years ago
|
||
Is this a dupe of this: Bug 1252802 ?
Comment 2•9 years ago
|
||
No, this is something new ... looking further.
Reporter | ||
Updated•9 years ago
|
Blocks: gecko-carets
Reporter | ||
Updated•9 years ago
|
Comment 3•9 years ago
|
||
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
Assignee | ||
Comment 4•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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 (?)
Reporter | ||
Comment 8•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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/
Updated•8 years ago
|
tracking-fennec: --- → ?
Comment 14•8 years ago
|
||
I can reproduce this on my personal GS5 on Nightly, FF50 – we should take care of this before it hits release.
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 15•8 years ago
|
||
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)
Comment 17•8 years ago
|
||
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)
Assignee | ||
Comment 18•8 years ago
|
||
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.
Assignee | ||
Comment 19•8 years ago
|
||
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)
Assignee | ||
Comment 20•8 years ago
|
||
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)
Comment 21•8 years ago
|
||
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)
Assignee | ||
Comment 22•8 years ago
|
||
That works beautifully, thanks!
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bugmail
status-firefox49:
--- → affected
status-firefox50:
--- → affected
Component: General → Text Selection
Assignee | ||
Comment 23•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/62726/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62726/
Attachment #8768546 -
Flags: review?(tlin)
Assignee | ||
Updated•8 years ago
|
Comment 24•8 years ago
|
||
Nit, use EmptyString() and not NS_LITERAL_STRING("")
Assignee | ||
Comment 25•8 years ago
|
||
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/
Assignee | ||
Comment 26•8 years ago
|
||
^ updated with comment 24.
Comment 27•8 years ago
|
||
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+
Assignee | ||
Comment 28•8 years ago
|
||
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)
Comment 30•8 years ago
|
||
[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+
status-firefox47:
--- → unaffected
tracking-firefox48:
--- → ?
tracking-firefox49:
--- → ?
tracking-firefox50:
--- → ?
Assignee | ||
Comment 31•8 years ago
|
||
Assignee | ||
Comment 32•8 years ago
|
||
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.
Assignee | ||
Comment 33•8 years ago
|
||
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
Assignee | ||
Comment 34•8 years ago
|
||
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.
Assignee | ||
Comment 35•8 years ago
|
||
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.
Assignee | ||
Comment 36•8 years ago
|
||
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 37•8 years ago
|
||
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+
Comment 38•8 years ago
|
||
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.)
Assignee | ||
Comment 39•8 years ago
|
||
Yeah I verified that the mDummyTouchListeners get cleaned up appropriately.
Comment 40•8 years ago
|
||
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
Assignee | ||
Comment 41•8 years ago
|
||
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]
Comment 42•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Comment 43•8 years ago
|
||
Nice :-D
Assignee | ||
Comment 44•8 years ago
|
||
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 45•8 years ago
|
||
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+
Updated•8 years ago
|
Comment 47•8 years ago
|
||
Track this as this affects user experiences.
Comment 48•8 years ago
|
||
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•8 years ago
|
||
Apparently the patch is not ok. See comment 41.
Comment 50•8 years ago
|
||
bugherder uplift |
Comment 51•8 years ago
|
||
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=2997032&repo=mozilla-aurora
Flags: needinfo?(bugmail)
Assignee | ||
Comment 52•8 years ago
|
||
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)
Assignee | ||
Comment 53•8 years ago
|
||
Whiteboard: [NO_UPLIFT]
Comment 54•8 years ago
|
||
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)
Updated•6 years ago
|
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•