1539827 - Copy/ Paste after Select All is broken

Status: VERIFIED FIXED

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

Description

[csheany]

User Agent: Mozilla/5.0 (Android 7.1.1; Tablet; rv:68.0) Gecko/68.0 Firefox/68.0

Steps to reproduce:

1. Open this bug
2. View Page Source
3. Select all
4. Attach File

Actual results:

Text is not pasted

Expected results:

Text is pasted

Comment 1

[csheany]

3.5 Copy :)

Comment 2

[Mohammad Maruf Rahman]

Hi :csheany,

Would you please elaborate the 4th step. What do you mean by 'Attach File'?

In my "Firefox Nightly for Android", I tried the following step:-

• Open Firefox Nightly for Android 68.0a1

• Visit this bug link

• View Page Source

• Select all the 'Page Source' elements and copy

• And then I tried paste the elements in somewhere else

Actual Result : The elements of 'Page Source' are actually not copied successfully

Is this the problem you are trying to say?

Please, let us know so that we can move ahead!

Comment 3

[csheany]

Thank you for your response.

Yes, that is the problem.

Comment 4

[csheany]

Attachment: Screenshot_20190328-150700.jpg

Comment 5

[Sorina Florean [:sflorean]]

Thanks for the report!
Tested on the latest Nightly build and I was able to reproduce the issue following the steps from description and comment 2.

Comment 6

[Makoto Kato [:m_kato]]

nsHTMLCopyEncoder::GetPromotedPoint throws error, so copy is failed. I will investigate this why this failure is only mobile.

Comment 7

[Makoto Kato [:m_kato]]

Before landing bug 676268, we don't copy HTML source, but after landing it, we try to copy HTML source.

And Fennec's action bar has a mistake to use selection all. Firefox/Desktop uses cmd_selectAll, but Fennec doesn't use it (cmd_selectAll isn't same as nsISelectionController.selectAll).

Comment 8

[Makoto Kato [:m_kato]]

Attachment: Bug 1539827 - Use cmd_selectAll instead of nsISelectionController.selectAll on action bar

After bug 676268 is landed, Gecko/Android supports text/html mime type on
clipboard. But copy command is sometimes failed after select all is executed.

nsISelectionContoller.selectAll is different of cmd_selectAll.
Since cmd_selectAll that is used on Firefox desktop doesn't select root
element, copy command always works well. So we should use it like desktop
browser on Fennec.

Also, GV already uses cmd_selectAll on action bar, so this is Fennec only.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6abaf5f96b42
Use cmd_selectAll instead of nsISelectionController.selectAll on action bar r=geckoview-reviewers,esawin

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/6abaf5f96b42

Comment 11

[Eliza Balazs]

Hello!

I tested this following the steps from Comment 2, on the latest version of Nightly 68.0a1 (2019-04-10) with Samsung Galaxy Note 8 (Android 9), Nokia 6 (Android 7.1.1) and I can reproduce this issue. Moreover the browser is closing when tapping on the "Copy" option.

Video: https://drive.google.com/file/d/1NW9KxSk7ykIw0RsN8KGLisr1vzPhiVLG/view

Note:

• On Nexus 6P (Android 8.1.0) the browser is not closing when tapping on the "Copy" option, but the issue is still reproducible.

Comment 12

[csheany]

Do you have any idea why this might be failing?

Comment 13

[Makoto Kato [:m_kato]]

(In reply to csheany from comment #12)

Do you have any idea why this might be failing?

When I test this, it work well. I guess that content is bigger than I test this issue, then TransactionTooLargeException is thrown. Although This is another issue, it is OK to investigate this on this issue.

Comment 14

[Makoto Kato [:m_kato]]

Even if I don't land bug 676268, I think that this will occurs with too large text.

Comment 15

[csheany]

It seemed to be working initially.

Have there been additional patches that could have had an impact?

Comment 16

[Makoto Kato [:m_kato]]

(In reply to csheany from comment #15)

It seemed to be working initially.

Have there been additional patches that could have had an impact?

Yeah, I am working additional fix.

Comment 17

[Makoto Kato [:m_kato]]

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

Comment 18

[Makoto Kato [:m_kato]]

Attachment: Bug 1539827 - Handle RuntimeException when setting clipboard data

When setting large clipboard data, it may cause TransactionTooLargeException
in binder IPC. So we have to handle RuntimeException.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53793d2ea3ec
Handle RuntimeException when setting clipboard data r=geckoview-reviewers,snorp

Comment 20

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/53793d2ea3ec

Comment 21

[csheany]

I'm getting a "Can't copy. Text too long" message.

I imagine that is a side effect of the patch but isn't ideal.

When first landed the functionality was in place.

Can someone help track down the "regression"?

I am still investigating on my end.

Comment 22

[Chris Peterson [:cpeterson]]

(In reply to csheany from comment #21)

I'm getting a "Can't copy. Text too long" message.

What is the exact wording of the error message? Does it look like an Android system message or a Fennec message?

Makoto's patch catches an Android RuntimeException and returns false. Perhaps the patch should pretend the copy succeeds and always return true? Perhaps returning false is triggering an error message in Gecko or Android.

Comment 23

[csheany]

Attachment: 2019_04_19_19_18_19.mp4

Comment 24

[csheany]

If copying doesn't succeed the user should know.

I'm trying to figure out what made it seeem like the first patch never existed.

I'm surprised it broke again in less than a week.

Maybe "Text Copied To Clipboard" shouldn't be displayed?

Comment 25

[Makoto Kato [:m_kato]]

(In reply to csheany from comment #21)

I'm getting a "Can't copy. Text too long" message.

When copying HTML, we try to copy HTML format. If failed, we copy as plain text. So this message may be outputted to logcat when HTML copy is failed. But since we try to text after it, so text is stored to clipboard.

This is Android OS's clipboard limitation that is around 1MB due to binder transaction.

So before landing bug 676268, coping large text will be failed and Fennec may crash due to unhandled exception. After landing this, Fennec doesn't crash at this situation.

Comment 26

[Sorina Florean [:sflorean]]

Tested on the latest Nightly build and the issue is not fixed. The browser is not closing but Paste & Go doesn't work.
The issue is not reproducible when we use just "paste".

Video: https://drive.google.com/file/d/1e0yEcwQRmRTSIrlOWa2zTVOW_fsp9Tuj/view?usp=sharing.

Comment 27

[csheany]

Makoto, thank you for the explanation.

Comment 28

[csheany]

Not again! :)

I am unable to play the video.

What are you exlecting to Paste & Go?

Comment 29

[Sorina Florean [:sflorean]]

(In reply to csheany from comment #28)

Not again! :)

I am unable to play the video.

What are you exlecting to Paste & Go?

The video is available for everyone with the link. For Paste & Go, I used a recommended by Pocket article.

Comment 30

[csheany]

I had to download it but...

Would that be more of an issue with Paste & Go?

This was about it not being copied to the clipboard in the first place.

Comment 31

[Makoto Kato [:m_kato]]

(In reply to Sorina Florean [:sflorean] from comment #26)

Tested on the latest Nightly build and the issue is not fixed. The browser is not closing but Paste & Go doesn't work.
The issue is not reproducible when we use just "paste".

Video: https://drive.google.com/file/d/1e0yEcwQRmRTSIrlOWa2zTVOW_fsp9Tuj/view?usp=sharing.

Could you file new bug instead of reopen this? This seems to be Paste and Go implementation issue of address bar.

Comment 32

[Makoto Kato [:m_kato]]

(Although I add to support text/html, address bar seems to allow formatted text. address bar shouldn't allow it then it only supports plain text that is included in text/html clipdata. So this is address bar issue.

Comment 33

[Sorina Florean [:sflorean]]

Filed bug 1546890 for Paste&Go option. I will close this and mark as verified since Paste option is working.