1297043 - TableWidget selection does not work when clicking on a cell text content

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P2)

Description

[Nicolas Chevobbe [:nchevobbe]]

STR :
1. Go to the provided URL 
2. Open the console
3. There should be a table, click on the second line ( the non-header one )

Expected result :
It "selects" the line ( turning it blue )

Actual result :
Nothing is selected and there is an error in the jsconsole : 

"TypeError: id is null: set selectedRow@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/TableWidget.js:146:7
TableWidget/this.bindSelectedRow@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/TableWidget.js:112:5
emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:191:13
Column.prototype.onMousedown@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/TableWidget.js:1432:7
EventListener.handleEvent*Column@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/TableWidget.js:1016:3
TableWidget.prototype.setColumns@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/TableWidget.js:696:9
TableWidget@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/TableWidget.js:106:5
Widgets.Table.prototype<.render@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/console-output.js:3615:18
Messages.ConsoleTable.prototype<._renderTable/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/console-output.js:1957:24
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:714:7
Messages.ConsoleTable.prototype<._populateTableData/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/console-output.js:1864:9
DebuggerClient.requester/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:285:9
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
emitOnObject@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:112:9
emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/event/core.js:89:38
Request.prototype.emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:1248:29
DebuggerClient.prototype.onPacket/emitReply@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:1011:29
DevTools RDP*DebuggerClient.prototype.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:707:21
DebuggerClient.requester/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/client/main.js:273:12
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
Messages.ConsoleTable.prototype<._populateTableData@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/console-output.js:1807:7
Messages.ConsoleTable.prototype<.init@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/console-output.js:1738:29
WebConsoleFrame.prototype.logConsoleAPIMessage@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/webconsole.js:1270:16
WebConsoleFrame.prototype._outputMessageFromQueue@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/webconsole.js:2157:16
WebConsoleFrame.prototype._flushMessageQueue@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/webconsole.js:2046:20
"

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

The root cause of the error is in the onMousedown function of the TableWidget.js :

http://searchfox.org/mozilla-central/source/devtools/client/shared/widgets/TableWidget.js#1431

We're using the target element to retrieve the data-id attribute, but deepest element of the "cell" is a span, without this attribute, which is supposed to be on its parent element.

We can even see that we're looking for the closest parent of the target with this attribute, but we won't use it after : 

http://searchfox.org/mozilla-central/source/devtools/client/shared/widgets/TableWidget.js#1426

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

For the STR : you have to click right on the "foo" text to see the bug.

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1297043 - Fixes TableWidget selection when wrapTextInElements is true.

We were using the onMousedown event target to select a row, based on a data-id attribute of the cell node.
But when the wrapTextInElements is true, there is an extra span element inside the cell. And in this case,
we were trying to retrieve the data-id attribute on this span, which led to calling the selection function with a
null id, and thus, not selecting the row.
We fix this by retrieving the attribute from the correct node.
A test case was added in order to make sure the bug is fixed

Review commit: https://reviewboard.mozilla.org/r/73364/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73364/

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

TRY looks okay https://treeherder.mozilla.org/#/jobs?repo=try&revision=87501cbbb0cd

Comment 5

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8783631 [details]
Bug 1297043 - Fixes TableWidget selection when wrapTextInElements is true.

https://reviewboard.mozilla.org/r/73364/#review71340

::: devtools/client/shared/test/browser_tableWidget_mouse_interaction.js:175
(Diff revision 1)
>       "Old node should not have selected class after the click on new node");
>  
> +  info("clicking on the third row cell content to select third row");
> +  event = table.once(TableWidget.EVENTS.ROW_SELECTED);
> +  let node3 = table.tbody.firstChild.firstChild.children[3];
> +  console.log(node3.outerHTML);

This console.log looks like a leftover from debugging and shouldn't be there.

I'd also add a comment explaining what's the difference between the tests on node2 and node3, i.e., that they click on different elements. It's otherwise difficult to figure out from this poorly readable code.

Comment 6

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8783631 [details]
Bug 1297043 - Fixes TableWidget selection when wrapTextInElements is true.

https://reviewboard.mozilla.org/r/73362/#review71342

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8783631 [details]
Bug 1297043 - Fixes TableWidget selection when wrapTextInElements is true.

https://reviewboard.mozilla.org/r/73364/#review71340

> This console.log looks like a leftover from debugging and shouldn't be there.
> 
> I'd also add a comment explaining what's the difference between the tests on node2 and node3, i.e., that they click on different elements. It's otherwise difficult to figure out from this poorly readable code.

sorry about that. 
Since you and I are bothered by how the test looks, I think I'm going to clean it up

Comment 8

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8783631 [details]
Bug 1297043 - Fixes TableWidget selection when wrapTextInElements is true.

https://reviewboard.mozilla.org/r/73364/#review71348

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8783631 [details]
Bug 1297043 - Fixes TableWidget selection when wrapTextInElements is true.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73364/diff/1-2/

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8783631 [details]
Bug 1297043 - Fixes TableWidget selection when wrapTextInElements is true.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73364/diff/2-3/

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8783631 [details]
Bug 1297043 - Fixes TableWidget selection when wrapTextInElements is true.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73364/diff/3-4/

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #7)
> Comment on attachment 8783631 [details]
> Since you and I are bothered by how the test looks, I think I'm going to
> clean it up

In fact, I'm quite busy those day and I won't rewrite the test in this patch.
I changed the variable name (node2, node3) to something more meaningful though.
I might end up rewrite the test later.

Since TRY was okay, I think this can be checked-in

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/a7ec26fab9e2
Fix TableWidget selection when wrapTextInElements is true. r=jsnajdr

Comment 14

[Ryan VanderMeulen (PTO, back July 27)]

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