Closed Bug 1058249 Opened 5 years ago Closed 5 years ago

Allow TableWidget to append DOMNodes as Cell values

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: gl, Assigned: gl)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Attached patch 1058249.patch (obsolete) — Splinter Review
Attachment #8478578 - Flags: feedback?(scrapmachines)
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
Attachment #8478578 - Flags: feedback?(scrapmachines)
Attached patch 1058249.patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=aa225eb62955
Attachment #8478578 - Attachment is obsolete: true
Attachment #8478823 - Flags: review?(bgrinstead)
Attachment #8478823 - Flags: feedback?(scrapmachines)
Assignee: nobody → gabriel.luong
Blocks: 899753
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
Attachment #8478823 - Flags: review?(bgrinstead) → review+
Attached patch 1058249.patch (obsolete) — Splinter Review
Attachment #8478823 - Attachment is obsolete: true
Attachment #8478823 - Flags: feedback?(scrapmachines)
Attachment #8479249 - Flags: review?(bgrinstead)
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.
Status: NEW → ASSIGNED
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.
Attachment #8479249 - Flags: review?(bgrinstead) → review+
(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.
(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
Keywords: checkin-needed
Keywords: checkin-needed
Attached patch 1058249.patchSplinter Review
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.
Attachment #8479249 - Attachment is obsolete: true
Attachment #8479472 - Flags: review?(bgrinstead)
Attachment #8479472 - Flags: review?(bgrinstead) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/cad3710aa522
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/cad3710aa522
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.