Closed
Bug 753200
Opened 13 years ago
Closed 13 years ago
many menu items disabled (bookmark, share, save as PDF)
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox14 verified, firefox15 verified, blocking-fennec1.0 +)
VERIFIED
FIXED
Firefox 15
People
(Reporter: kbrosnan, Assigned: sriram)
References
Details
Attachments
(4 files, 2 obsolete files)
67.60 KB,
image/png
|
Details | |
2.13 MB,
video/webm
|
Details | |
7.18 KB,
patch
|
mfinkle
:
review+
mfinkle
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
7.96 KB,
patch
|
mfinkle
:
review+
mfinkle
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•13 years ago
|
OS: Windows 7 → Android
Hardware: x86 → ARM
Reporter | ||
Comment 1•13 years ago
|
||
Comment 2•13 years ago
|
||
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•13 years ago
|
||
Ok nm comment #2, that's way way way after init
Comment 4•13 years ago
|
||
I can reproduce on the SII (Aurora 05/08).
Comment 5•13 years ago
|
||
logcats?
Comment 6•13 years ago
|
||
Discussed in triage - release+, finkle suggests Sriram as assignee.
Assignee: nobody → sriram
blocking-fennec1.0: ? → +
Comment 7•13 years ago
|
||
fwiw: I can't repro with S2 GT-I9100 on Gingerbread 2.3.4 aurora (2012-05-09).
Assignee: sriram → nobody
blocking-fennec1.0: + → ?
Comment 9•13 years ago
|
||
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.
Assignee | ||
Comment 11•13 years ago
|
||
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.
Assignee | ||
Comment 12•13 years ago
|
||
http://stackoverflow.com/questions/7659354/when-and-how-often-onprepareoptionsmenu-method-is-called-for-actionbar This is available only since honeycomb.
Comment 13•13 years ago
|
||
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?
Assignee | ||
Comment 14•13 years ago
|
||
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?
Assignee | ||
Comment 15•13 years ago
|
||
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)
Comment 16•13 years ago
|
||
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•13 years ago
|
||
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-
Assignee | ||
Comment 18•13 years ago
|
||
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)
Comment 19•13 years ago
|
||
(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.
Updated•13 years ago
|
Attachment #623814 -
Flags: review?(mark.finkle) → review-
Assignee | ||
Comment 20•13 years ago
|
||
(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".
Assignee | ||
Comment 22•13 years ago
|
||
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•13 years ago
|
||
Sriram got a new patch?
Assignee | ||
Comment 24•13 years ago
|
||
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)
Updated•13 years ago
|
Attachment #627313 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 25•13 years ago
|
||
Comment 26•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
Comment 27•13 years ago
|
||
Sriram, can you nom this if you think it's ready?
Comment 28•13 years ago
|
||
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?
Assignee | ||
Comment 29•13 years ago
|
||
(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).
Assignee | ||
Comment 30•13 years ago
|
||
This patch is aurora ready.
Comment 31•13 years ago
|
||
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+
Reporter | ||
Comment 32•13 years ago
|
||
Verified on nightly (2012-05-29) and on Mfinkle's custom Aurora build with the patch.
Status: RESOLVED → VERIFIED
status-firefox15:
--- → verified
Comment 33•13 years ago
|
||
Comment on attachment 628071 [details] [diff] [review]
Patch: For aurora
[Triage Comment]
Attachment #628071 -
Flags: approval-mozilla-aurora+
Updated•13 years ago
|
Attachment #627313 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Comment 34•13 years ago
|
||
status-firefox14:
--- → fixed
Comment 35•13 years ago
|
||
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
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•