Closed Bug 1245831 Opened 4 years ago Closed 4 years ago

The action bar animation is overlapping with the three dot menu

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox44 --- affected
firefox45 --- affected
firefox46 --- affected
firefox47 --- affected
firefox48 --- verified
fennec + ---

People

(Reporter: cos_flaviu, Assigned: sebastian)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
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.
Flags: needinfo?(markcapella)
(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.
Doesn't seem a recent regression, I can repro in nightly from 10/1/2015. Surprised this hasn't been noticed.
Flags: needinfo?(markcapella)
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.
tracking-fennec: ? → +
Flags: needinfo?(michael.l.comella)
(In reply to :Margaret Leibovic from comment #4)

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

Filed bug 1245939.
(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? :)
Flags: needinfo?(michael.l.comella) → needinfo?(markcapella)
... yes, but I'm gonna need some kissin' :)
Flags: needinfo?(markcapella)
meh, based on timing, you probably have to also check mPopupMenu.getMenu().size() > 0
Attached patch bug1245831.diffSplinter Review
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.
Attachment #8716146 - Flags: review?(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.
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 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.
Attachment #8716146 - Flags: review?(margaret.leibovic)
With bug 1171110, won't we still need the ActionBar (vs FloatingToolbar) for devices pre-M?
(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.
@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?
Flags: needinfo?(markcapella)
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.
Flags: needinfo?(markcapella)
Ok. I haven't tried reproducing any of those two bugs yet.
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
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).
Attachment #8716146 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/5344f269671b9cb2cebccfcb0cc6667053c77279
Bug 1245831 - The action bar animation is overlapping with the three dot menu. r=sebastian
https://hg.mozilla.org/mozilla-central/rev/5344f269671b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Verified as fixed in build 48.0a1 2016-04-22;
Device: Asus ZenPad 8 (Android 5.0.2).
You need to log in before you can comment on or make changes to this bug.