Closed Bug 1310509 Opened 3 years ago Closed 3 years ago

Opening new tab when text is selected does not properly dismiss the selection menu

Categories

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

49 Branch
All
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 52
Tracking Status
firefox49 --- affected
fennec 52+ ---
firefox50 --- affected
firefox51 --- affected
firefox52 --- verified

People

(Reporter: cranberrydoughan, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [TPE-1])

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160920074044

Steps to reproduce:

1.) Press and hold to select some text; this pops up the text selection menu (copy, share, select all).
2.) Without dismissing the text selection menu, press and hold a link to open up the new tab options.
3.) Select Open Link in New Tab or Open Link in Private Tab.
4.) Switch to the new tab.
5.) Scroll down slightly. 


Actual results:

The text selection menu, now only showing "select all" appears. It cannot properly be dismissed (if the back button is pressed, it goes away, but then reappears as soon as the user scrolls again). 


Expected results:

No text selection menu should appear in the new tab.
OS: Unspecified → Android
Hardware: Unspecified → ARM
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
Priority: -- → P2
Hardware: ARM → All
Whiteboard: [TPE-1]
tracking-fennec: ? → 52+
Flags: needinfo?(tlin)
Component: General → Text Selection
If the carets are dismiss due to blur event but the selection highlight is still there, scrolling the content a bit will bring the carets back by [1]. I'll see what I can do.

[1] https://dxr.mozilla.org/mozilla-central/rev/f0f1aaf051d6798e1e73d1feee07ca847333167a/layout/base/AccessibleCaretManager.cpp#703
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Flags: needinfo?(tlin)
I don't quite follow why this bug occurs.  Do we get an OnScrollEnd on the old document
or the new document?  If it's the new document (which I assume), why do we even have
active selection carets there if no selection has occurred in the new tab?
Flags: needinfo?(tlin)
Comment on attachment 8803282 [details]
Bug 1310509 - Call NotifyAsyncPanZoomStarted/Stopped on document containing current scrolling content.

Cancelling the review until the questions in comment 4 are addressed.
Attachment #8803282 - Flags: review?(mats)
(In reply to Mats Palmgren (:mats) from comment #4)
> I don't quite follow why this bug occurs.  Do we get an OnScrollEnd on the old document
> or the new document?  If it's the new document (which I assume), why do we even have
> active selection carets there if no selection has occurred in the new tab?

In adb logcat, I find that we get OnScollEnd() called on every PresShell. In this bug, it's old document in the background tab which still have a selection that fires the CaretStateChanged event which makes the floating text selection menu shown. The new document doesn't have any selection or carets. 

I don't know the details that the selection is going to be cleared or not when switching tabs. The selection is often cleared when I click the number on the top right corner to switch tab. However if I switch tab via the prompt "New tab opened SWITCH", it doesn't clear the selection. 

The solution I propose could also fix bug 1171843 which has the similar scenario but it's in the same document. That's why I think we should fix the AccessibleCaret by not bring the carets back when they're hidden by previous blur event. (And sorry for the delayed response.)
Flags: needinfo?(tlin) → needinfo?(mats)
OK, so switching tab blurs the document in the current tab, and that blur event
triggers OnScrollEnd(?) which hides the carets?

And the tab you switch to gets a focus event on its document which is supposed
to restore the carets?

Perhaps the problem is that these events does not occur in the expected order?
Or, are not fired at all?
If so, we should probably look into that first, since it seems quite important
in general that we get these events in a reliable order.

Anyway, personally I do think that we should restore the carets when switching
back to a tab that had a selection.  Switching between tabs shouldn't be
"destructive" to the selection state IMO.
Flags: needinfo?(mats)
BTW, it seems layout.accessiblecaret.enabled=true isn't working to enable
the carets in my local Linux debug build anymore.  Is there some other
pref I need to set as well?
Re comment 7:

> OK, so switching tab blurs the document in the current tab, and that blur event
> triggers OnScrollEnd(?) which hides the carets?

The blur event hides the carets in the old tab (step 4 in comment 0), and the subsequent scroll-down (step 5) on the new tab triggers the OnScrollEnd() in the old tab, which dispatches the CaretStateChanged event that makes the floating toolbar shown.

> And the tab you switch to gets a focus event on its document which is supposed
> to restore the carets?

The new tab doesn't have a new selection unless the user long taps on something.

> Perhaps the problem is that these events does not occur in the expected order?
> Or, are not fired at all?

I think the event order is not the problem. The problem is if a selection doesn't have carets accompanied due to the previous blur, do we want to restore carets via scrolling in OnScrollEnd()? 

> Anyway, personally I do think that we should restore the carets when switching
> back to a tab that had a selection.  Switching between tabs shouldn't be
> "destructive" to the selection state IMO.

I totally agree! Ideally, if the selection is *visible* to the user (not in the background tab or covered by other dom element like the Wikipedia in bug 1171843), we should update the carets. However, I had no idea how to determine whether the selection is visible or not. It's also possible that only part of the selection highlight is covered. Things get tricky. It's doable to hide the carets on blur event, and not bring them back in any circumstances except the user long tap on an existing selection or on a new word.


Re comment 8:
> BTW, it seems layout.accessiblecaret.enabled=true isn't working to enable
> the carets in my local Linux debug build anymore.  Is there some other
> pref I need to set as well?

Oh. You'll need to turn off "layout.accessiblecaret.hide_carets_for_mouse_input". On Linux desktop, you might want to try the STR in bug 1171843 comment 0. And "layout.accessiblecaret.use_long_tap_injector" was default to false now, you'll need to use double-click or drag to make a new selection.
Flags: needinfo?(mats)
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #9)
> The blur event hides the carets in the old tab (step 4 in comment 0),

OK, good.

> the subsequent scroll-down (step 5) on the new tab triggers the
> OnScrollEnd() in the old tab

This seems wrong to me.  Scrolling in the new tab shouldn't trigger events
in the old tab, those events should go to the new tab.

Since the problem here seems related to event handling, perhaps
enndeakin / smaug can give better advice?
Flags: needinfo?(mats)
Flags: needinfo?(enndeakin)
Flags: needinfo?(bugs)
The OnScrollEnd() is called by AccessibleCaretEventHub::AsyncPanZoomStopped() [1], which is registered via nsIScrollObserver. I'm not sure whether APZ should distinguish the observer is in the background or not.

[1] http://searchfox.org/mozilla-central/rev/4012e61cc38758ffa1f7ce26039173f67b048853/layout/base/AccessibleCaretEventHub.cpp#727
That doesn't sounds really event handling related.
Shouldn't the observer unregister itself when the docshell goes to background?
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #12)
> That doesn't sounds really event handling related.
> Shouldn't the observer unregister itself when the docshell goes to background?

It seems difficult (or probably overkill) to register/unregister the listener every time the visibility of a document is changed. An easy solution would be just ignore the APZ callbacks when the document is not visible.
Flags: needinfo?(enndeakin)
Comment on attachment 8803282 [details]
Bug 1310509 - Call NotifyAsyncPanZoomStarted/Stopped on document containing current scrolling content.

https://reviewboard.mozilla.org/r/87588/#review89300

Why is this a problem only for AsyncPanZoomStarted/Stopped and not ScrollPositionChanged etc?

IIRC, we have one of these listeners per presshell?  So it seems it would be a performance
problem to have all of those listening for events.  Maybe not so much on mobile, where
I assume the number of tabs is low, but on desktop it's not uncommon to have hundreds
of shells, so if we want to enable these carets on desktop we should probably address
this potential performance issue first.

Perhaps we could add something to PresShell::Freeze()/Thaw() to (de)register the listeners?

This is probably more enndeakin / smaug's area of expertise than mine, so I'd suggest
you get review from one of them instead.  Thanks.
Attachment #8803282 - Flags: review?(mats)
Re comment 15:

> Why is this a problem only for AsyncPanZoomStarted/Stopped and not ScrollPositionChanged etc?

My bad. ScrollPositionChanged should be included as well.

> IIRC, we have one of these listeners per presshell?  So it seems it would be a performance
> problem to have all of those listening for events.  Maybe not so much on mobile, where
> I assume the number of tabs is low, but on desktop it's not uncommon to have hundreds
> of shells, so if we want to enable these carets on desktop we should probably address
> this potential performance issue first.

Yes. We have one AccessibleCaretEventHub per PresShell. I've done some test locally, those listeners only trigger on current tab on my Linux desktop, so I think there's no performance issue on desktop.

> Perhaps we could add something to PresShell::Freeze()/Thaw() to (de)register the listeners?

On Fennec, switching tabs do not trigger PresShell::Freeze()/Thaw().

> This is probably more enndeakin / smaug's area of expertise than mine, so I'd suggest
> you get review from one of them instead.  Thanks.

Mats, thank you for the help so far :)
> I've done some test locally, those listeners only trigger on current tab on my Linux desktop, so I think there's no performance issue on desktop.

This is untrue for desktop with non-e10s. The behavior for non-e10s is like Fennec.

I'm not sure I understand the relationship between APZ and the document structure. Here's what I have observed.

On Fennec, after the scroll ended, APZEventState::ProcessAPZStateChange() calls NotifyAsyncPanZoomStopped() with the top level document (I assume it's the chrome UI) [1].  And then the function calls docShell->NotifyAsyncPanZoomStopped() [2] for all it's child docshells which are all the tabs. That's why all the observers of AccessibleCaretEventHub in all PresShell get called.

I see two possible solutions for this.

1) Call NotifyAsyncPanZoomStarted/Stopped etc only if the document is not hidden assuming the listeners in background tab won't be interested in the callbacks.  Not sure if this makes sense. 
2) Otherwise, I'll go early return if the document is hidden in callbacks AccessibleCaretEventHub, and update patch with Mats' comment 15 addressed.

Kats, this is somewhat related to APZ, so beside smaug, perhaps you'll have some insight. Thanks!

[1] http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/gfx/layers/apz/util/APZEventState.cpp#450-454
[2] http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/docshell/base/nsDocShell.cpp#3113
Flags: needinfo?(bugs)
Flags: needinfo?(bugmail)
Third solution is to implement nsIDocumentActivity for AccessibleCaretEventHub, and register/unregister itself when document goes to foreground/backgound as smaug suggested in comment 12. I'll try it this works as expected.
Flags: needinfo?(bugs)
Flags: needinfo?(bugmail)
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #17)
> 1) Call NotifyAsyncPanZoomStarted/Stopped etc only if the document is not
> hidden assuming the listeners in background tab won't be interested in the
> callbacks.  Not sure if this makes sense. 

Something we could do is just fire the NotifyAsyncPanZoomStarted/Stopped on the innermost document that contains the scrolling thing. That might cut down on the perf hit and also fix this bug. If you want to try it the code in APZEventState::ProcessAPZStateChange would need to be modified to get the document from the viewId rather than using the aDocument that is passed in.
Oh I guess we might have to move the mActiveAPZTransforms counter into the document as well.
Comment on attachment 8803282 [details]
Bug 1310509 - Call NotifyAsyncPanZoomStarted/Stopped on document containing current scrolling content.

https://reviewboard.mozilla.org/r/87588/#review89744

Looks ok to me, but sounds like kats had some different idea too. I guess both approaches work.
Attachment #8803282 - Flags: review?(bugs) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #20)

I'm happy with my latest patch, but I would like to learn a bit more about your approach before I land the patch because modifying something in APZ takes more time for me :)

> Something we could do is just fire the NotifyAsyncPanZoomStarted/Stopped on
> the innermost document that contains the scrolling thing. 

If the innermost document is an iframe, do you mean that we just fire NotifyAsyncPanZoomStarted/Stopped on it? What if I'm scrolling the parent document of the iframe?

> That might cut down on the perf hit and also fix this bug. If you want to try it the code
> in APZEventState::ProcessAPZStateChange would need to be modified to get the
> document from the viewId rather than using the aDocument that is passed in.

My APZ-fu is a bit weak... How to get the document from the viewID?
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #23)
> If the innermost document is an iframe, do you mean that we just fire
> NotifyAsyncPanZoomStarted/Stopped on it? What if I'm scrolling the parent
> document of the iframe?

If you're scrolling the parent document then the "innermost document that contains the scrolling thing" would be the parent document, rather than the iframe, so we'd fire the Notify methods on the parent rather than the iframe.

> My APZ-fu is a bit weak... How to get the document from the viewID?

The ViewID maps to an nsIContent, which you can then use to get the document:
  nsIContent* content = nsLayoutUtils::FindContentFor(viewId);
  nsIDocument* doc = content ? content->GetComposedDoc() : nullptr;
Comment on attachment 8803282 [details]
Bug 1310509 - Call NotifyAsyncPanZoomStarted/Stopped on document containing current scrolling content.

https://reviewboard.mozilla.org/r/87588/#review90362
Attachment #8803282 - Flags: review?(bugmail) → review+
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fbbd157345f
Call NotifyAsyncPanZoomStarted/Stopped on document containing current scrolling content. r=kats
https://hg.mozilla.org/mozilla-central/rev/6fbbd157345f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
I was able to reproduce the issue following the STR provided in the Description.

The fix is now verified on latest Nightly (2016-11-08) by using the next devices:
- Nexus 5 (Android 6.0.1);
- Honor Huawei 5x (Android 5.1.1);
- Nexus 9 (Android 6.0.1).
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1345100
You need to log in before you can comment on or make changes to this bug.