Closed Bug 753200 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 15
Tracking Status
firefox14 --- verified
firefox15 --- verified
blocking-fennec1.0 --- +

People

(Reporter: kbrosnan, Assigned: sriram)

References

Details

Attachments

(4 files, 2 obsolete files)

Attached image 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
OS: Windows 7 → Android
Hardware: x86 → ARM
Attached video video of the issue
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?
Ok nm comment #2, that's way way way after init
I can reproduce on the SII (Aurora 05/08).
Discussed in triage - release+, finkle suggests Sriram as assignee.
Assignee: nobody → sriram
blocking-fennec1.0: ? → +
fwiw: I can't repro with S2 GT-I9100 on Gingerbread 2.3.4 aurora (2012-05-09).
Assignee: sriram → nobody
blocking-fennec1.0: + → ?
resetting flags
Assignee: nobody → sriram
blocking-fennec1.0: ? → +
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.
Duplicate of this bug: 751295
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.
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?
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?
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #623753 - Flags: review?(mark.finkle)
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 on attachment 623753 [details] [diff] [review]
Patch

Let's try an approach that does not add isFirstTimeMenuOpen (or any other boolean state)
Attachment #623753 - Flags: review?(mark.finkle) → review-
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #623814 - Flags: review?(mark.finkle)
(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.
Attachment #623814 - Flags: review?(mark.finkle) → review-
(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".
Duplicate of this bug: 757022
I just realized that we can call the "first and forced" onPrepareOptionsMenu() when we receive "Gecko:Ready". (yaaay!) I'll post a patch today.
Sriram got a new patch?
Attached patch PatchSplinter Review
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.
Attachment #623753 - Attachment is obsolete: true
Attachment #623814 - Attachment is obsolete: true
Attachment #627313 - Flags: review?(mark.finkle)
Attachment #627313 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/963bdd1cd1d7
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
Sriram, can you nom this if you think it's ready?
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:
Attachment #627313 - Flags: approval-mozilla-aurora?
(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).
This patch is aurora ready.
Comment on attachment 628071 [details] [diff] [review]
Patch: For aurora

Looks like a good port. Waiting for QA to verify the Nightly fix too.
Attachment #628071 - Flags: review+
Verified on nightly (2012-05-29) and on Mfinkle's custom Aurora build with the patch.
Status: RESOLVED → VERIFIED
Comment on attachment 628071 [details] [diff] [review]
Patch: For aurora

[Triage Comment]
Attachment #628071 - Flags: approval-mozilla-aurora+
Attachment #627313 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
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
Depends on: 760267
You need to log in before you can comment on or make changes to this bug.