1266322 - Fennec for Android doesn't allow user to paste from clipboard on etherpad

Status: RESOLVED FIXED

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

Description

[Swarnava Sengupta (:Swarnava)]

STR:
1. Go to https://public.etherpad-mozilla.org/p/testforclipboard
2. Try to paste something using long tap on text area.
 
Actual Result:
Nothing happening, neither the paste option is coming not copy paste clipboard toolbar is appearing on urlbar.

Expected Result: 
It should allow pasting from other's source.

Comment 1

[Mark Capella [:capella]]

Ah, Etherpad again :/ I notice that CUT isn't made available as an action either, so I'd guess Services.FocusManager is tricking us in _getSelectionTargets() /  [0].


[0] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/ActionBarHandler.js?rev=4646acc6df79&mark=165-165#157

Comment 2

[:Margaret Leibovic]

Is a regression on Nightly, or does this affect all release channels?

Comment 3

[Swarnava Sengupta (:Swarnava)]

(In reply to :Margaret Leibovic from comment #2)
> Is a regression on Nightly, or does this affect all release channels?

I tested with Fennec 46 beta on Samsung Galaxy S4 running Android 5.1.1 Custom Rom

Comment 4

[Mark Capella [:capella]]

Etherpad is a document in designMode. Updating the ActionBarHandler / building some tests doesn't look too difficult on first glance.

I haven't looked at retro-fitting SelectionHandler for pre-48 versions yet.

Comment 5

[Mark Capella [:capella]]

fyi, I'm using [0] for a simplified testcase for Etherpad / designMode documents for now. It may become more robust / complex as I continue through testing. I believe it mostly completes updates for the ActionBarHandler, so the Selection UI (ActionCompat / FloatingToolbar) will work.

I'm going to post my WIP and ask TYLin and jchen to look it over, for core::selection and Keyboards/IME coverage.

for TYLin:
I see cases where I believe the AccessibleCaret shouldn't be hiding for example. It looks like |AccessibleCaretManager::IsCaretDisplayableInCursorMode| is returning false, likely based on the same assumption ActionBarHandler was making, that it's only valid where editables are involved.

for jchen:
It seems there's similar issues associated with having editing available in this new state. Blur/Focus isn't available on the document, and IME hide/show can also be confused when toggling DesignMode via control such as the button in my testpage.

More details as I dig further...

[0] https://www.dropbox.com/s/uruq7j9o62yv8nf/simple_designMode.html?dl=0

Comment 6

[Mark Capella [:capella]]

Attachment: MozReview Request: Bug 1266322 - Fennec doesn't allow user cut/paste in etherpad

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

Comment 7

[Mark Capella [:capella]]

So, the bit I saw for Caret visibility is being addressed seperately in bug 1267576.

@jchen, Using the testcase from comment 5, if I tap the "Design Mode" toggle ON, the keyboard opens for editing, but if you tap it a second time / toggle it OFF, the keyboard stays open. Is this expected? I can file a followup if not.

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Mark Capella [:capella] from comment #7)
> 
> @jchen, Using the testcase from comment 5, if I tap the "Design Mode" toggle
> ON, the keyboard opens for editing, but if you tap it a second time / toggle
> it OFF, the keyboard stays open. Is this expected? I can file a followup if
> not.

Yeah that's currently the expected behavior, but I don't know if it's the correct behavior.

Comment 9

[Mark Capella [:capella]]

Chrome, Opera, default CM13 Browser all behave uniformly, different than Fennec. I filed followup:

Bug 1268968 - Correct IME open/close behaviour on enabling/disabling document.designMode

Comment 10

[Mark Capella [:capella]]

Comment on attachment 8744577 [details]
MozReview Request: Bug 1266322 - Fennec doesn't allow user cut/paste in etherpad

I think this particular patch is ready for review.

Comment 11

[:Margaret Leibovic]

I don't know that I'm the best reviewer here. 

Ting, can you help review this?

Comment 12

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

I'm not super familiar with ActionBarHandler.js, but I can help review and test it.

Comment 13

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

Comment on attachment 8744577 [details]
MozReview Request: Bug 1266322 - Fennec doesn't allow user cut/paste in etherpad

https://reviewboard.mozilla.org/r/48627/#review47415

If I understand this patch correctly, it updates the action bar when the document is in designMode to correctly expose the 'cut' operation. And it works for me.

::: mobile/android/chrome/content/ActionBarHandler.js:68
(Diff revision 1)
>  
>      // Else, update an open ActionBar.
>      if (this._selectionID) {
> -      let [element, win] = this._getSelectionTargets();
> -      if (this._targetElement === element && this._contentWindow === win) {
> +      if (!this._selectionHasChanged()) {
> +        // Still the same active selection.
>          if (e.reason == 'visibilitychange' || e.reason == 'presscaret') {

You'll need to rebase and expect a conflict here. A related easy follow-up here in bug 1266922 :)

::: mobile/android/tests/browser/robocop/testAccessibleCarets.js:130
(Diff revision 1)
>             selectionID: ActionBarHandler._selectionID,
>    };
>  }
>  
>  /**
> + * Checks the Selection UI (ActionBar or FloatingToolbar) 

Nit: white space at the end of the line.

Comment 14

[Mark Capella [:capella]]

Going on review from TYLin, rebased, and nit corrected, let's push-to-try:

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

Comment 15

[Mark Capella [:capella]]

Bah, the one test that complains validly is ours ...

|is(UIhasActionByID("paste_action"), value| fails on TRY. The code assumes pasteable text in the clipboard. While most likely in local tests, it's not true at the start of robocop tests.

Re-push the RC4 test with successful patch. I plan to qfold this and carryover the r+ this weekend.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c284c253914d742e22e26d3ccb86e3601d90360a

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/95f7873a7910

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/95f7873a7910

Comment 18

[Alexandru Simonca [:asimonca], Release Desktop QA]

Is this something you would consider uplifting in Beta 47 and Aurora 48?
I'm going to leave this flagged as: "verified for 49" until further info.