1233709 - Action bar overflow menu sometimes appears with only one item

Status: RESOLVED FIXED

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

Description

[:Margaret Leibovic]

From bug 1171929:

The icons displayed in the ActionBar strip are added dynamically based on available options for the displayed text selection (Cut, Copy, Paste, Share, Search, etc).

As the images are added, we check for the total amount of space required to determine when the main ActionBar is "full", and subsequently add images to the overflow / dropdown menu.

I find on installing Fennec Beta 37.0 en-US on my GS3 (similar sized device) that previously we did indeed manage to fit the share image onto the main ActionBar [0], and now on nightly 46.0a1 it slips into the overflow [1].

I see a difference in the width of the leftmost "Close / checkmark" button that probably accounts for the unexpected behaviour. (Note the position of the thin vertical line to its right in the images).

This would seem correctable, perhaps filed as its own bug.

[0] https://www.dropbox.com/s/zjfru9xykwgdr8l/bug_1171929%20ActionBar%20prev.png?dl=0
[1] https://www.dropbox.com/s/fb1alix62fsm1pb/bug_1171929%20ActionBar%20now.png?dl=0

Comment 1

[Mark Capella [:capella]]

I wonder if mcomella might have an idea here what regressed? ... iirc, there was work with view style and icon consolidations (share / plane, etc) (?)

If not, I can bi-sect towards the issue and/or dig further.

Comment 2

[Mark Capella [:capella]]

Attachment: bug1233709.diff

For this issue I find:
   last good nightly: 2/11/2015 - 228469:ee093ca70666
   first bad nightly: 2/12/2015 - 228716:3094601af679

Inspection of the changesets between those points, only flags |Bug 1097337 - Set the Android 5 statusbar color| as possibly suspicious, as it involves a style/color change. Testing w/backout fails to correct the problem.

I did notice a difference in the styles.xml files for |GeckoActionBar.Title|, and quick testing the attached tweak solves the problem.

View styling/design is not my focus, so asking review? of mcomella to be sure I'm not breaking anything else in the architecture.

Comment 3

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

Comment on attachment 8700346 [details] [diff] [review]
bug1233709.diff

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

From my understanding, this "title" view houses the checkbox. The view became wider than it needs to be so it pushed the other menu elements off the side of the screen into the overflow menu.

I'm honestly not sure why this patch works but I don't see any bad side effects – minWidth should have no effect, afaik, and layout_gravity has the possibility of changing where the check is positioned, but shouldn't have any effect given layout_width="wrap_content". Additionally, this is the only place this style is used.

I don't see any visual changes from these code changes on my GS4 so I can't debug it further.

So if it works for you, it works for me.

::: mobile/android/base/resources/values-v11/styles.xml
@@ +83,5 @@
>  
>      <style name="TextAppearance.Widget.ActionBar.Title" parent="@android:style/TextAppearance.Medium"/>
>  
>      <style name="GeckoActionBar.Title" parent="TextAppearance.Widget.ActionBar.Title">
> +        <item name="android:gravity">center_vertical</item>

Can you separate these from the other attrs and add a comment explaining this is a hack to fix the width of the checkbox? e.g.

<old attrs>

<!-- comment -->
<new attrs>

Comment 4

[Mark Capella [:capella]]

Thanks! For completeness, can I ask you to attach a screenprint?

Comment 5

[Mark Capella [:capella]]

Quick push-to-try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d567c1d57870

Comment 6

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

I can't reproduce this on my 4.* GS4 but I can reproduce on my 5.* GS5. Capella is reproducing on his 5.* GS3.

Maybe 5.* sets a minWidth attribute for the title and this patch works because it overrides that value with 0.

Getting a screenshot...

Comment 7

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

Attachment: Screenshot post patch

lgtm!

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4928b3867881

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4928b3867881

Comment 10

[:Margaret Leibovic]

If you think this is low risk, let's nom it for uplift to aurora and beta.

Comment 11

[Mark Capella [:capella]]

I believe it's low risk, and I'm trying to help the original reporter asap.

Personally, I'm hard-coded to avoid straight-to-beta from m-c at all costs. Is it reasonable to bake on Aurora for a short time first, on the order of "days", or "a week"?

Comment 12

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #11)
> I believe it's low risk, and I'm trying to help the original reporter asap.
> 
> Personally, I'm hard-coded to avoid straight-to-beta from m-c at all costs.
> Is it reasonable to bake on Aurora for a short time first, on the order of
> "days", or "a week"?

Sure, that's fine. It's still early in the cycle. Just don't want us to forget about it is all :)

Comment 13

[Mark Capella [:capella]]

Comment on attachment 8700346 [details] [diff] [review]
bug1233709.diff

Approval Request Comment: We can't always quickly help our most passionate users, but in this case we can.

[Feature/regressing bug #]: Probable OS/API quirk
[User impact if declined]: Negligible. No loss of functionality, but we present inconsistent renderings on similar devices.
[Describe test coverage new/current, TreeHerder]: Hands-on testing by author and reviewer.

[Risks and why]: Minor-to-none. Unlikely case of backout is extremely simple.
[String/UUID change made/needed]: N/A

Comment 14

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/5628bb302871

Comment 15

[Mark Capella [:capella]]

Comment on attachment 8700346 [details] [diff] [review]
bug1233709.diff

Approval Request Comment: Moved to Aurora last week with no expected nor reported issues. Let's complete to Beta, as per margarets suggestion in comment #10.

[Feature/regressing bug #]: Probable OS/API quirk
[User impact if declined]: Negligible. No loss of functionality, but we present inconsistent renderings on similar devices.
[Describe test coverage new/current, TreeHerder]: Hands-on testing by author and reviewer, and baked in m-c and slightly in Aurora.

[Risks and why]: Minor-to-none. Unlikely case of backout is extremely simple.
[String/UUID change made/needed]: N/A

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8700346 [details] [diff] [review]
bug1233709.diff

Verified and baked on Nightly and Aurora. Beta44+

Comment 17

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

https://hg.mozilla.org/releases/mozilla-beta/rev/2bb74ed66aae

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/2bb74ed66aae

Comment 19

[u549602]

Verified as fixed on 44.0b6, tested on Samsung Galaxy S6 Edge with Android 5.1.1.

Comment 20

[Teodora Vermesan (:TeoVermesan)]

Reproduced the issue on 18-12-2015 Nightly build, using Samsung Galaxy S6 Edge (Android 5.1.1).
Verified as fixed using 06-01-2016 Nightly build.

Comment 21

[u549602]

Verified as fixed on latest Aurora build (45.0a2 / 06-01-2016), using Samsung Galaxy S6 Edge (Android 5.1.1)