765643 - profile path in profile manager asserts error when hovering over blank

Status: RESOLVED FIXED

(SeaMonkey :: Startup & Profiles, defect)

Description

[Edmund Wong (:ewong)]

Quoting from 670561#c20
> Sadly this has a bug; if you hover over a blank area of the tree you get a 
> blank tooltip, and additionally an assertion in debug builds.


ASSERTION: band index:'aIndex >= 0 && aIndex < PRInt32(mRows.Length())', file
           /c:/mozstuff/smbd/mozilla/layout/xul/base/src/tree/src/nsTreeContentView.cpp, line 729

then it crashes to desktop.

Comment 1

[Edmund Wong (:ewong)]

Attachment: Ensure that hovering over empty areas do not assert. (v1)

Comment 2

[Philip Chee]

are you sure you need either of aEvent.stopPropagation() or aEvent.preventDefault();?
Try something like this:

  if (row.value == -1 || row.value > tree.view.rowCount-1 || aEvent.originalTarget.localName != "treechildren")
    //aEvent.stopPropagation();
    //aEvent.preventDefault();
    return;
  }
  treeTip.label = tree.view.getItemAtIndex(row.value).tooltip;

Comment 3

[Edmund Wong (:ewong)]

(In reply to Philip Chee from comment #2)
> are you sure you need either of aEvent.stopPropagation() or
> aEvent.preventDefault();?
> Try something like this:
> 
>   if (row.value == -1 || row.value > tree.view.rowCount-1 ||
> aEvent.originalTarget.localName != "treechildren")
>     //aEvent.stopPropagation();
>     //aEvent.preventDefault();
>     return;
>   }
>   treeTip.label = tree.view.getItemAtIndex(row.value).tooltip;

That doesn't work.  It doesn't display any of the profiles.

if I tried:

  if (row.value >= 0) {
    treeTip.label = tree.view.getItemAtIndex(row.value).tooltip;
  }
  else {
    return;
  }

it displays the last hovered profile name.  

The stopPropagation() and the preventDefault() both stops it from displaying
an empty tooltip.

Comment 4

[[e-mail]]

(In reply to Philip Chee from comment #2)
> are you sure you need either of aEvent.stopPropagation() or
> aEvent.preventDefault();?
You need aEvent.preventDefault(); to stop the popup from opening. (The other alternative is to return false from the event handler, but you would have to change the XUL to propagate the return value.)

Nit: patch contains too many {}s.

Comment 5

[Ian Neal]

Comment on attachment 633945 [details] [diff] [review]
Ensure that hovering over empty areas do not assert. (v1)

> function HandleToolTipEvent(aEvent)
> {
>   var treeTip = document.getElementById("treetip");
>   var tree = document.getElementById("profiles");
>   var row = {};
> 
>   tree.treeBoxObject.getCellAt(aEvent.clientX, aEvent.clientY, row, {}, {});
>+  if (row.value >= 0) {
>+    treeTip.label = tree.view.getItemAtIndex(row.value).tooltip;
>+  }
>+  else {
>+    aEvent.stopPropagation();
>+    aEvent.preventDefault();
>+  }
> }
I guess you could check if row.value is < 0 do your aEvent stuff and then return early. You could move your defining of treeTip and setting of its label to the end of the function. Not sure how else you can reduce the number of {}s otherwise.
r=me either way.

Comment 6

[Edmund Wong (:ewong)]

Attachment: Ensure that hovering over empty areas do not assert. (v2)

Comment 7

[Edmund Wong (:ewong)]

Attachment: Ensure that hovering over empty areas do not assert. (v3)

Comment 8

[Edmund Wong (:ewong)]

Comment on attachment 638986 [details] [diff] [review]
Ensure that hovering over empty areas do not assert. (v3)

think I better get a second review.

Comment 9

[Ian Neal]

Comment on attachment 638986 [details] [diff] [review]
Ensure that hovering over empty areas do not assert. (v3)

>+  if (row.value < 0) {
You've not removed the {, testing should have shown that.
>+    aEvent.preventDefault();
>+  else
>+    treeTip.label = tree.view.getItemAtIndex(row.value).tooltip;
> }
r=me with that fixed.

Comment 10

[Edmund Wong (:ewong)]

Attachment: Ensure that hovering over empty areas do not assert. (v4)

Comment 11

[Edmund Wong (:ewong)]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/5c2016c497e7