1352993 - Custom Tab: Action bar is not displayed when selecting a word

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Sorina Florean [:sflorean]]

Attachment: Screenshot_2017-04-03-16-00-14.png

Environment: 
Device: Huawei Honor (Android 5.1.1);
Build: Nightly 55.0a1 (2017-04-02);

Steps to reproduce:
1. Open a web page by custom tabs;
2. Long tap to select a word;

Expected result:
The selected word is highlighted. The action bar is displayed with the following options: share, search, copy and select all items.


Actual result:
The selected word is highlighted. The action bar is not displayed.

Notes:
- Long tap on a page to paste a string doesn't work either (e.g. google.com opened by custom tab and long tap on search field to paste something).
- On Chrome action bar is displayed.

Comment 1

[Julian Chu [:walkingice]]

Attachment: screenshot_20170406.png

I use today's nightly, and I can see those options. It likely is fixed by another bug. Could you verify again once you have time slot, thanks!

Comment 2

[Rachelle Yang [:ryang][[e-mail]]]

Hi Sorina, Per comment1, Can you please help retest the bug ?
Thank you very much !

Comment 3

[Sorina Florean [:sflorean]]

Tested again with Huawei MediaPad M2 (Android 5.1.1), and the issue is still reproducible on latest Nightly.

(In reply to Julian Chu [:walkingice] from comment #1)
> Created attachment 8855302 [details]
> screenshot_20170406.png
I think that is a Android 6 or more, right? The action bar is different for devices with Android 5 and 6.

Comment 4

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

P1 per the triage on April12

Comment 5

[Julian Chu [:walkingice]]

I used Nevin's phone (which has Android 5) and be able to reproduce this bug.

Comment 6

[Julian Chu [:walkingice]]

I just realized that what "ActionBar" did Sorina mean. The action-bar I thought[1] was the view consists url, title on the top of CustomTabsActivity. Now I know it should be the component for text selection.[2]

It seems we have to add the ViewFlipper to CustomTabsActivity.

[1] https://developer.android.com/training/appbar/index.html
[2] https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#1328

Comment 7

[Julian Chu [:walkingice]]

In CustomTabsActivity, we use official ActionBar which supports action mode, and I would like to use it for text-selection.

Current ActionBarTextSelection has some custom classes, and those classes has very similar interface with Android classes. ie, ActionModeCompat vs. ActionMode. My first step is to get rid of those custom classes type. Less custom types means we have more flexibility to implement UI in Activity.

Then, use Presenter pattern, to provide UI response for text-selection.

The final step is to let CustomTabsActivity implements the presenter.

Comment 8

[Julian Chu [:walkingice]]

Attachment: Bug 1352993 - Part 0, remove useless variable

No one is using this variable, remove it.

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

Comment 9

[Julian Chu [:walkingice]]

Attachment: Bug 1352993 - Part 1, Generalize ActionModeCompat

We want to reuse ActionBarTextSelection in more activities, so remove
custom classes from it, to make it easier to interactive with general
Andorid classes.

To achieve that
    * Let ActionModeCompat to be a subclass of ActionMode
    * Get rid of GeckoMenu

BrowserApp is the only one consumer for ActionModeCompat, and it do
understand the class type. We could move animateIn to it safely. After
doing this, TextSelectionActionModeCallback could become a general
ActionMode.Callback.

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

Comment 10

[Julian Chu [:walkingice]]

Attachment: Bug 1352993 - Part 2, Add presenter class which response text-selection

To create new interface ActionModePresenter, a presenter could to
operate action-mode.

For pre-marshmallow Android version, we use ActionBar to provide UI
action for text-selection. Therefore BrowserApp will implement this
presenter for text-selection.

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

Comment 11

[Julian Chu [:walkingice]]

Attachment: Bug 1352993 - Part 3, CustomTabsActivity prvoides presenter

CustomTabsActivity use standard ActionBar, so we can easily use action
mode for text-selection.

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

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8859129 [details]
Bug 1352993 - Part 0, remove useless variable

https://reviewboard.mozilla.org/r/131176/#review133706

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8859130 [details]
Bug 1352993 - Part 1, Generalize ActionModeCompat

https://reviewboard.mozilla.org/r/131178/#review133708

Nice!

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8859131 [details]
Bug 1352993 - Part 2, Add presenter class which response text-selection

https://reviewboard.mozilla.org/r/131180/#review133710

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8859132 [details]
Bug 1352993 - Part 3, CustomTabsActivity prvoides presenter

https://reviewboard.mozilla.org/r/131182/#review133712

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Awesome!

Note that link reported one error:
https://public-artifacts.taskcluster.net/QfhB02gtSVOwRCQdtP1h_A/0/public/android/lint/lint-results-automationDebug.html

Comment 17

[Julian Chu [:walkingice]]

Comment on attachment 8859130 [details]
Bug 1352993 - Part 1, Generalize ActionModeCompat

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131178/diff/1-2/

Comment 18

[Julian Chu [:walkingice]]

Comment on attachment 8859131 [details]
Bug 1352993 - Part 2, Add presenter class which response text-selection

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131180/diff/1-2/

Comment 19

[Julian Chu [:walkingice]]

Comment on attachment 8859132 [details]
Bug 1352993 - Part 3, CustomTabsActivity prvoides presenter

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131182/diff/1-2/

Comment 20

[Julian Chu [:walkingice]]

(In reply to Sebastian Kaspari (:sebastian) from comment #16)
> Awesome!
> 
> Note that link reported one error:
> https://public-artifacts.taskcluster.net/QfhB02gtSVOwRCQdtP1h_A/0/public/
> android/lint/lint-results-automationDebug.html

Thanks!

We have to use ALWAYS to instead of IfRoom(otherwise, some buttons will be hided). So I added SuppressLint annotation to make linter happy.

Comment 21

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 483055072a06 -d 98d3518db0a4: rebasing 390454:483055072a06 "Bug 1352993 - Part 0, remove useless variable r=sebastian"
rebasing 390455:90706f52f943 "Bug 1352993 - Part 1, Generalize ActionModeCompat r=sebastian"
merging mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
rebasing 390456:00120d614925 "Bug 1352993 - Part 2, Add presenter class which response text-selection r=sebastian"
merging mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
merging mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/GeckoApp.java! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 22

[Julian Chu [:walkingice]]

Comment on attachment 8859129 [details]
Bug 1352993 - Part 0, remove useless variable

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131176/diff/1-2/

Comment 23

[Julian Chu [:walkingice]]

Comment on attachment 8859130 [details]
Bug 1352993 - Part 1, Generalize ActionModeCompat

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131178/diff/2-3/

Comment 24

[Julian Chu [:walkingice]]

Comment on attachment 8859131 [details]
Bug 1352993 - Part 2, Add presenter class which response text-selection

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131180/diff/2-3/

Comment 25

[Julian Chu [:walkingice]]

Comment on attachment 8859132 [details]
Bug 1352993 - Part 3, CustomTabsActivity prvoides presenter

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131182/diff/2-3/

Comment 26

[Julian Chu [:walkingice]]

OK! rebased onto m-c

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23e46ff286f5
Part 0, remove useless variable r=sebastian
https://hg.mozilla.org/integration/autoland/rev/3a8bbe748b38
Part 1, Generalize ActionModeCompat r=sebastian
https://hg.mozilla.org/integration/autoland/rev/bad2a571577a
Part 2, Add presenter class which response text-selection r=sebastian
https://hg.mozilla.org/integration/autoland/rev/229519a81ed2
Part 3, CustomTabsActivity prvoides presenter r=sebastian

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/23e46ff286f5
https://hg.mozilla.org/mozilla-central/rev/3a8bbe748b38
https://hg.mozilla.org/mozilla-central/rev/bad2a571577a
https://hg.mozilla.org/mozilla-central/rev/229519a81ed2

Comment 29

[Sorina Florean [:sflorean]]

Attachment: Screenshot_2017-05-08_TextSelectionCustomTab.png

Verified as fixed on latest Nightly build 55.0a1 from 2017-05-08.
Tested with Huawei Honor (Android 5.1.1) and when a word is selected the ActionBar is displayed. There is still an difference regarding the colors but for that issue I'll file a new bug since this was only for ActionBar.