334216 - Tabs opened in the background from bookmarks have no generic icon

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Ria Klaassen (not reading all bugmail)]

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?

Comment 1

[Daniel C]

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.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

This happens because error events don't bubble anymore.
I guess they should bubble, at least according to DOM 3 Events draft

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Make error event bubble

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.

Comment 4

[Boris Zbarsky [: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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

sicking, you probably want to raise this in whatever WG is doing DOM3 Events; this needs to be changed in the spec.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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".

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Fixing tabbrowser

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?

Comment 9

[Boris Zbarsky [:bzbarsky]]

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?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(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?

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(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)

Comment 13

[Boris Zbarsky [:bzbarsky]]

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).

Comment 14

[chris hofmann]

would be good to get some kind of resolution on this for 1.9a1

Comment 15

[Ria Klaassen (not reading all bugmail)]

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.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: testcase for anonymous events bubbling

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Attachment: make non-bubbling events to bubble in anonymous content

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...

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

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....

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: bubble to bindingParent (or to point where retargeting happens)

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Attachment: er, this one

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Attachment: bah, even simpler patch for bindingParents

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 24

[Boris Zbarsky [:bzbarsky]]

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?

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 26

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 27

[Boris Zbarsky [:bzbarsky]]

So what behavior does that patch intend to create?

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

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 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 221444 [details] [diff] [review]
bah, even simpler patch for bindingParents

OK.  Let's give this a shot.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 218827 [details] [diff] [review]
Fixing tabbrowser

I'll check in attachment 221444 [details] [diff] [review].

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Checked in

Comment 32

[[e-mail]]

Hmm... so what would happen to, say, a focus event on an anonymous HTML input in a XUL textbox binding?

Comment 33

[Ria Klaassen (not reading all bugmail)]

Thanks for the fix! I haven't seen any empty spots since then.

Comment 34

[[e-mail]]

I filed bug 732738 on comment #32.