Closed
Bug 1245831
Opened 9 years ago
Closed 8 years ago
The action bar animation is overlapping with the three dot menu
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox44 affected, firefox45 affected, firefox46 affected, firefox47 affected, firefox48 verified, fennec+)
RESOLVED
FIXED
Firefox 48
People
(Reporter: cos_flaviu, Assigned: sebastian)
References
Details
Attachments
(1 file)
10.53 KB,
patch
|
sebastian
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
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)
Reporter | ||
Comment 2•9 years ago
|
||
(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•9 years ago
|
||
Doesn't seem a recent regression, I can repro in nightly from 10/1/2015. Surprised this hasn't been noticed.
Flags: needinfo?(markcapella)
Comment 4•9 years ago
|
||
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•9 years ago
|
||
(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)
Comment 8•9 years ago
|
||
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•9 years ago
|
||
meh, based on timing, you probably have to also check mPopupMenu.getMenu().size() > 0
Comment 10•9 years ago
|
||
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)
Comment 11•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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)
Comment 14•9 years ago
|
||
With bug 1171110, won't we still need the ActionBar (vs FloatingToolbar) for devices pre-M?
Comment 15•9 years ago
|
||
(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.
Reporter | ||
Updated•9 years ago
|
status-firefox48:
--- → affected
Assignee | ||
Comment 16•8 years ago
|
||
@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)
Comment 17•8 years ago
|
||
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)
Assignee | ||
Comment 18•8 years ago
|
||
Ok. I haven't tried reproducing any of those two bugs yet.
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Assignee | ||
Comment 19•8 years ago
|
||
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+
Assignee | ||
Comment 20•8 years ago
|
||
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•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Reporter | ||
Comment 22•8 years ago
|
||
Verified as fixed in build 48.0a1 2016-04-22;
Device: Asus ZenPad 8 (Android 5.0.2).
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•