947281 - Search via web magnifying glass alternates visibility in action-bar on text-selection

Status: VERIFIED FIXED

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

Description

[Aaron Train [:aaronmt]]

See test-case, long-tap on selected text and see no magnifying glass in text-selection action-bar. Attempt to long-tap on selected text again, on second attempt the magnifying glass will appear.

--
Nightly (12/06) | LG Nexus 4 (Android 4.4)

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

Not sure if sriram is back or not, so I figured you might enjoy learning this margaret (plus, its good to spread the knowledge out :) )

Found a few little bugs here :( We're sending the correct data to Java, but our measurements say we don't have room in the action bar for this item. We bail and didn't show the menu button.

1.) This fixes the menu button showing but making sure we DON'T add these to GeckoMenu.mActionItems (so that GeckoMenu.hasVisibleItems returns true).

2.) Only adjust the width for a menu button if the menu button isn't visible.

3.) Fix removing items to use the measured with with a View.UNSPECIFIED spec. That's the measurement we use when adding items. It won't match the size after the items are added (but gives us a good maximum). We need to use the same spec when items are removed.

Comment 2

[:Margaret Leibovic]

Comment on attachment 8344829 [details] [diff] [review]
Patch

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

It took me a bit of time to learn the context around this, and I'm still not 100% confident I understand how everything works, but this seems reasonable to me given your explanation. However, I think it would also be good for Sriram to take a look at this if there's not a big rush to get this landed today.

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 8344829 [details] [diff] [review]
Patch

Lets have ram take a look then :)

Comment 4

[Wesley Johnston (:wesj)]

Review ping?

Comment 5

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 8344829 [details] [diff] [review]
Patch

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

Looks good.

::: mobile/android/base/menu/GeckoMenu.java
@@ +174,5 @@
> +        if (mActionItemBarPresenter.addActionItem(actionView)) {
> +            mActionItems.put(menuItem, actionView);
> +            mItems.add(menuItem);
> +            return true;
> +        }

A newline after this.

Comment 6

[Sriram Ramasubramanian [:sriram]]

Reviewed.

Comment 7

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/847cd981f103

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 8344829 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 768667
User impact if declined: icons randomly don't appear in the menu
Testing completed (on m-c, etc.): Landed on mc today 
Risk to taking this patch (and alternatives if risky): Low risk. Fixing a new feature. Without, will have to disable.
String or IDL/UUID changes made by this patch: None

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/847cd981f103

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5f8fe9d708c9

Comment 11

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on: 
Build: Firefox for Android 28 Beta 8 
Device: LG Nexus 4
OS: Android 4.4