Closed
Bug 1386786
Opened 7 years ago
Closed 7 years ago
Fix Android browserAction.setTitle/getTitle behavior on switching and closing tabs
Categories
(WebExtensions :: Android, defect, P1)
WebExtensions
Android
Tracking
(firefox55 wontfix, firefox56 verified, firefox57 verified)
VERIFIED
FIXED
mozilla57
People
(Reporter: rpl, Assigned: rpl)
References
Details
(Keywords: regression)
Attachments
(3 files)
59 bytes,
text/x-review-board-request
|
mixedpuppy
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
9.62 MB,
image/gif
|
Details | |
4.54 MB,
image/gif
|
Details |
As described in Bug 1351111 Comment 27 the browserAction Android implementation is currently not updating the browserAction title as expected when the user switches between tabs that have a custom browserAction title.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8893065 -
Flags: review?(mixedpuppy)
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8893065 [details] Bug 1386786 - Fix Android browserAction.setTitle/getTitle behavior on switching and closing tab. https://reviewboard.mozilla.org/r/164054/#review170038 ::: mobile/android/components/extensions/ext-browserAction.js:87 (Diff revision 2) > } else { > delete properties[prop]; > } > } > > - if (tab && tab.selected) { > + if (!tab || (tab && tab.selected)) { nit: Couldn't this just be !tab || tab.selected ::: mobile/android/modules/BrowserActions.jsm:16 (Diff revision 2) > > this.EXPORTED_SYMBOLS = ["BrowserActions"]; > > var BrowserActions = { > _browserActions: {}, > + _browserActionTitles: {}, I see why this is being done, but this whole jsm seems pretty redundant, if we had time I'd want to refactor all of this.
Attachment #8893065 -
Flags: review?(mixedpuppy) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #3) > > - if (tab && tab.selected) { > > + if (!tab || (tab && tab.selected)) { > > nit: Couldn't this just be !tab || tab.selected definitely! changed in the updated patch. > ::: mobile/android/modules/BrowserActions.jsm:16 > (Diff revision 2) > > > > this.EXPORTED_SYMBOLS = ["BrowserActions"]; > > > > var BrowserActions = { > > _browserActions: {}, > > + _browserActionTitles: {}, > > I see why this is being done, but this whole jsm seems pretty redundant, if > we had time I'd want to refactor all of this. yeah, I totally agree, and as I wrote in Bug 1387026 Comment 2, once we have fixed this API (and possibly also uplifted the needed fixes to 56 beta), I'd like to refactor a good part of it (in both these js/jsm and the related changes in the BrowserApp java class as well).
Pushed by luca.greco@alcacoop.it: https://hg.mozilla.org/integration/autoland/rev/7fcd967f247d Fix Android browserAction.setTitle/getTitle behavior on switching and closing tab. r=mixedpuppy
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7fcd967f247d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 8•7 years ago
|
||
https://hg.mozilla.org/projects/date/rev/7fcd967f247d62593d90ebaabad88cf11bf63e92 Bug 1386786 - Fix Android browserAction.setTitle/getTitle behavior on switching and closing tab. r=mixedpuppy
Comment 9•7 years ago
|
||
This issue is verified as fixed based on the instructions from https://bugzilla.mozilla.org/show_bug.cgi?id=1351111#c27 on Fennec 57.0a1 (2017-08-08) under Android 7.1.2. “Foo” menu item is displayed for each tab except the second one which displays “Bar” item. See attached screencast.
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Comment 10•7 years ago
|
||
Lemnis, your confirmation would be also very appreciated. Could you please retest your issue described in https://bugzilla.mozilla.org/show_bug.cgi?id=1351111#c27 and see if it’s fixed in Nightly 57? Thanks!
Flags: needinfo?(lemnis.martens)
Assignee | ||
Comment 11•7 years ago
|
||
Comment on attachment 8893065 [details] Bug 1386786 - Fix Android browserAction.setTitle/getTitle behavior on switching and closing tab. Approval Request Comment [Feature/Bug causing the regression]: - Bug 1351111 - Add support for browserAction.setTitle and browserAction.getTitle [User impact if declined]: An extension which uses the browserAction setTitle/getTitle API methods will not behave correctly, and they would be basically not usable in the affected versions of Firefox for Android. [Is this code covered by automated tests?]: Yes, the existent tests have been adapted to better cover this issue. [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: Yes, the same STR used to verify the fix on Nightly should be used to verify it on beta. [List of other uplifts needed for the feature/fix]: Bug 1375857 and Bug 1387026 fixes other issues related to the browserAction Android (complementary to the fixes introduced by this patch, but needed to make the browserAction and its setTitle/getTitle API methods to fully behave as expected). [Is the change risky?]: Low [Why is the change risky/not risky?]: The fix itself is pretty small (it basically fixes three typos on the current implementation, and then introduces some additional small changes to ensure that the test related to this feature is able to catch this kind of issues), the impact of the change is restricted to the browserAction internals and the browserAction setTitle/getTitle test. [String changes made/needed]: None
Attachment #8893065 -
Flags: approval-mozilla-beta?
Updated•7 years ago
|
Comment 12•7 years ago
|
||
Comment on attachment 8893065 [details] Bug 1386786 - Fix Android browserAction.setTitle/getTitle behavior on switching and closing tab. Fix for regression from 55, let's take this for beta 3. Thanks for adding tests.
Attachment #8893065 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 13•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/e7efe3c1a1c4
Comment 14•7 years ago
|
||
Almost, I found a small remaining bug 1. Open Firefox and make sure the second tab is shown (where 'Bar' is displayed) 2. Force close Firefox 3. Reopen Firefox (the second tab should be loaded) 4. Foo is displayed instead of 'Bar' Switching between tabs will update the menu to the correct title. As it is a different bug, I maybe should have opened a new bug report. But I didn't do it before, so I will not do it know O:)
Updated•7 years ago
|
Flags: needinfo?(lemnis.martens)
Comment 15•7 years ago
|
||
This issue is verified as fixed on Fennec 56.0b3 under Android 6.0.1. The “Foo” menu item is displayed for each tab except the second one which displays “Bar” menu item.
Comment 16•7 years ago
|
||
Thanks for your help Lemnis Martens! I opened another Bug 1391212 for the issue you described in comment 14.
Updated•7 years ago
|
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•