Closed Bug 1233709 Opened 4 years ago Closed 4 years ago

Action bar overflow menu sometimes appears with only one item

Categories

(Firefox for Android :: Text Selection, defect)

35 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox44 --- verified
firefox45 --- verified
firefox46 --- verified
b2g-v2.5 --- fixed
fennec + ---

People

(Reporter: Margaret, Assigned: capella)

Details

Attachments

(2 files)

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
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.
Flags: needinfo?(michael.l.comella)
Attached patch bug1233709.diffSplinter Review
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.
Flags: needinfo?(michael.l.comella)
Attachment #8700346 - Flags: review?(michael.l.comella)
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>
Attachment #8700346 - Flags: review?(michael.l.comella) → review+
Thanks! For completeness, can I ask you to attach a screenprint?
Flags: needinfo?(michael.l.comella)
Assignee: nobody → markcapella
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...
Attached image Screenshot post patch
lgtm!
Flags: needinfo?(michael.l.comella)
https://hg.mozilla.org/mozilla-central/rev/4928b3867881
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
If you think this is low risk, let's nom it for uplift to aurora and beta.
tracking-fennec: ? → +
Flags: needinfo?(markcapella)
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"?
Flags: needinfo?(markcapella) → needinfo?(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 :)
Flags: needinfo?(margaret.leibovic)
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
Attachment #8700346 - Flags: approval-mozilla-aurora?
Attachment #8700346 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Attachment #8700346 - Flags: approval-mozilla-beta?
Comment on attachment 8700346 [details] [diff] [review]
bug1233709.diff

Verified and baked on Nightly and Aurora. Beta44+
Attachment #8700346 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified as fixed on 44.0b6, tested on Samsung Galaxy S6 Edge with Android 5.1.1.
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.
Verified as fixed on latest Aurora build (45.0a2 / 06-01-2016), using Samsung Galaxy S6 Edge (Android 5.1.1)
You need to log in before you can comment on or make changes to this bug.