Closed Bug 1386786 Opened 2 years ago Closed 2 years ago

Fix Android browserAction.setTitle/getTitle behavior on switching and closing tabs

Categories

(WebExtensions :: Android, defect, P1)

defect

Tracking

(firefox55 wontfix, firefox56 verified, firefox57 verified)

VERIFIED FIXED
mozilla57
Tracking Status
firefox55 --- wontfix
firefox56 --- verified
firefox57 --- verified

People

(Reporter: rpl, Assigned: rpl)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files)

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.
Assignee: nobody → lgreco
Blocks: 1330159
Status: NEW → ASSIGNED
Priority: -- → P1
See Also: → 1351111
Attachment #8893065 - Flags: review?(mixedpuppy)
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+
(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
https://hg.mozilla.org/mozilla-central/rev/7fcd967f247d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
https://hg.mozilla.org/projects/date/rev/7fcd967f247d62593d90ebaabad88cf11bf63e92
Bug 1386786 - Fix Android browserAction.setTitle/getTitle behavior on switching and closing tab. r=mixedpuppy
Attached image Animation.gif
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.
Status: RESOLVED → VERIFIED
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)
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?
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+
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:)
Flags: needinfo?(lemnis.martens)
Attached image Foo56.gif
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.
Depends on: 1391212
Thanks for your help Lemnis Martens! 

I opened another Bug 1391212 for the issue you described in comment 14.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.