Closed
Bug 1263800
Opened 8 years ago
Closed 8 years ago
New install issue, Bookmark actions stop working after Browser dismiss
Categories
(Firefox for Android Graveyard :: General, defect, P2)
Firefox for Android Graveyard
General
Tracking
(firefox45 unaffected, firefox46 unaffected, firefox47 unaffected, firefox48 verified, fennec48+)
VERIFIED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox45 | --- | unaffected |
firefox46 | --- | unaffected |
firefox47 | --- | unaffected |
firefox48 | --- | verified |
fennec | 48+ | --- |
People
(Reporter: capella, Assigned: ahunt)
References
Details
(Keywords: regression)
Attachments
(1 file)
On my test device, with current m-c repo, clobber, full/non-artifact build and fresh install, toggling Bookmarks star in the menu works properly, until after I swipe FF closed and then re-start it. After that, I seem to lose Bookmarks toggle action. [0] https://www.dropbox.com/s/7lilmociia06834/bug_newInstall.mp4?dl=0
Comment 2•8 years ago
|
||
It seems like this only affects small tablets.
tracking-fennec: --- → ?
status-firefox45:
--- → unaffected
status-firefox46:
--- → unaffected
status-firefox47:
--- → unaffected
status-firefox48:
--- → affected
Comment 3•8 years ago
|
||
Regression window: Last good build: 2016-04-09 First bad build: 2016-04-10 pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=705c5fb32d49de2c09b564aa5ff8e8edf1be85ae&tochange=d62963756d9a9d19cbbb5d8f3dd3c7cfa8fdef88
Keywords: regression
Updated•8 years ago
|
Assignee: nobody → ahunt
Assignee | ||
Comment 4•8 years ago
|
||
FWIW I can reproduce this on an N9 too (large tablet, which shows the full tablet UI). I have a suspicion that this is a side effect of: https://hg.mozilla.org/mozilla-central/rev/870df3ae9853 Which interacts with us disabling menu items if the tab == null at: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#3188
Assignee | ||
Comment 5•8 years ago
|
||
If tab data is initially null then we disable the bookmark star. We need to explicitly reenable it when we load an actual page. All other menu items seem to be explicitly enabled as needed below, only bookmarks was omitted (since it's expected to be enabled ~all the time) - but we still could have disabled it previously. Review commit: https://reviewboard.mozilla.org/r/46495/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46495/
Attachment #8741450 -
Flags: review?(gkruglov)
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P2
Comment 6•8 years ago
|
||
Comment on attachment 8741450 [details] MozReview Request: Bug 1263800 - Ensure bookmark star is enabled if tab data available r?grisha https://reviewboard.mozilla.org/r/46495/#review43093 ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3188 (Diff revision 1) > !PrefUtils.getStringSet(GeckoSharedPrefs.forProfile(this), > ClearOnShutdownPref.PREF, > new HashSet<String>()).isEmpty(); > aMenu.findItem(R.id.quit).setVisible(visible); > > if (tab == null || tab.getURL() == null) { I would add a comment here stating explicitely that when we don't have tab data, we disable bunch of things and early return out of this function. Return being the key bit of information. ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3210 (Diff revision 1) > MenuUtils.safeSetEnabled(aMenu, R.id.add_search_engine, false); > MenuUtils.safeSetEnabled(aMenu, R.id.add_to_launcher, false); > > return true; > } > And here, to make code easier to parse quickly, I'd add a comment stating that if we DO have tab data, let's re-enable things as necessary and set proper checked/visible/etc values. Otherwise, since the above `if` clause returns and doesn't have an `else`, if you miss that little return statement, you're spending some amount (small yet i'll never get it back!!) time trying to figure what the hell is this doing.
Attachment #8741450 -
Flags: review?(gkruglov) → review+
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Andrzej Hunt :ahunt from comment #4) > FWIW I can reproduce this on an N9 too (large tablet, which shows the full > tablet UI). > > I have a suspicion that this is a side effect of: > https://hg.mozilla.org/mozilla-central/rev/870df3ae9853 > > Which interacts with us disabling menu items if the tab == null at: > http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/ > mozilla/gecko/BrowserApp.java#3188 Oops I'm wrong, the cause is much simpler, I removed the setEnabled(!isReaderView) call instead of making it setEnabled(true).
Assignee | ||
Comment 8•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/1a22155b5544b1690270b7a3f9e93e495ed4cdd8 Bug 1263800 - Ensure bookmark star is enabled if tab data available r=grisha
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1a22155b5544
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•8 years ago
|
tracking-fennec: ? → 48+
Comment 10•8 years ago
|
||
Verified as fixed in build 48.0a1 2016-04-21; Device: Asus ZenPad 8 (Android 5.0.2);
Status: RESOLVED → VERIFIED
Updated•8 years ago
|
Version: unspecified → Trunk
Updated•3 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
•