Closed Bug 1384964 Opened 2 years ago Closed 2 years ago

pageAction show and hide do not work on tabId 0 on Android

Categories

(WebExtensions :: Android, defect, P1)

defect

Tracking

(firefox56 verified, firefox57 verified)

VERIFIED FIXED
mozilla56
Tracking Status
firefox56 --- verified
firefox57 --- verified

People

(Reporter: rpl, Assigned: rpl)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

The pageAction show and hide implementation on Firefox for Android is currently unable to work on tabId 0, because of the following ternary `tabId ? tabTracker.getTab(tabId) : null;` which will return null for `tabId == 0`:

- http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/mobile/android/components/extensions/ext-pageAction.js#240,245

(also getPopup and setPopup seems to have the same issue: http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/mobile/android/components/extensions/ext-pageAction.js#250,256)
Attachment #8890921 - Flags: review?(mixedpuppy)
Assignee: nobody → lgreco
Blocks: 1263005
Status: NEW → ASSIGNED
Priority: -- → P1
There should be no real reason to check the tabId manually inside this API methods, the type of this parameter is already checked by the schema wrappers.

I would have liked to also change the test cases to explicitly test to guard this from regressing again, unfortunately the tabId 0 is used by the test harness in the mochitests (that is likely to be the reason why it wasn't tested with the tabId 0), and so I opted to attach a fix for the issue as soon as possible.
Comment on attachment 8890921 [details]
Bug 1384964 - Fix pageAction.show/hide/getPopup/setPopup on Android for tabId 0.

https://reviewboard.mozilla.org/r/162124/#review167436
Attachment #8890921 - Flags: review?(mixedpuppy) → review+
I noticed that browserAction.setTitle/getTitle have a similar issue with tabId 0 (the main difference is that browserAction tabId parameter is actually optional and so we need to keep the ternary but fix it to detect tabId 0 as a valid tabId).
Duplicate of this bug: 1384158
Comment on attachment 8891347 [details]
Bug 1384964 - Fix browserAction.setTitle/getTitle on Android for tabId 0.

https://reviewboard.mozilla.org/r/162544/#review167878
Attachment #8891347 - Flags: review?(mixedpuppy) → review+
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/60076d86bb5c
Fix pageAction.show/hide/getPopup/setPopup on Android for tabId 0. r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/5c6a8f30184c
Fix browserAction.setTitle/getTitle on Android for tabId 0. r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/60076d86bb5c
https://hg.mozilla.org/mozilla-central/rev/5c6a8f30184c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Attached image Bug1384964.gif
This issue is verified as fixed on Fennec 57.0a1 (2017-08-28) under Android 6.0.1.

The pageAction.show/hide, browserAction setTitle/getTitle, pageAction setPopup/getPopup are working on the web page that has tabId 0.

(On Fennec 56.0b5 under Android 6.0.1 browserAction setTitle/getTitle is not working because of the Bug 1392322 which was fixed only in Fennec 57, the pageAction.show/hide and pageAction setPopup/getPopup work as expected in Fennec 56.0b5.)

Please see the attached video.
Status: RESOLVED → VERIFIED
Attached image 56.0b7-1384964.gif
This issue is verified as fixed on Fennec 56.0b7 under Android 6.0.1.

With the uplift of Bug 1392322 on Fennec 56.0b7,the pageAction.show/hide, browserAction setTitle/getTitle, pageAction setPopup/getPopup are working on the web page that has tabId 0.

Please see the attached video.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.