Closed Bug 334216 Opened 16 years ago Closed 16 years ago

Tabs opened in the background from bookmarks have no generic icon

Categories

(Firefox :: Tabbed Browser, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ria.klaassen, Assigned: smaug)

References

Details

(Keywords: regression)

Attachments

(4 files, 3 obsolete files)

Steps to reproduce: 
1. Open a folder with bookmarks in tabs, or:
2. bookmark this link: http://www.rssfeeds.nl/ , set browser.tabs.loadBookmarksInBackground to true and click on the bookmark
3. Result: where the icon should be present, there is an empty spot.
Screenshot of the problem present in Bug 334214.

Regression range: 
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-03-07+03%3A00&maxdate=2006-03-07+13%3A00

It is not a places bug, for I see it also in a recent build without places.
Maybe bug 234455?
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060417 Firefox/3.0a1

Confirming with Linux. I can sometimes reproduce this when loading in the foreground though.
OS: Windows XP → All
This happens because error events don't bubble anymore.
I guess they should bubble, at least according to DOM 3 Events draft
Attached patch Make error event bubble (obsolete) — Splinter Review
earlier error did bubble in xul, but not elsewhere. I think we want to make
it bubble everywhere. That is anyway what DOM spec says.
Assignee: nobody → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #218687 - Flags: superreview?(bzbarsky)
Attachment #218687 - Flags: review?(bzbarsky)
Comment on attachment 218687 [details] [diff] [review]
Make error event bubble

This will break web sites.  Badly.  The onerror event in HTML on the window is NOT expected to be fired for images.  We went over this once already when I reviewed the patch for bug 234455 and asked you to remove this EXACT code.

I really don't care what the DOM spec says here, if it's not compatible with the web.
Attachment #218687 - Flags: superreview?(bzbarsky)
Attachment #218687 - Flags: superreview-
Attachment #218687 - Flags: review?(bzbarsky)
Attachment #218687 - Flags: review-
sicking, you probably want to raise this in whatever WG is doing DOM3 Events; this needs to be changed in the spec.
hmm, error does bubble (even in 1.8) when it is created for <script>.
And NS_IMAGE_ERROR and NS_SCRIPT_ERROR are the error events we have.
So just wondering which sites will be broken if all error events bubble?
> hmm, error does bubble (even in 1.8) when it is created for <script>.

Correct.

> So just wondering which sites will be broken if all error events bubble?

Feel free to look up CVS blame on the code in question.  But in brief, the answer is "any site that sets an onerror handler on the window to catch script errors".
Because the error event doesn't bubble to tab it's better to handle it in capture phase. If this is ok, should I check it in also to branch?
Attachment #218687 - Attachment is obsolete: true
Attachment #218827 - Flags: superreview?(mconnor)
Attachment #218827 - Flags: review?(mconnor)
The other possibility here would be to "bubble" to the bindingParent for error events, no?  That would make a lot of sense to me; would it fix this bug?
(In reply to comment #9)
> The other possibility here would be to "bubble" to the bindingParent for error
> events, no?  That would make a lot of sense to me; would it fix this bug?
> 

That would be quite strange. Why one event type should behave that way?
I think we should do that for all non-bubbling events (and I believe I mentioned that before).  If we did that, I would be happy to make focus and blur not bubble too.
(In reply to comment #11)
> I think we should do that for all non-bubbling events (and I believe I
> mentioned that before).  If we did that, I would be happy to make focus and
> blur not bubble too.
> 

That should be done in a separate bug (though not sure if that really
works well in all cases. At least it is quite confusing to developers, imo)
My point is that if we did that, then this patch would not be needed.  And without either that, or some sort of event re-firing, events that don't bubble are not usable with XBL (as you encountered in this bug).
would be good to get some kind of resolution on this for 1.9a1
Flags: blocking1.9a1+
Weird, if I load this link in a tab, it has the same problem, although it is not a bookmark: http://forum.fok.nl/topic/844183/2/50
And the same regression range.
This is one possibility. IMO, it is harder for developers to understand
how events works with anon-content, but on the other hand bubbling events 
work better...
And depending on the desired functionality, 
+        // Window starts a new "event retargeting scope".
+        if (!window && !(window = nextTarget->IsWindow())) {
+          nextTarget->SetAnonymousBubbling(PR_TRUE);
+        }
can be removed. That is if bubbling should happen only to bindingParent, 
not eventtargets between target and bindingParent. Hmm, how should it be....
I think that for xbl1 it would make sense to when firing the event at the bound element, treat the event as if we're in AT_TARGET phase. That is, fire all non-capturing (and in current codebase, capturing) listeners and have event.eventPhase set to AT_TARGET.

If this should be done during the capturing or bubbling phase I don't really know. See my recent posting to m.d.t.xbl
Attached patch er, this one (obsolete) — Splinter Review
Attachment #221442 - Attachment is obsolete: true
Attachment #221443 - Attachment is obsolete: true
Comment on attachment 221444 [details] [diff] [review]
bah, even simpler patch for bindingParents

If something like this helps us clean up things elsewhere I think we should do it. Otherwise I think this might as well wait for xbl2
Another option is that bindings could deal with events themselves (eg add a listener on the <browser>, and have that listener fire the same event on the <tabbrowser>).  That would have to be done on a per-event basis as things stand, so it doesn't scale very well... but it does give the binding author full control over whether onload on some random anonymous child corresponds to onload on the bound element itself...  Then again, that would lose the event target info, would it not?
What I want to avoid is spending a lot of time and effort on hacking on xbl1. So I think for now we should do the easy thing, whatever that is.
Comment on attachment 221444 [details] [diff] [review]
bah, even simpler patch for bindingParents

We need to do something, so I propose this. (though it is ok to just fix tabbrowser too)
Attachment #221444 - Flags: superreview?(bzbarsky)
Attachment #221444 - Flags: review?(bugmail)
So what behavior does that patch intend to create?
events have the normal capture phase and the bubble phase
on the "event retargeting points".
So events don't bubble inside anonymous content, but just from targets to
their bindingParent.
Comment on attachment 221444 [details] [diff] [review]
bah, even simpler patch for bindingParents

OK.  Let's give this a shot.
Attachment #221444 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 218827 [details] [diff] [review]
Fixing tabbrowser

I'll check in attachment 221444 [details] [diff] [review].
Attachment #218827 - Flags: superreview?(mconnor)
Attachment #218827 - Flags: review?(mconnor)
Checked in
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Hmm... so what would happen to, say, a focus event on an anonymous HTML input in a XUL textbox binding?
Thanks for the fix! I haven't seen any empty spots since then.
Blocks: 229089
Depends on: 732738
You need to log in before you can comment on or make changes to this bug.