1245831 - The action bar animation is overlapping with the three dot menu

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Flaviu Cos]

Environment: 
Device: Nexus 5 (Android 6.0.1);
Build: Nightly 47.0a1 (2012-02-03);

Steps to reproduce:
1. Launch Fennec;
2. Go to imdb.com;
3. Type anything in the imdb search field;
4. Long tap on the imdb search field;
5. Delete the text entered at step 3;
6. Long tap again on imdb search field.

Expected result:
Action bar is correctly displayed without any overlapping.

Actual result:
The action bar animation is overlapping with the three dot menu.

Notes:
Please check the video:
https://youtu.be/CgGKrkXJdBA

Comment 1

[:Margaret Leibovic]

Looking closely, I don't think this is actually an overlap with the main 3-dot menu, but rather an overlap with the overflow menu from the action bar itself.

I also just tried tapping on an input box on a website, and I saw the overflow menu appear in the action bar. Then when I tapped on it, it went away and fewer icons were shown. But I can't reproduce that issue on Aurora, so I think this is a recent regression.

I think there are some real problems here.

Comment 2

[Flaviu Cos]

(In reply to :Margaret Leibovic from comment #1)

> I also just tried tapping on an input box on a website, and I saw the
> overflow menu appear in the action bar. Then when I tapped on it, it went
> away and fewer icons were shown. But I can't reproduce that issue on Aurora,
> so I think this is a recent regression.

That may be a different issue since it is only reproducible on Nightly and the issue from the description is reproducible on all branches.

Comment 3

[Mark Capella [:capella]]

Doesn't seem a recent regression, I can repro in nightly from 10/1/2015. Surprised this hasn't been noticed.

Comment 4

[:Margaret Leibovic]

Mike, do you have ideas on what might need to be done to fix this? Could this be a good mentor bug?

I'll file another bug about the more serious problem I'm seeing.

Comment 5

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #4)

> I'll file another bug about the more serious problem I'm seeing.

Filed bug 1245939.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to :Margaret Leibovic from comment #4)
> Mike, do you have ideas on what might need to be done to fix this?

I honestly don't know much about the action bar (I think capella might know better?).

I think we dynamically fill the action bar options so if I had to guess, we're going from a blank state and removing/adding options from the blank state to correctly fill it and we do so during the animation. The menu button could be a disappearing artifact from the blank state.

> Could this be a good mentor bug?

Probably not – we do some weird JS munging to get this to work and it's always confused me when I've looked at it briefly.

Mark, do you have a better informed opinion? :)

Comment 7

[Mark Capella [:capella]]

... yes, but I'm gonna need some kissin' :)

Comment 8

[Mark Capella [:capella]]

perhaps if() check the view visiblity at [0] ?

[0] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/ActionModeCompatView.java#175

Comment 9

[Mark Capella [:capella]]

meh, based on timing, you probably have to also check mPopupMenu.getMenu().size() > 0

Comment 10

[Mark Capella [:capella]]

Attachment: bug1245831.diff

As mentioned on irc, the big problem was that the ActionBar animation was being called before the menu items were actually populated, so there wasn't really a correct way to predict whether or not to individually animate the overflow button on its initial showing.

This seems like a simple enough fix. I've moved the animateIn() trigger later in the code flow, guarded the popup menu button animation, and fixed a minor Menu vs. GeckoMenu typing inconsistency that seems safe.

I'll start with margaret for r? and let her decide if she wants to involve others. kats (?) might be interested due to the BrowserApp change near the DynamicToolbar bits.

Comment 11

[:Margaret Leibovic]

I'm having a tough time reproducing this locally because I'm just seeing bug 1245939, and in that case we always show the 3-dot menu.

I also don't feel really confident reviewing this patch... mcomella might be a better reviewer. But first I want to be able to verify locally that this resolves the issue.

Comment 12

[Mark Capella [:capella]]

Ah, STR is:
   ) Launch simple <input> [0]
   ) Long-press |production| to open ActionBar with overflow menu
   ) Ignore if carets disappear, that's currently noted
     in Bug 1245754 comment 1
   ) Tap selectAll
   ) Tap backspace
   ) Tap ActionBar close button
   ) Longpress into empty input to open ActionBar with -no- overflow menu
   ) Observe issue [1]

I'd welcome review from mcomella. I lost support in this area after wes moved on.


[0] https://www.dropbox.com/s/mzzgw1i4uav925y/test_bug1241558_backspace.html?dl=0
[1] https://www.dropbox.com/s/em629gjx4xdajtf/bug_1245831_theBug.mp4?dl=0

Comment 13

[:Margaret Leibovic]

Comment on attachment 8716146 [details] [diff] [review]
bug1245831.diff

Review of attachment 8716146 [details] [diff] [review]:
-----------------------------------------------------------------

I'm clearing review for now, since I think our time would be better spent fixing bug 1171110, which would address this issue.

Comment 14

[Mark Capella [:capella]]

With bug 1171110, won't we still need the ActionBar (vs FloatingToolbar) for devices pre-M?

Comment 15

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #14)
> With bug 1171110, won't we still need the ActionBar (vs FloatingToolbar) for
> devices pre-M?

I believe Sebastian was going to look into copying the FloatingToolbar code into our tree, so that we'll use our own implementation.

We're already using our own code for this custom action bar, so I think it would be fine to make a switch to use different code for the custom floating toolbar.

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

@capella: Now that we know that the floating toolbar will only be on Android 6+ devices... do we need to review/land this patch here? Does this patch fix bug 1245939 too?

Comment 17

[Mark Capella [:capella]]

Yah, sorry on IRC I conflated these two, but I'm still thinking they're related. This patch demonstrably fixs this issue, and it will still exist for users of pre-6.0 (non Floating) ActionBar.

It can be skinnied a bit to make it easier to review (GeckoMenu change is "while we're there"), some of the null-checking bit is overkill.

I was mentally putting off looking into bug 1245939 until factoring out this one, and then you got assigned there so I've moved on.

Have you repro-ed bug 1245939 before/after this patch? I'm fine to toss these both to you.

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Ok. I haven't tried reproducing any of those two bugs yet.

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8716146 [details] [diff] [review]
bug1245831.diff

Review of attachment 8716146 [details] [diff] [review]:
-----------------------------------------------------------------

I can reproduce the issue (and also see some other flickering). The patch fixes it and looks sane. I'll land a rebased version of the patch (minor merge conflict).

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/5344f269671b9cb2cebccfcb0cc6667053c77279
Bug 1245831 - The action bar animation is overlapping with the three dot menu. r=sebastian

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/5344f269671b

Comment 22

[Flaviu Cos]

Verified as fixed in build 48.0a1 2016-04-22;
Device: Asus ZenPad 8 (Android 5.0.2).