Wladimir Palant Reporter
	Description • 7 years ago

Steps to reproduce:

1. Download attached extension package.
2. Go to about:debugging and click "Load Temporary Add-on".
3. Select the downloaded extension package.
4. Note the new icon in toolbar with badge text "123".
5. Navigate to http://example.com/

Expected results:

Badge text is cleared automatically when a new page loads into this tab. This is the behavior I see in Chrome 60.

Actual results:

Badge text stays unchanged on the new page. This is the behavior in Firefox 55 as well as in Firefox 57.0a1 nightly (2017-08-29).

	Andy McKay
	Comment 1 • 7 years ago

I believe that rpl got this in a recent bug, at least it doesn't reproduce for me on Nightly now.

	Andy McKay
	Comment 2 • 7 years ago

Oh my mistake, in comment 1 it gets cleared on opening a new tab. I didn't follow the STR in comment 0.

	Mark Striemer [:mstriemer] Assignee
	Comment 3 • 7 years ago

Luca, do you know how we might go about doing this? I would assume we need some sort of listener for navigation to clear the badge but I'm not sure of the implications of that.

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 4 • 7 years ago

Yeah, we do something similar for the pageAction (we clear the pageAction state when the tab has been navigated).

Follows some pointers from the pieces of code that implements the similar behavior for the pageAction:

The pageAction API internals are registering a listener to the tabContext's "location-change" event:

- http://searchfox.org/mozilla-central/rev/2ef8bd8a46a02c68ddbb1d5f25fa254dd7be1fbd/browser/components/extensions/ext-pageAction.js#61

and then the subscribed listener clears the tabContext data related to the tab (if the location has changed because the tab has navigated on a different page) and then updates the button state:

- http://searchfox.org/mozilla-central/rev/2ef8bd8a46a02c68ddbb1d5f25fa254dd7be1fbd/browser/components/extensions/ext-pageAction.js#264-269

The Firefox for Desktop version of the TabContext class is defined in ext-browser.js:

- http://searchfox.org/mozilla-central/rev/2ef8bd8a46a02c68ddbb1d5f25fa254dd7be1fbd/browser/components/extensions/ext-browser.js#104

	Shane Caraveo (:mixedpuppy)
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8912417 [details]
Bug 1395074 - Clear browserAction tab data on navigation

https://reviewboard.mozilla.org/r/183738/#review188990

My first question is, why did we test that navigation did not change tab context?   Are there items in tabcontext that should survive the location change?  I'd like to know that before giving an r+.

	Mark Striemer [:mstriemer] Assignee
	Comment 7 • 7 years ago

(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> My first question is, why did we test that navigation did not change tab context?

I'm not sure why it was tested before. It looks like both pageAction and browserAction were added in bug 1197422 but only the pageAction was getting cleared on navigation. Clearing the global browserAction data appears to be what is happening in Chrome though.

> Are there items in tabcontext that should survive the location change?

Looking at Chrome's implementation [1] it appears to clear everything that can be set with the API [2]. I'm assuming `is_visible_` is the enabled state in Chrome, I tested this with an extension and the enabled state was cleared on navigation. We don't seem to support setting the text colour on the badge but they clear that too.

[1] https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_action.cc?l=214&rcl=c4225f93ca04ad70479d82879124ba3e6db9f7e4
[2] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserAction

	Shane Caraveo (:mixedpuppy)
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8912417 [details]
Bug 1395074 - Clear browserAction tab data on navigation

https://reviewboard.mozilla.org/r/183738/#review189406

::: browser/components/extensions/test/browser/browser_ext_browserAction_context.js:295
(Diff revision 1)
>          },
> +        expect => {
> +          browser.test.log("Navigate to a new page. Expect defaults.");
> +
> +          // TODO: This listener should not be necessary, but the |tabs.update|
> +          // callback currently fires too early in e10s windows.

Hrm, someone left a TODO.  Can you check if this is still actually necessary and if so we should reference a bug# rather than TODO in comments...unless it is just expected behavior.  IIRC we do get more than one onUpdated call.

	Pulsebot
	Comment 10 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/dda6098cc516
Clear browserAction tab data on navigation r=mixedpuppy

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 11 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/dda6098cc516

	Madalin Cotetiu
	Comment 12 • 7 years ago

Verified as fixed in FF58.0b6 in Win 7 64 bit, Ubuntu 14.04 32 bit and MacOS 10.13.1
Marking bug as verified fixed.

	Will Bamberg [:wbamberg]
	Comment 13 • 6 years ago

I've updated the compat data for all the set* APIs:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserAction#Browser_compatibility

Please let me know if this covers it.

	Mark Striemer [:mstriemer] Assignee
	Comment 14 • 6 years ago

Looks good, thanks!