Closed
Bug 671305
Opened 12 years ago
Closed 10 years ago
tabs.onReady does not fire for images or when using browser's "back"
Categories
(Add-on SDK Graveyard :: General, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: stomlinson, Assigned: jsantell)
References
Details
Attachments
(1 file)
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•12 years ago
|
||
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.
Priority: -- → P2
Target Milestone: --- → 1.1
(Pushing all open bugs to the --- milestone for the new triage system)
Target Milestone: 1.1 → ---
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jsantell
Comment 3•10 years ago
|
||
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
Assignee | ||
Comment 4•10 years ago
|
||
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
Priority: P2 → P1
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #730886 -
Flags: review?(rFobic)
Comment 8•10 years ago
|
||
Comment on attachment 730886 [details]
GH Pull Request 907
r+ with comment related to test addressed.
Attachment #730886 -
Flags: review?(rFobic) → review+
Comment 9•10 years ago
|
||
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
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 10•10 years ago
|
||
Awesome. Thanks guys! So this will be in FireFox 22?
Assignee | ||
Comment 11•10 years ago
|
||
Yup! :)
Comment 12•10 years ago
|
||
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•10 years ago
|
||
(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.
Assignee | ||
Comment 14•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 15•10 years ago
|
||
Reopened until the tests are resolved: https://tbpl.mozilla.org/?tree=Jetpack&rev=77b6f0c0b817
Assignee | ||
Comment 16•10 years ago
|
||
Ahhh, okay, now back in master! https://hg.mozilla.org/projects/addon-sdk/rev/afdc545097c3
Assignee | ||
Updated•10 years ago
|
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Comment 17•10 years ago
|
||
Cool, thanks Jordan! I hope that we can still get the change uplifted to mozilla-central in time for Firefox 22 =)
Comment 18•10 years ago
|
||
(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•10 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•