725573 - make outputTree function of accessibleEvents viewer to expand tree

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

outputTree(node, [higlightnodes]) should show highlighted nodes in result tree

Comment 1

[[e-mail]]

Comment on attachment 595659 [details] [diff] [review]
patch

While I was testing this I was disappointed that the accessible event tree had no treelines. I manually added them and was further disappointed to see that they didn't work properly :-( It looks like you misread bug 553364 comment 67 - you removed the variable as well as the check. (The check isn't necessary because the parent must have at least one child, the node itself.)

>+  for (let i = parentChain.length - 1; i >= 0; i--) {
>+    var rowIdx = this.getRowIdxFor(parentChain[i]);
Might be worth having a second parameter for where to start looking for the node, to save having to start at 0 all the time.

>+  if (!row || row.isOpen) {
Want to do the equivalent for collapseRowAt too for consistency?

>+  }
>+  return -1;
Nit: blank line in between here.

>+  var nodesToExpand = [ ];
Nit: []

>+  if (nodesToExpand.length == 0 && isHighlighted) {
>+     nodesToExpand.push(parent);
Nit: 1 space too much indentation.
Interesting trick to avoid expanding the parent of an expanded node though!

Comment 2

[[e-mail]]

Comment on attachment 595659 [details] [diff] [review]
patch

Alternative implementation, although probably wacky:
inDataTreeView.prototype.expandNode =
  function inDataTreeView_expandNode(aNode)
{
  var parentChain = [];
  aNode = aNode.parent;
  while (aNode) {
    parentChain.push(aNode);
    aNode = aNode.parent;
  }

  aNode = parentChain.pop();
  for (var i = 0; aNode && i < this.mRows.length; i++) {
    if (this.getRowAt(i).node == parentChain[0]) {
      this.expandRowAt(rowIdx);
      aNode = parentChain.pop();
    }
  }
}

Comment 3

[[e-mail]]

I just thought of a question:
Should expandNode expand the node itself, or just reveal it? If the latter, you could rename it to revealNode and avoid renaming the other methods.

(In reply to comment #1)
>(From update of attachment 595659 [details] [diff] [review])
>>+  var nodesToExpand = [ ];
>Nit: []
I saw another one somewhere else.

Comment 4

[[e-mail]]

Comment on attachment 595659 [details] [diff] [review]
patch

I came up with another wacky idea: collect all the nodes that need to be expanded in advance in order, then open them all at once. Because they are in order, you only need to loop through the rows once :-)

Note: With extra code to avoid expanding the highlighted rows themselves.

inDataTreeView.prototype.expandNodes =
  function inDataTreeView_expandNodes(aNodes)
{
  var node = aNodes.shift();
  for (var i = 0; node && i < this.mRows.length; i++) {
    if (this.getRowAt(i).node == node) {
      this.expandRowAt(rowIdx);
      node = aNodes.shift();
    }
  }
}

function inAccTreeView(aAccessible, aHighlightList)
{
  inDataTreeView.call(this);

  var list = this.generateChildren(aAccessible, aHighlightList);
  if (list)
    this.expandNodes(list);
}

@@ inAccTreeView.prototype.generateChildren
  var nodesToExpand = null;

  // Add children.
  var childCount = aAccessible.childCount;
  for (let i = 0; i < childCount; i++) {
    var list =
      this.generateChildren(aAccessible.getChildAt(i), aHighlightList, parent);
    if (list)
      nodesToExpand = list.concat(nodesToExpand || []);
  }

  return nodesToExpand ? nodesToExpand.concat(parent) : isHighlighted && [];
}

Comment 5

[[e-mail]]

(In reply to comment #2)
>   aNode = parentChain.pop();
>   for (var i = 0; aNode && i < this.mRows.length; i++) {
>     if (this.getRowAt(i).node == parentChain[0]) {
This should be == aNode, of course.

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 7

[[e-mail]]

Comment on attachment 598756 [details] [diff] [review]
patch2

>+ * Expand nodes from the given list. The caller should ensure that every node
>+ * to expand preceeds its parents in the list.
Typo: precedes
Nit: ancestors?
Could also add:
The list should be in reverse order, and include all ancestor nodes.

>+inDataTreeView.prototype.expandNodes =
>+  function inDataTreeView_expandNode(aNodes)
Nit: expandNodes

Comment 8

[alexander :surkov (:asurkov)]

landed with Neil's nit fixed http://hg.mozilla.org/dom-inspector/rev/c68e5442cef5