300808 - When content is removed from the document the corresponding tooltip should not appear

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

This is a follow-up from bug 269927
See upcoming testcase. The testcase should explain what the bug is about.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Actually, IE6 does this also wrong (but displays the tooltip at the right spot
at least).
Opera8 does it right, it does not show a tooltip.

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase2

Another case for when the tooltip is already there and then the content gets
removed.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Variant of first testcase that defeats a basic "is in document" check

Comment 4

[Boris Zbarsky [:bzbarsky]]

s/deletes/defeats/

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 6

[Martijn Wargers (dead)]

Attachment: testcase4

I tried the patch. It works fine for testcase1 and testcase2. 
But for the "Variant of first..." testcase, I can't get a tooltip at all for
the green div with the patch.

I would also hope the patch this testcase4 variant, but it doesn't seem like it
does.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yeah, testcase 3 is a problem, since in that case any mousemove actually removes
the node from the document, canceling the tooltip timer...  I really don't see a
good way to fix it.  :(

I think I can fix testcase 4.  Let me poke a second.  ;)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 189356 [details] [diff] [review]
Proposed patch

This patch causes shutdown crashes anyway...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Not quite working...

So the DocumentWillBeDestroyed notification comes far too late to fix testcase
4.  Further, this is still crashing when tearing down the main browser window,
and I can't tell why -- somehow a dead nsXULTooltipListener is ending up as an
observer on the XULDocument, even though I think I wrote code to prevent
that...

I've sort of reached the end of the time I alloted to myself to look into this,
but if someone wants to pick it up, they should feel free to.

Comment 10

[Martijn Wargers (dead)]

I now had a close look at testcase3, I now actually think it is correct to not
show a tooltip. Opera8 is doing that (not showing a tooltip).

Comment 11

[Martijn Wargers (dead)]

Attachment: patch3

This is the "Not quite working..." patch from Boris, but it also removes the
majority of the code in nsXULTooltipListener::MouseOut and replaces it with
HideTooltip().
It fixes the crash on exit.
I don't know why it fixes that, but I think the current code in
nsXULTooltipListener::MouseOut isn't really useful, afaics.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Ah... you do want to have the code in mouseout, because otherwise an
already-posted tooltip will die if you move around a bit (say you have two spans
in a div, the tooltip is for the div, and you move from one span to the other)...

Does just replacing that first block in that function with:

  if (!mCurrentTooltip) {
    KillTooltipTimer();
  }

fix the crash?  It looks to me like it possibly should.

Comment 13

[Martijn Wargers (dead)]

(In reply to comment #12)
> Ah... you do want to have the code in mouseout, because otherwise an
> already-posted tooltip will die if you move around a bit (say you have two spans
> in a div, the tooltip is for the div, and you move from one span to the other)...
That isn't working already now, see:
http://wargers.org/titletip.htm
Moving from one div to the other and the tooltip disappears and appears again.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Hmm... That seems odd... any idea why it doesn't work?

Comment 15

[Martijn Wargers (dead)]

I think so. When mouseout is fired that piece of code aks for getTarget which is
the node that was left and then it checks if that node is the same node as the
node where the tooltipnode is attached to. So, that code should always succeed,
hence what is happening in comment 13.
Perhaps it can be fixed by checking if the relatedTarget is inside the targetNode.

> Does just replacing that first block in that function with:
> 
>   if (!mCurrentTooltip) {
>     KillTooltipTimer();
>   }
> 
> fix the crash?  It looks to me like it possibly should.
Yes, that seems to fix the crash.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Hmm... I guess the targetNode is not the node that has the tooltip on it but
just the last thing we did mousemove over, eh?  Given that, this may be
reasonable...

Comment 17

[Boris Zbarsky [:bzbarsky]]

So that still leaves the problem of not fixing testcase 4, right?

Comment 18

[Martijn Wargers (dead)]

Right. Any ideas how to fix that?
I've submitted bug 301317 for the issue mentioned in comment 13 (it's not easy
for me to fix and I rather keep it separate from this).

Comment 19

[Boris Zbarsky [:bzbarsky]]

Not sure offhand.  I suppose we could add an event listener for the PageHide
event to the document we're observing... Or we could propagate PageHide
notifications to nsIDocumentObservers in some way.  jst, peterv, sicking, thoughts?

I'm guessing the event listener approach is simpler...

Comment 20

[Johnny Stenback (:jst)]

The event listener approach does indeed sound more attractive.

Comment 21

[Samuel Sidler (old account; do not CC)]

Isn't this a dupe of bug 45497 (though that one was filed under Linux)?

Comment 22

[Boris Zbarsky [:bzbarsky]]

This bug is a little broader than that one, I think.  And the original problem
reported here is quite different.

Comment 23

[Martijn Wargers (dead)]

Ok, testcase1 seems to work for me now.
With Testcase 2 and testcase3 I can still see the issue. However, I'm beginning to think this is not a bug, it's more a matter of tastes. Currently, Mozilla is acting the same as IE6 regarding testcase2 and testcase3.

Testcase4, on the other hand, is a real bug (and an annoying one).
Neil created a nifty patch for bug 329521: https://bugzilla.mozilla.org/attachment.cgi?id=215521
Similar code could be used to fix the issue in testcase4.
I've tried this once, and it works.
Is this a good way to fix the bug, Boris?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Hmm... I guess the context menu _is_ rather outside of Gecko that way, eh?

Sure, let's do that.  Make sure to fix both xpfe and toolkit?

Comment 25

[Martijn Wargers (dead)]

Attachment: patch4

I haven't checked this in SeaMonkey (since I only build Firefox), but I think it should also work fine in there.

Comment 26

[[e-mail]]

Comment on attachment 221019 [details] [diff] [review]
patch4

>+     }
>+     else {
Nit: xpfe style is } else {

Comment 27

[Martijn Wargers (dead)]

Comment on attachment 221019 [details] [diff] [review]
patch4

Does this need sr?
/me naively asking Boris for sr.

Comment 28

[Martijn Wargers (dead)]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.649; previous revision: 1.648
done
Checking in xpfe/browser/resources/content/nsBrowserStatusHandler.js;
/cvsroot/mozilla/xpfe/browser/resources/content/nsBrowserStatusHandler.js,v  <--
  nsBrowserStatusHandler.js
new revision: 1.67; previous revision: 1.66
done

Checked in on trunk.

I'm marking this bug fixed. Like I said in comment 23, I don't necessarily think that testcase2 and testcase3 show a bug in Mozilla. If anyone has a different opinion about this, please file a new bug for it.

Comment 29

[Martijn Wargers (dead)]

(I checked in with Neil's nit, btw)