[Sync] Make about:sync events page more selection friendly

[Sync] Make about:sync events page more selection friendly

The details toggle only happens when you actually click the button now,
and it no longer resizes columns and modifies the page when expanded.
Instead, the details column takes over the whole row, so its easier to
view and select text.

R=rlarocque@chromium.org
TBR=bauerb@chromium.org
BUG=401323
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291478

[Nicolas Zea, 2014-08-07 22:25:51]

PTAL

[rlarocque, 2014-08-07 22:49:13]

Some comments.

For now, I'll ignore the fact that we're using tables here, since that issue
predates your CL.

https://codereview.chromium.org/454573002/diff/1/chrome/browser/resources/syn...
File chrome/browser/resources/sync_internals/events.js (right):

https://codereview.chromium.org/454573002/diff/1/chrome/browser/resources/syn...
chrome/browser/resources/sync_internals/events.js:17: if
(detailsNode.getAttribute('hidden') != null) {
hasAttribute()?

https://codereview.chromium.org/454573002/diff/1/chrome/browser/resources/syn...
chrome/browser/resources/sync_internals/events.js:18: for (var node in
detailsRow.childNodes) {
Rather than iterate and change the DOM structure like this, it might make more
sense to toggle a CSS class to make these structures toggle between a normal
state and "display: none".

https://codereview.chromium.org/454573002/diff/1/chrome/browser/resources/syn...
chrome/browser/resources/sync_internals/events.js:19: if
(detailsRow.childNodes[node].nodeType == 1) {
== Node.ELEMENT_NODE?

Better yet, why not just iterate over elements?
(try detailsRow.children)

https://codereview.chromium.org/454573002/diff/1/chrome/browser/resources/syn...
chrome/browser/resources/sync_internals/events.js:20:
detailsRow.childNodes[node].setAttribute('hidden', 'hidden');
set value to ''?  A hidden=hidden attribute seems weird.

[Nicolas Zea, 2014-08-21 22:29:57]

PTAL. Kept the tables, but tried to make things more standard and use CSS where
possible.

https://codereview.chromium.org/454573002/diff/1/chrome/browser/resources/syn...
File chrome/browser/resources/sync_internals/events.js (right):

https://codereview.chromium.org/454573002/diff/1/chrome/browser/resources/syn...
chrome/browser/resources/sync_internals/events.js:17: if
(detailsNode.getAttribute('hidden') != null) {
On 2014/08/07 22:49:12, rlarocque wrote:
> hasAttribute()?

Done.

https://codereview.chromium.org/454573002/diff/1/chrome/browser/resources/syn...
chrome/browser/resources/sync_internals/events.js:18: for (var node in
detailsRow.childNodes) {
On 2014/08/07 22:49:12, rlarocque wrote:
> Rather than iterate and change the DOM structure like this, it might make more
> sense to toggle a CSS class to make these structures toggle between a normal
> state and "display: none".

Done.

https://codereview.chromium.org/454573002/diff/1/chrome/browser/resources/syn...
chrome/browser/resources/sync_internals/events.js:19: if
(detailsRow.childNodes[node].nodeType == 1) {
On 2014/08/07 22:49:12, rlarocque wrote:
> == Node.ELEMENT_NODE?
> 
> Better yet, why not just iterate over elements?
> (try detailsRow.children)

I'd still have to check the nodeType though right? the |length| attribute is
considered an element (and included in the children iterator).

Although this is a moot point now that I'm using CSS for this.

https://codereview.chromium.org/454573002/diff/1/chrome/browser/resources/syn...
chrome/browser/resources/sync_internals/events.js:20:
detailsRow.childNodes[node].setAttribute('hidden', 'hidden');
On 2014/08/07 22:49:12, rlarocque wrote:
> set value to ''?  A hidden=hidden attribute seems weird.

Done.

[rlarocque, 2014-08-21 23:39:32]

Some more nits.

I'm not opposed to committing it as is, so LGTM.

https://codereview.chromium.org/454573002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/events.css (right):

https://codereview.chromium.org/454573002/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/events.css:22: display: inherit;
I don't know about this...

As a <td>, this would have a default display of 'table-cell'.  Its parent, <tr>
has display of 'table-row'.

Maybe it's better to omit this part.

https://codereview.chromium.org/454573002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/events.js (right):

https://codereview.chromium.org/454573002/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/events.js:18: if (detailsRow.className
== 'event-row-minimized') {
nit:

You can have more than one className on an element.  You might be able to make
this code cleaner by taking advantage of it.  ie. Make 'minimized' a separate
class, and toggle it off and on.

See https://developer.mozilla.org/en-US/docs/Web/API/Element.classList

[Nicolas Zea, 2014-08-22 00:06:29]

Comments, addressed, committing.

https://codereview.chromium.org/454573002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/events.css (right):

https://codereview.chromium.org/454573002/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/events.css:22: display: inherit;
On 2014/08/21 23:39:31, rlarocque wrote:
> I don't know about this...
> 
> As a <td>, this would have a default display of 'table-cell'.  Its parent,
<tr>
> has display of 'table-row'.
> 
> Maybe it's better to omit this part.

Removed.

https://codereview.chromium.org/454573002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/sync_internals/events.js (right):

https://codereview.chromium.org/454573002/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_internals/events.js:18: if (detailsRow.className
== 'event-row-minimized') {
On 2014/08/21 23:39:31, rlarocque wrote:
> nit:
> 
> You can have more than one className on an element.  You might be able to make
> this code cleaner by taking advantage of it.  ie. Make 'minimized' a separate
> class, and toggle it off and on.
> 
> See https://developer.mozilla.org/en-US/docs/Web/API/Element.classList

Switched to classLists

[Nicolas Zea, 2014-08-22 00:07:02]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-08-22 00:09:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/454573002/40001

[commit-bot: I haz the power, 2014-08-22 01:14:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-22 01:48:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-22 01:49:00]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nicolas Zea, 2014-08-22 02:13:34]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-08-22 02:15:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/454573002/40001

[commit-bot: I haz the power, 2014-08-22 03:31:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-22 04:06:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-22 04:06:43]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Nicolas Zea, 2014-08-22 17:37:55]

+TBR Bernhard for trivial browsertest fix.

[Nicolas Zea, 2014-08-22 17:37:58]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-08-22 17:39:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/454573002/50001

[commit-bot: I haz the power, 2014-08-22 18:33:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[commit-bot: I haz the power, 2014-08-22 18:37:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-22 18:37:27]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Nicolas Zea, 2014-08-22 19:29:08]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-08-22 19:30:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/454573002/50001

[commit-bot: I haz the power, 2014-08-22 19:40:07]

Committed patchset #4 (50001) as 291478