Closed Bug 300808 Opened 19 years ago Closed 18 years ago

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

Categories

(Core :: Layout, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: martijn.martijn)

References

(Blocks 1 open bug)

Details

(Keywords: testcase)

Attachments

(7 files, 1 obsolete file)

This is a follow-up from bug 269927
See upcoming testcase. The testcase should explain what the bug is about.
Attached file 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.
Attached file testcase2
Another case for when the tooltip is already there and then the content gets
removed.
s/deletes/defeats/
Attachment #189347 - Attachment description: Variant of first testcase that deletes a basic "is in document" check → Variant of first testcase that defeats a basic "is in document" check
Attached patch Proposed patch (obsolete) — Splinter Review
Attachment #189356 - Flags: superreview?(jst)
Attachment #189356 - Flags: review?(neil.parkwaycc.co.uk)
Attached file 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.
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 on attachment 189356 [details] [diff] [review]
Proposed patch

This patch causes shutdown crashes anyway...
Attachment #189356 - Attachment is obsolete: true
Attachment #189356 - Flags: superreview?(jst)
Attachment #189356 - Flags: superreview-
Attachment #189356 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #189356 - Flags: review-
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.
Keywords: helpwanted
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).
Attached patch patch3Splinter Review
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.
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.
(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.
Hmm... That seems odd... any idea why it doesn't work?
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.
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...
So that still leaves the problem of not fixing testcase 4, right?
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).
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...
The event listener approach does indeed sound more attractive.
Blocks: 271054
Isn't this a dupe of bug 45497 (though that one was filed under Linux)?
This bug is a little broader than that one, I think.  And the original problem
reported here is quite different.
Blocks: 45497
Flags: blocking1.9a1?
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?
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?
Attached patch patch4Splinter Review
I haven't checked this in SeaMonkey (since I only build Firefox), but I think it should also work fine in there.
Attachment #221019 - Flags: review?(neil)
Comment on attachment 221019 [details] [diff] [review]
patch4

>+     }
>+     else {
Nit: xpfe style is } else {
Attachment #221019 - Flags: review?(neil) → review+
Comment on attachment 221019 [details] [diff] [review]
patch4

Does this need sr?
/me naively asking Boris for sr.
Attachment #221019 - Flags: superreview?(bzbarsky)
Attachment #221019 - Flags: superreview?(bzbarsky) → superreview+
Assignee: nobody → martijn.martijn
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.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
(I checked in with Neil's nit, btw)
Flags: blocking1.9a1?
Keywords: helpwanted
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: