753200 - many menu items disabled (bookmark, share, save as PDF)

Status: VERIFIED FIXED

(Firefox for Android :: General, defect)

Description

[Kevin Brosnan [:kbrosnan]]

Attachment: screen shot

Several bookmark items are disabled when first displayed. The menu items that are disabled are bookmark, share, and save as PDF.

This only seems to happen on the Galaxy SII

STR:
Open Firefox
Click on a top site in about:home
Wait for the page to load and several more seconds
Menu items are disabled

Expected:
Menu items are available

Comment 1

[Kevin Brosnan [:kbrosnan]]

Attachment: video of the issue

Comment 2

[Aaron Train [:aaronmt]]

Basically Gecko is still churning when you opened the menu before they were initialized. They reappear if you close and open the menu again, right?

Comment 3

[Aaron Train [:aaronmt]]

Ok nm comment #2, that's way way way after init

Comment 4

[Aaron Train [:aaronmt]]

I can reproduce on the SII (Aurora 05/08).

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

logcats?

Comment 6

[Johnathan Nightingale [:johnath]]

Discussed in triage - release+, finkle suggests Sriram as assignee.

Comment 7

[Erin Lancaster [:elan]]

fwiw: I can't repro with S2 GT-I9100 on Gingerbread 2.3.4 aurora (2012-05-09).

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

resetting flags

Comment 9

[Aaron Train [:aaronmt]]

I also see this on the Samsung Nexus S (Android 4.0.3)

Just checked logcat while trying this out on that and the SII and there's pretty much nothing visible in log.

Comment 11

[Sriram Ramasubramanian [:sriram]]

http://code.google.com/p/android/issues/detail?id=24231
The idea would be to manually call onPrepareOptionsMenu() once the Document Stop is received on the selected-tab / and a tab is switched.

Comment 12

[Sriram Ramasubramanian [:sriram]]

http://stackoverflow.com/questions/7659354/when-and-how-often-onprepareoptionsmenu-method-is-called-for-actionbar This is available only since honeycomb.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Sriram - Are you able to reproduce the problem? If not, can you make a patch and a test build so Kevin and/or Aaron can try it out?

Comment 14

[Sriram Ramasubramanian [:sriram]]

Here is the problem:
As per documentation, onPrepareOptionsMenu() is called everytime a menu is called.
The first time it is getting called is during startup -- when no tab is loaded, hence everything is disabled.
Now when a tab is loaded, and we try to open the menu, as per android, it has called onPrepareOptionsMenu() already, and hence shows it directly. So, all the items are in disabled. For every other call, onPrepareOptionsMenu() will be called.

How can we force trigger onPrepareOptionsMenu() for the first time it is shown?

Comment 15

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch calls the onPrepareOptionsMenu() for the first time menu opening. There by, the very early call of onPrepareOptionsMenu() is overridden by the new one. This is also optimized in that, we call this only for the first time.

Comment 16

[Aaron Train [:aaronmt]]

Tried out one of Sriram's builds earlier this afternoon and it seemed to fix the issue. Would like a second look from Kevin on it too.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Let's try an approach that does not add isFirstTimeMenuOpen (or any other boolean state)

Comment 18

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This would work fine.
There is one little issue / bug:
What is the probability of getting "Menu:Remove" before trying to open the menu for first time? If that happens, the removed menu will be shown still.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #18)
> Created attachment 623814 [details] [diff] [review]
> Patch
> 
> This would work fine.
> There is one little issue / bug:
> What is the probability of getting "Menu:Remove" before trying to open the
> menu for first time? If that happens, the removed menu will be shown still.

I don't like having this issue built into the code.

Comment 20

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #19)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #18)
> > Created attachment 623814 [details] [diff] [review]
> > Patch
> > 
> > This would work fine.
> > There is one little issue / bug:
> > What is the probability of getting "Menu:Remove" before trying to open the
> > menu for first time? If that happens, the removed menu will be shown still.
> 
> I don't like having this issue built into the code.

The way add-on menu items are implemented is wrong. It tries to add the menu items every time menu is shown. It shouldn't be the case. It should be added once in onCreateOptionsMenu() and when "Menu:Add" is received. Everytime else, it should just ensure that the list is correct as per entries in sExtraMenuItem. This verification can be done with unique id's for addons like "addon_fullscreen", "addon_dominspector" and so on. This will remove the need for sMenu during "Menu:Remove".

Comment 22

[Sriram Ramasubramanian [:sriram]]

I just realized that we can call the "first and forced" onPrepareOptionsMenu() when we receive "Gecko:Ready". (yaaay!) I'll post a patch today.

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

Sriram got a new patch?

Comment 24

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch solves the menu items MIA. The patch calls invalidateOptionsMenu() whenever we update the back/forward button or refresh the url-bar. This basically prepares the options menu for next time it is shown. In case of honeycomb+, we call the parent implementation, to get the overflow-items updated.

Tested on gingerbread, honeycomb and ics. All work fine.

Comment 25

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/963bdd1cd1d7

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/963bdd1cd1d7

Comment 27

[Joe Drew (not getting mail)]

Sriram, can you nom this if you think it's ready?

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 29

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #28)
> Comment on attachment 627313 [details] [diff] [review]
> Patch
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
> User impact if declined: 
> Testing completed (on m-c, etc.): 
> Risk to taking this patch (and alternatives if risky): 
> String or UUID changes made by this patch:

This patch was applied over a lot of tablet UI related patches. If this gets uplifted without them, we might run into merge conflicts in the future.

(Especially the code related to mBrowserToolbar.refresh() in many places).

Comment 30

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: For aurora

This patch is aurora ready.

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 628071 [details] [diff] [review]
Patch: For aurora

Looks like a good port. Waiting for QA to verify the Nightly fix too.

Comment 32

[Kevin Brosnan [:kbrosnan]]

Verified on nightly (2012-05-29) and on Mfinkle's custom Aurora build with the patch.

Comment 33

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 628071 [details] [diff] [review]
Patch: For aurora

[Triage Comment]

Comment 34

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0604c47151d9

Comment 35

[Cristian Nicolae (:xti)]

I cannot reproduce this issue on the latest Aurora and Beta builds. 
Closing bug as verified fixed on:

Firefox 14.0a2 (2012-05-31)
Firefox 14.0b4 (2012-05-31)

Device: Samsung Galaxy Nexus
OS: Android 4.0.2