1058249 - Allow TableWidget to append DOMNodes as Cell values

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1058249.patch

Comment 2

[Girish Sharma [:Optimizer]]

Comment on attachment 8478578 [details] [diff] [review]
1058249.patch

Review of attachment 8478578 [details] [diff] [review]:
-----------------------------------------------------------------

Please write a test for this.

Also, I would like to see this in action with over sized dom nodes.

::: browser/devtools/shared/widgets/TableWidget.js
@@ +757,5 @@
>  
>      // Only sort the array if we are sorting based on this column
>      if (this.sorted == 1) {
>        items.sort((a, b) => {
> +        a = (a[this.id] instanceof Ci.nsIDOMNode) ?

Use different variable name here , you are doing a = a[this.id];

Not completely sure, but reference issues can occur.

@@ +885,5 @@
>  
>  Cell.prototype = {
>  
>    set id(value) {
> +    if (value instanceof Ci.nsIDOMNode) {

id can never be a dom node, its only the value that can be. Note, not the local scoped variable value, but this.value

@@ +899,5 @@
>      return this._id;
>    },
>  
>    set value(value) {
> +    if (value instanceof Ci.nsIDOMNode) {

save the whole dom node in value instead of just textContent

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1058249.patch

https://tbpl.mozilla.org/?tree=Try&rev=aa225eb62955

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8478823 [details] [diff] [review]
1058249.patch

Review of attachment 8478823 [details] [diff] [review]:
-----------------------------------------------------------------

We should have documentation inside of tablewidget about the accepted types of values for each cell (string, DOMNode, anything else?).  In this we should note that a DOMNode passed in gets directly appended to the table (at first I assumed it would be a clone, but I think it is fine as-is, we just need to document it).

::: browser/devtools/shared/test/browser_tableWidget_basic.js
@@ +109,5 @@
>      col4: "value34"
>    });
> +
> +  let span = doc.createElement("span");
> +  span.textContent = "Object";

Please use a different string for the text content (since Object could be confused for something else later in the test).  Maybe "domnode" or something.

@@ +333,5 @@
> +  ok(table.isSelected({
> +    col1: "id9",
> +    col2: "value11",
> +    col3: "value23",
> +    col4: span

You don't actually need the span at all for the isSelected assertion - you could replace span with "foo" and still pass the assertion.  In fact, I think it is confusing that it is there (it's a different element that just happens to have the same textContent).

If the point of the check is to make sure that the isSelected function doesn't break when passing in a DOMNode then I would be more explicit:

  ok(table.isSelected({
   col1: "id9",
  }), "Correct row is selected");

  ok(table.isSelected({
   col1: "id9",
   someNode: doc.createElement("span")
  }), "isSelected still works with a DOMNode");

@@ +343,5 @@
> +  is(cell.textContent, "Object", "DOMNode sorted correctly");
> +  cell = cell.nextSibling;
> +
> +  while(cell) {
> +    let currentCell = cell.value || cell.textContent;

I think we should come up with a checkDescending and checkAscending function that takes in a cell and runs this logic for all relevant places in this test.  I usually wouldn't mind duplication in the test, but since the logic has expanded I'd like to make sure all of the sort assertions in this file are using the same checks.

::: browser/devtools/shared/widgets/TableWidget.js
@@ +757,4 @@
>      // Only sort the array if we are sorting based on this column
>      if (this.sorted == 1) {
>        items.sort((a, b) => {
> +        let val1 = (a[this.id] instanceof Ci.nsIDOMNode) ?

Maybe add an isDOMNode utility function at the bottom of the file to kill some of the extra parens around the instanceof checks to make the ternaries easier to read?  Up to you

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1058249.patch

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8478823 [details] [diff] [review]
1058249.patch

Review of attachment 8478823 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/shared/test/browser_tableWidget_basic.js
@@ +109,5 @@
>      col4: "value34"
>    });
> +
> +  let span = doc.createElement("span");
> +  span.textContent = "Object";

Fixed. Used "domnode"

@@ +333,5 @@
> +  ok(table.isSelected({
> +    col1: "id9",
> +    col2: "value11",
> +    col3: "value23",
> +    col4: span

The point of this test was to make sure the json object matched up with the items in the row. However, that doesn't seem to be what isSelected actually does. I removed this test.

@@ +343,5 @@
> +  is(cell.textContent, "Object", "DOMNode sorted correctly");
> +  cell = cell.nextSibling;
> +
> +  while(cell) {
> +    let currentCell = cell.value || cell.textContent;

Fixed. Added 2 new functions to handle the checks.

::: browser/devtools/shared/widgets/TableWidget.js
@@ +757,4 @@
>      // Only sort the array if we are sorting based on this column
>      if (this.sorted == 1) {
>        items.sort((a, b) => {
> +        let val1 = (a[this.id] instanceof Ci.nsIDOMNode) ?

Decided not to do this.

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

https://tbpl.mozilla.org/?tree=Try&rev=c57381889d36

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8479249 [details] [diff] [review]
1058249.patch

Review of attachment 8479249 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/shared/test/browser_tableWidget_basic.js
@@ +322,5 @@
> +  // testing if sorting works should sort by ascending manner
> +  table.sortBy("col4");
> +  let cell = table.tbody.children[6].firstChild.children[1];
> +  is(cell.textContent, "domnode", "DOMNode sorted correctly");
> +  checkAscendingOrder(cell.nextSibling);

Why does cell.nextSibling get passed in here, but cell gets passed in on the previous call to checkAscendingOrder?  Maybe `cell` should just get passed in for all cases, then the first line of the checkAscending would be cell = cell.nextSibling?  If the tests pass it's fine, but I'm just wondering if there is an off by one case that is being missed.

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> Comment on attachment 8479249 [details] [diff] [review]
> 1058249.patch
> 
> Review of attachment 8479249 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/shared/test/browser_tableWidget_basic.js
> @@ +322,5 @@
> > +  // testing if sorting works should sort by ascending manner
> > +  table.sortBy("col4");
> > +  let cell = table.tbody.children[6].firstChild.children[1];
> > +  is(cell.textContent, "domnode", "DOMNode sorted correctly");
> > +  checkAscendingOrder(cell.nextSibling);
> 
> Why does cell.nextSibling get passed in here, but cell gets passed in on the
> previous call to checkAscendingOrder?  Maybe `cell` should just get passed
> in for all cases, then the first line of the checkAscending would be cell =
> cell.nextSibling?  If the tests pass it's fine, but I'm just wondering if
> there is an off by one case that is being missed.

Since I am checking for domnodes, I am starting off by one in those particular cases compare to the previous tests. In checkAscendingOrder, it starts off one ahead and compares its current cell to the previous cell, and vice versa for descending order. The tests should pass, but I hope that is okay.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Gabriel Luong (:gl) from comment #9)
> (In reply to Brian Grinstead [:bgrins] from comment #8)
> > Comment on attachment 8479249 [details] [diff] [review]
> > 1058249.patch
> > 
> > Review of attachment 8479249 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/devtools/shared/test/browser_tableWidget_basic.js
> > @@ +322,5 @@
> > > +  // testing if sorting works should sort by ascending manner
> > > +  table.sortBy("col4");
> > > +  let cell = table.tbody.children[6].firstChild.children[1];
> > > +  is(cell.textContent, "domnode", "DOMNode sorted correctly");
> > > +  checkAscendingOrder(cell.nextSibling);
> > 
> > Why does cell.nextSibling get passed in here, but cell gets passed in on the
> > previous call to checkAscendingOrder?  Maybe `cell` should just get passed
> > in for all cases, then the first line of the checkAscending would be cell =
> > cell.nextSibling?  If the tests pass it's fine, but I'm just wondering if
> > there is an off by one case that is being missed.
> 
> Since I am checking for domnodes, I am starting off by one in those
> particular cases compare to the previous tests. In checkAscendingOrder, it
> starts off one ahead and compares its current cell to the previous cell, and
> vice versa for descending order. The tests should pass, but I hope that is
> okay.

That's fine

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1058249.patch

Another fix was needed to make this fully work. If I click on an appended domnode, I would need to bubble up the tree to get the required data-id in order to select the row.

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

https://tbpl.mozilla.org/?tree=Try&rev=2d884cd5a60b

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/cad3710aa522

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cad3710aa522