1306634 - Long-pressing on an iframe which doesn't have focus puts selection in unexpected place

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

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

On Fennec, or Windows e10s with touch:

1. Load a page like http://people.mozilla.org/~kgupta/gridframe.html which has an iframe
2. Tap outside the iframe to make sure the parent window has focus
3. Long-press inside the iframe (on some blank space) to bring up the text-selection carets

ER: text selection carets show up inside the iframe

AR: text selection carets show up outside the iframe

Note that in step 2 if you tap inside the iframe (on blank space) then the iframe gets focus and things work as expected. I believe what we need to do is give the iframe focus once we detect the long-press. Chrome seems to do this as well.

Comment 1

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

I thought this would be fixable by simply copying the code at [1] down into the eLongTap case, but that doesn't work.

After looking at the code some more I then thought that the problem was the AccessibleCaretEventHub schedules its internal long-tap injector when it receives the touchstart, but at that point the focus is still in the parent window. So the long-tap injector triggers a long-tap on the parent window, regardless of whether the focus changes later. But disabling the long-tap injector and using the eMouseLongTap event didn't fix the problem either. Still digging...

[1] http://searchfox.org/mozilla-central/rev/d1276b5b84e6cf7991c8e640b5e0ffffd54575a6/dom/ipc/TabChild.cpp#1781-1783

Comment 2

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

I think it might be related to how the EventHub is delivered events from nsPresShell. It looks like it always sends it from the focused presShell, or the top-level presShell. In this case we want it to go to the presShell that's under the finger which is only computed later in the code. I haven't yet confirmed.

Comment 3

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

Attachment: Hack

Indeed, that is the problem. A patch like the one attached, which passes positioned events to the AccessibleCaretEventHub of the presshell *after* hit-testing fixes this bug. It probably breaks other things, though. I'm not sure what the correct mechanism here is to fix this.

Comment 4

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

Unassigning since I'm not familiar enough with the intricacies of the AccessibleCaret event delivery timing to really tackle this.

Comment 5

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

On Windows this will affect 52 and up, but on Fennec it is already in release (48 and up).

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

I feel the patch in comment 3 makes some sense. However, after applying the patch, sometimes I cannot drag caret. 

Each PresShell owns an EventHub, and we should want to let the event hub under the finger to handle the event.  The event retargeting code [1] was from the very first implementation in bug 924692.  Morris, do you recall why we did [1]?

[1] http://searchfox.org/mozilla-central/rev/d1276b5b84e6cf7991c8e640b5e0ffffd54575a6/layout/base/nsPresShell.cpp#7341-7347

Comment 8

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

That because we want to active only one caret at the same time. Otherwise it might have multiple caret appeared on the screen.

Comment 9

[Julien Cristau [:jcristau]]

Mass wontfix for bugs affecting firefox 52.

Comment 11

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1306634 Part 1 - Handle a long press to select a word in an unfocused iframe.

Long-pressing on a text in an unfocused iframe to select a word never
works. Currently, you need to single tap to focus the iframe first.

Each PresShell has an associated AccessibleCaretEventHub. This patch
fixes this bug by routing the event to the AccessibleCaretEventHub under
the event point, and handle it there. If the event is not handled, then
we handle it by the focused AccessibleCaretEventHub as before.

I've experimented with only routing the event to the
AccessibleCaretEventHub under the event point, without routing to the
fallback focused AccessibleCaretEventHub. However, caret dragging didn't
work in iframes. I didn't debug further.

Comment 12

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1306634 Part 2 - Do not show carets if the window is not focused. r=mats

Consider the following scenario:

1. Select a word in an iframe.
2. Select a word in the parent document. (The carets in iframe hide
   due to blur, but the selection is still there.)
3. Scroll the parent document.

The selection in the iframe (made in step 1) has a non-collapsed range.
Thus the carets show again after scrolling due to UpdateCarets()
called in the end of AccessibleCaretManager::OnScrollEnd().

To fix the dual AccessibleCaret in the same page (described in bug
1199967), we can simply show no carets if the window is not focused.
This behavior always matches Google Chrome on Android.

Depends on D52767

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/35651fd9e240
Part 1 - Handle a long press to select a word in an unfocused iframe. r=mats,marionette-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/fb20602d0c39
Part 2 - Do not show carets if the window is not focused. r=mats

Comment 14

[Noemi Erli[:noemi_erli]]

Backed out 2 changesets (Bug 1306634) for causing assertion failures in nsAutoLayoutPhase.cpp CLOSED TREE

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=285253547&resultStatus=testfailed%2Cbusted%2Cexception&revision=fb20602d0c39d727109ee19cf7d609ac908c2314

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=285263243&repo=autoland&lineNumber=37064

Backout: https://hg.mozilla.org/integration/autoland/rev/5dd4c1288dc7eb4dc2ab12e522043613c7e79f8f

Comment 15

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

OK, so by looking at the failure log, the assertion is "display list building in the middle of frame construction."

That is, we are in the middle of destroying some frames, and some windows-only related stuff [1] can send WidgetQueryContentEvent with eQuerySelectedText messages that PresShell needs to handle. My new code added in Part 1 uses PresShell::EventHandler::ComputeEventTargetFrameAndPresShellAtEventPoint() to find the desired target frame and PresShell, which requires building display lists to do the hit test.

In fact, AccessibleCaretEventHub [2] handles only mouse, touch, and keyboard events. To fix this, I think it is reasonable to filter the event classes upfront in PresShell::EventHandler::MaybeHandleEventWithAccessibleCaret(), and add an assertion in AccessibleCaretEventHub::HandleEvent() that it never gets events other than the three types it cares about.

[1] https://searchfox.org/mozilla-central/rev/68b2e0fd4323261a229233ec2ab8606228979141/widget/windows/TSFTextStore.cpp#2977-2979
[2] https://searchfox.org/mozilla-central/rev/68b2e0fd4323261a229233ec2ab8606228979141/layout/base/AccessibleCaretEventHub.cpp#405

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ad87a37592b
Part 1 - Handle a long press to select a word in an unfocused iframe. r=mats,marionette-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/29c9a00754c1
Part 2 - Do not show carets if the window is not focused. r=mats

Comment 17

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/3ad87a37592b
https://hg.mozilla.org/mozilla-central/rev/29c9a00754c1