331513 - Deselect selectable items when removed

Status: NEW

(Core :: XUL, defect)

Description

[Neil Deakin]

In nsXULElement::RemoveChildAt, there is some code which deselects listitems. Issues:
- this code should handle any selectable element.
- it can use the currentIndex property to find the current index instead of iterating (currentIndex didn't exist when this code was written)
- it only handles the case where an item is removed from its parent, but should be able to handle it when some other DOM change that would logically cause the selection to be removed happens. 

Also, moving that code might allow us to just use the inherited RemoveChild code, with the benefit of handling mutation events the same way.

Comment 1

[Neil Deakin]

Attachment: unselect items

This patch:
 - changes the code which unselects listitems when they are deleted, so that it supports other selectable items, any element which implements nsIDOMXULSelectControlItemElement
 - when such an element is removed, the next item in the list is automatically selected, or the previous element is the last item is being deleted.

Comment 2

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

So the old code seemed to try to deal with selected elements appearing in hierarchies. I.e. that a selected element could be the child of the removed element. Is that no longer the case, or was it never the case?

Comment 3

[Neil Deakin]

(In reply to comment #2)
> So the old code seemed to try to deal with selected elements appearing in
> hierarchies. I.e. that a selected element could be the child of the removed
> element. Is that no longer the case, or was it never the case?
> 

I think that was leftover from when this code was used for trees, which is no longer the case. The existing code only handles the case when a 'listitem' is removed and listboxes are not hierarchical.

Comment 4

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

Comment on attachment 256199 [details] [diff] [review]
unselect items

>Index: content/xul/content/src/nsXULElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xul/content/src/nsXULElement.cpp,v
>retrieving revision 1.688
>diff -u -p -8 -r1.688 nsXULElement.cpp
>--- content/xul/content/src/nsXULElement.cpp	18 Feb 2007 14:38:02 -0000	1.688
>+++ content/xul/content/src/nsXULElement.cpp	23 Feb 2007 17:26:49 -0000
>@@ -1068,103 +1068,77 @@ nsXULElement::RemoveChildAt(PRUint32 aIn
>     nsresult rv = EnsureContentsGenerated();
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     nsCOMPtr<nsIContent> oldKid = mAttrsAndChildren.GetSafeChildAt(aIndex);
>     if (!oldKid) {
>       return NS_OK;
>     }
> 
>-    // On the removal of a <treeitem>, <treechildren>, or <treecell> element,
>-    // the possibility exists that some of the items in the removed subtree
>-    // are selected (and therefore need to be deselected). We need to account for this.
>-    nsCOMPtr<nsIDOMXULMultiSelectControlElement> controlElement;
>-    nsCOMPtr<nsIListBoxObject> listBox;
>-    PRBool fireSelectionHandler = PR_FALSE;
>-
>-    // -1 = do nothing, -2 = null out current item
>-    // anything else = index to re-set as current
>-    PRInt32 newCurrentIndex = -1;
>-
>-    if (oldKid->NodeInfo()->Equals(nsGkAtoms::listitem, kNameSpaceID_XUL)) {
>-      // This is the nasty case. We have (potentially) a slew of selected items
>-      // and cells going away.
>-      // First, retrieve the tree.
>-      // Check first whether this element IS the tree
>-      controlElement = do_QueryInterface(NS_STATIC_CAST(nsIContent*, this));
>-
>-      // If it's not, look at our parent
>-      if (!controlElement)
>-        rv = GetParentTree(getter_AddRefs(controlElement));
>-
>-      nsCOMPtr<nsIDOMElement> oldKidElem = do_QueryInterface(oldKid);
>-      if (controlElement && oldKidElem) {
>-        // Iterate over all of the items and find out if they are contained inside
>-        // the removed subtree.
>-        PRInt32 length;
>-        controlElement->GetSelectedCount(&length);
>-        for (PRInt32 i = 0; i < length; i++) {
>-          nsCOMPtr<nsIDOMXULSelectControlItemElement> node;
>-          controlElement->GetSelectedItem(i, getter_AddRefs(node));
>-          // we need to QI here to do an XPCOM-correct pointercompare
>-          nsCOMPtr<nsIDOMElement> selElem = do_QueryInterface(node);
>-          if (selElem == oldKidElem &&
>-              NS_SUCCEEDED(controlElement->RemoveItemFromSelection(node))) {
>-            length--;
>-            i--;
>-            fireSelectionHandler = PR_TRUE;
>-          }
>-        }
>-
>-        nsCOMPtr<nsIDOMXULSelectControlItemElement> curItem;
>-        controlElement->GetCurrentItem(getter_AddRefs(curItem));
>-        nsCOMPtr<nsIContent> curNode = do_QueryInterface(curItem);
>-        if (curNode && nsContentUtils::ContentIsDescendantOf(curNode, oldKid)) {
>-            // Current item going away
>-            nsCOMPtr<nsIBoxObject> box;
>-            controlElement->GetBoxObject(getter_AddRefs(box));
>-            listBox = do_QueryInterface(box);
>-            if (listBox && oldKidElem) {
>-              listBox->GetIndexOfItem(oldKidElem, &newCurrentIndex);
>+    // When a selected element is removed, remove it from the selectable
>+    // control's current selection.
>+    PRBool changeCurrentIndex = PR_FALSE;

A better name for this seems to be 'controlIsMultiControl'. Or could you even remove this variable entierly and use |if (multiControl)| instead?

>+    PRInt32 newSelectedIndex = -1;
>+    nsCOMPtr<nsIDOMXULSelectControlElement> control;
>+    nsCOMPtr<nsIDOMXULMultiSelectControlElement> multiControl;
>+    nsCOMPtr<nsIDOMXULSelectControlItemElement> selectableElement =
>+      do_QueryInterface(NS_STATIC_CAST(nsIContent*, oldKid));
>+    if (selectableElement) {
>+      PRBool selected;
>+      rv = selectableElement->GetSelected(&selected);
>+      if (NS_SUCCEEDED(rv) && selected) {
>+        rv = selectableElement->GetControl(getter_AddRefs(control));
>+        if (NS_SUCCEEDED(rv) && control) {

I'm not a big fan of this pattern. I'd rather use:

rv = selectableElement->GetSelected(&selected);
NS_ENSURE_SUCCESS(rv, rv);
if (selected) {
  ...
}

Except of these methods are known to throw for stupid reasons (which should IMHO then be filed as a bug and fixed).

This applies to a lot of call sites in the new code. For example getItemAtIndex doesn't seem to throw but simply return null. Please look into where that is really needed.

r/sr=me with that.

Comment 5

[Neil Deakin]

(In reply to comment #4)
> >+    // When a selected element is removed, remove it from the selectable
> >+    // control's current selection.
> >+    PRBool changeCurrentIndex = PR_FALSE;
> 
> A better name for this seems to be 'controlIsMultiControl'. Or could you even
> remove this variable entierly and use |if (multiControl)| instead?

Not sure how, changeCurrentIndex isn't always set when multiControl is. I'm missing something here maybe?

> 
> Except of these methods are known to throw for stupid reasons (which should
> IMHO then be filed as a bug and fixed).
> 
> This applies to a lot of call sites in the new code. For example getItemAtIndex
> doesn't seem to throw but simply return null. Please look into where that is
> really needed.

Why wouldn't they throw exceptions?

Comment 6

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

(In reply to comment #5)
> (In reply to comment #4)
> > >+    // When a selected element is removed, remove it from the selectable
> > >+    // control's current selection.
> > >+    PRBool changeCurrentIndex = PR_FALSE;
> > 
> > A better name for this seems to be 'controlIsMultiControl'. Or could you 
> > even remove this variable entierly and use |if (multiControl)| instead?
> 
> Not sure how, changeCurrentIndex isn't always set when multiControl is. I'm
> missing something here maybe?

Once newSelectedIndex is >=0 the two are always the same no? Which is the only time when you're using changeCurrentIndex.

> > Except of these methods are known to throw for stupid reasons (which should
> > IMHO then be filed as a bug and fixed).
> > 
> > This applies to a lot of call sites in the new code. For example 
> > getItemAtIndex doesn't seem to throw but simply return null. Please look 
> > into where that is really needed.
> 
> Why wouldn't they throw exceptions?

The problem is that your code eats the exceptions that these functions throw. So rather than:

   PRBool selected;
   rv = selectableElement->GetSelected(&selected);
   if (NS_SUCCEEDED(rv) && selected) {
     rv = selectableElement->GetControl(getter_AddRefs(control));
     if (NS_SUCCEEDED(rv) && control) {

I'd prefer:

   PRBool selected;
   rv = selectableElement->GetSelected(&selected);
   NS_ENSURE_SUCCESS(rv, rv);
   if (selected) {
     rv = selectableElement->GetControl(getter_AddRefs(control));
     NS_ENSURE_SUCCESS(rv, rv);
     if (control) {

I.e. always simply put an NS_ENSURE_SUCCESS after functions that can 'throw' and then only use the out parameters as normal.

Comment 7

[Neil Deakin]

Attachment: address comments

- remove the changeCurrentIndex variable
- propagate more errors
- add a testcase

Comment 8

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

Comment on attachment 268121 [details] [diff] [review]
address comments

Looks great!

Comment 9

[Neil Deakin]

I never checked this in because I felt this was too risky and wanted to test it more.

Comment 10

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

Well, the earlier you check in the more testing it'll get. Though it would be great if you added some mochitests too.