671305 - tabs.onReady does not fire for images or when using browser's "back"

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Shane Tomlinson [:stomlinson]]

tabs.onReady does not fire when loading up images/non-HTML content.  The code to do this is as follows.

in an add-on script:

const tabs = require("tabs");

tabs.on("ready", function(tab) {
   console.log("tab ready");
});

1) Browse to wikipedia - "tab ready" is displayed on every HTML based page.
2) Open a wikipedia JPEG (drill all the way down to the JPEG, not just a page containing the JPEG) - "tab ready" is not displayed.

The second part of the problem is, when using the "back" button back to an HTML page, "tab ready" is never re-displayed as expected.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

The second issue is intentional, as "ready" is only supposed to fire when content is loaded, not when history is traversed.  The first issue is probably because we trigger "ready" on "DOMContentLoaded", which doesn't fire for non-DOM content.

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(Pushing all open bugs to the --- milestone for the new triage system)

Comment 3

[Ben McCann]

While the behavior in the second issue is intentional, it would be good to create some sort of solution for that use case. It's a very valid use case and I've not been able to come up with any solution that works in that instance. By contrast, Chrome's chrome.tabs.onUpdated is much easier to work with.

https://bugzilla.mozilla.org/show_bug.cgi?id=850023

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Looking into tying the `pageshow` event for tabs (which would solve the back/forward button issue) and a `load` event (which would solve the image/non-HTML content loading issue)

Initial thoughts of possible issues while playing around with this are that DOM content would trigger both the `ready` and `load` event, unless we do MIME-type checking (too scary). Possibly leave how to use it safely up to the add-on developer, providing tools for DOMContentLoaded (ready), pageshow and load

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: GH Pull Request 907

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 730886 [details]
GH Pull Request 907

r+ with comment related to test addressed.

Comment 9

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/75f92ec7940f4012abd5d33c727b56cebe9ecf9e
Merge pull request #907 from jsantell/tabs-onready-for-non-html-671305

Fix Bug 671305: Tabs fire events when loading from cache or non-DOM content r+=@gozala

Comment 10

[Ben McCann]

Awesome. Thanks guys! So this will be in FireFox 22?

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

Yup! :)

Comment 12

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/d5bc78c7aac210af764a08451e33215b11559c3e
Merge pull request #913 from jsantell/tabs-onready-for-non-html-671305

Fix bug 671305, Added pageshow and load events to tabs for bfcache/non-DOM content (resubmitting with non-timer dependent tests)

Comment 13

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Ben McCann from comment #10)
> Awesome. Thanks guys! So this will be in FireFox 22?

Firefox 22 goes to Aurora today or tomorrow, this change hasn't been submitted to mozilla-central yet. Cc'ing Mossop, we should discuss whether to request uplift for this. Aside: it's a common-enough gotcha that I lean to yes.

Comment 14

[Jordan Santell [:jsantell] [@jsantell]]

Currently, the change was reverted to tests failing in Nightly/Aurora for CI -- seems to work fine but getting issues of race conditions, hoping to fix those up today and get it back into addon-sdk master first

Comment 15

[Jordan Santell [:jsantell] [@jsantell]]

Reopened until the tests are resolved: https://tbpl.mozilla.org/?tree=Jetpack&rev=77b6f0c0b817

Comment 16

[Jordan Santell [:jsantell] [@jsantell]]

Ahhh, okay, now back in master!
https://hg.mozilla.org/projects/addon-sdk/rev/afdc545097c3

Comment 17

[Ben McCann]

Cool, thanks Jordan! I hope that we can still get the change uplifted to mozilla-central in time for Firefox 22 =)

Comment 18

[Dave Townsend [:mossop]]

(In reply to Ben McCann from comment #17)
> Cool, thanks Jordan! I hope that we can still get the change uplifted to
> mozilla-central in time for Firefox 22 =)

It's already too late to land non-critical fixes for Firefox 22 so this is going to end up in Firefox 23.

Comment 19

[Ben McCann]

4 months is a pretty long time for us to have our extension not work :-( It's a real shame the release cycle is so long. I'd really love if there was anyway that we could get this out earlier.