Use actual client width of list excluding scroll bar for adjusting column widths.

Use actual client width of list excluding scroll bar for adjusting column widths.

The clientWidth of the list can be narrowed by the thickness of the
vertical scrollbar. Existing logic in cr.ui.Table doesn't take this
into account, but assumes that it's always same to the table width.

This change will fix it by always referring the clientWidth of the
list, instead of that of the table. It also handles ON/OFF of the
vertical scrollbar which happens when:
- changing the content of list using batch update, or
- resizing the window


cr.ui.Table is currently only used by the Files app.
This change should not give any visible change in it by itself because
it currently uses overlay scroll bars instead of the normal ones.

This is a preparation before removing the individual implementation of
an overlay scroll bar in file_manager by this change:
  https://codereview.chromium.org/2724303003
After that point, we will display normal solid scroll bar after that,
iff. the native overlay scroll bar is disabled by about:flags.

TEST=tested manually by also patching crrev.com/2724303003
BUG=697057
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

[yamaguchi, 2017-03-10 02:28:06]

Description was changed from

==========
Reflect altered table client width to table header.

This change should will not give any visible change by itself because
the clientWidth of the list never change after redrawing.

This is a preparation before removing the individual implementation of
an overlay scroll bar in file_manager by this change:
  https://codereview.chromium.org/2724303003

We will display normal solid scroll bar after that, when the native
overlay scroll bar is disabled by about:flags. In that case the client
width of the table may be decreased by the width of the vertical scroll
bar after the list content changes.

TEST=none
BUG=697057
==========

to

==========
Reflect altered table client width to table header.

This change should will not give any visible change by itself because
the clientWidth of the list never change after redrawing.

This is a preparation before removing the individual implementation of
an overlay scroll bar in file_manager by this change:
  https://codereview.chromium.org/2724303003

We will display normal solid scroll bar after that, when the native
overlay scroll bar is disabled by about:flags. In that case the client
width of the table may be decreased by the width of the vertical scroll
bar after the list content changes.

TEST=none
BUG=697057
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[yamaguchi, 2017-03-10 02:28:12]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 02:28:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yamaguchi, 2017-03-10 02:34:35]

Description was changed from

==========
Reflect altered table client width to table header.

This change should will not give any visible change by itself because
the clientWidth of the list never change after redrawing.

This is a preparation before removing the individual implementation of
an overlay scroll bar in file_manager by this change:
  https://codereview.chromium.org/2724303003

We will display normal solid scroll bar after that, when the native
overlay scroll bar is disabled by about:flags. In that case the client
width of the table may be decreased by the width of the vertical scroll
bar after the list content changes.

TEST=none
BUG=697057
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Reflect altered table client width to table header.

This change should will not give any visible change by itself because
the clientWidth of the list never change after redrawing currently.

This is a preparation before removing the individual implementation of
an overlay scroll bar in file_manager by this change:
  https://codereview.chromium.org/2724303003

We will display normal solid scroll bar after that, when the native
overlay scroll bar is disabled by about:flags. In that case the client
width of the table may be decreased by the width of the vertical scroll
bar after the list content changes.

TEST=none
BUG=697057
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[yamaguchi, 2017-03-10 02:38:09]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 02:38:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yamaguchi, 2017-03-10 03:16:06]

yamaguchi@chromium.org changed reviewers:
+ fukino@chromium.org

[yamaguchi, 2017-03-10 03:16:06]

PTAL.

Do you think we should add unit test in browser_test FileTableTest?
I am afraid the change for the test might be a bot too large compared to the
impact of this code path.

[commit-bot: I haz the power, 2017-03-10 03:16:47]

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

[commit-bot: I haz the power, 2017-03-10 03:16:48]

Dry run: This issue passed the CQ dry run.

[yamaguchi, 2017-03-13 09:58:18]

Description was changed from

==========
Reflect altered table client width to table header.

This change should will not give any visible change by itself because
the clientWidth of the list never change after redrawing currently.

This is a preparation before removing the individual implementation of
an overlay scroll bar in file_manager by this change:
  https://codereview.chromium.org/2724303003

We will display normal solid scroll bar after that, when the native
overlay scroll bar is disabled by about:flags. In that case the client
width of the table may be decreased by the width of the vertical scroll
bar after the list content changes.

TEST=none
BUG=697057
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Reflect altered table client width to table list and header.

This change will prevent potential UI glitch that happens when the
vertical scrollbar appear or disappear when:
- changing the content of list using batch update, or
- resizing the window

cr.ui.Table is currently only used by the Files app.
This change should not give any visible change in it by itself because
it currently uses overlay scroll bars instead of the normal ones.

This is a preparation before removing the individual implementation of
an overlay scroll bar in file_manager by this change:
  https://codereview.chromium.org/2724303003
After that point, we will display normal solid scroll bar after that,
iff. the native overlay scroll bar is disabled by about:flags.

TEST=tested manually by also patching crrev.com/2724303003
BUG=697057
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[yamaguchi, 2017-03-13 10:00:32]

PTAL

Revised to put the change to the base class, reflecting the offline discussion.

[yamaguchi, 2017-03-13 10:03:03]

Description was changed from

==========
Reflect altered table client width to table list and header.

This change will prevent potential UI glitch that happens when the
vertical scrollbar appear or disappear when:
- changing the content of list using batch update, or
- resizing the window

cr.ui.Table is currently only used by the Files app.
This change should not give any visible change in it by itself because
it currently uses overlay scroll bars instead of the normal ones.

This is a preparation before removing the individual implementation of
an overlay scroll bar in file_manager by this change:
  https://codereview.chromium.org/2724303003
After that point, we will display normal solid scroll bar after that,
iff. the native overlay scroll bar is disabled by about:flags.

TEST=tested manually by also patching crrev.com/2724303003
BUG=697057
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Use the actual client width of the list excluding scroll bar for adjusting
column widths.

This change will prevent potential UI glitch that happens when the
vertical scrollbar appear or disappear when:
- changing the content of list using batch update, or
- resizing the window

cr.ui.Table is currently only used by the Files app.
This change should not give any visible change in it by itself because
it currently uses overlay scroll bars instead of the normal ones.

This is a preparation before removing the individual implementation of
an overlay scroll bar in file_manager by this change:
  https://codereview.chromium.org/2724303003
After that point, we will display normal solid scroll bar after that,
iff. the native overlay scroll bar is disabled by about:flags.

TEST=tested manually by also patching crrev.com/2724303003
BUG=697057
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[yamaguchi, 2017-03-13 10:04:07]

Description was changed from

==========
Use the actual client width of the list excluding scroll bar for adjusting
column widths.

This change will prevent potential UI glitch that happens when the
vertical scrollbar appear or disappear when:
- changing the content of list using batch update, or
- resizing the window

cr.ui.Table is currently only used by the Files app.
This change should not give any visible change in it by itself because
it currently uses overlay scroll bars instead of the normal ones.

This is a preparation before removing the individual implementation of
an overlay scroll bar in file_manager by this change:
  https://codereview.chromium.org/2724303003
After that point, we will display normal solid scroll bar after that,
iff. the native overlay scroll bar is disabled by about:flags.

TEST=tested manually by also patching crrev.com/2724303003
BUG=697057
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Use actual client width of list excluding scroll bar for adjusting column
widths.

This change will prevent potential UI glitch that happens when the
vertical scrollbar appear or disappear when:
- changing the content of list using batch update, or
- resizing the window

cr.ui.Table is currently only used by the Files app.
This change should not give any visible change in it by itself because
it currently uses overlay scroll bars instead of the normal ones.

This is a preparation before removing the individual implementation of
an overlay scroll bar in file_manager by this change:
  https://codereview.chromium.org/2724303003
After that point, we will display normal solid scroll bar after that,
iff. the native overlay scroll bar is disabled by about:flags.

TEST=tested manually by also patching crrev.com/2724303003
BUG=697057
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[fukino, 2017-03-14 03:12:11]

Hi yamaguchi@, could you clarify what is wrong with cr.ui.Table?
(Maybe passing clientWidth to normalizeWidths() is wrong since it does not
consider the scrollbar width?)

[yamaguchi, 2017-03-14 03:37:16]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 03:37:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yamaguchi, 2017-03-14 03:45:12]

On 2017/03/14 03:12:11, fukino wrote:
> Hi yamaguchi@, could you clarify what is wrong with cr.ui.Table?
> (Maybe passing clientWidth to normalizeWidths() is wrong since it does not
> consider the scrollbar width?)

Yes, that's right. That is the essential issue we're going to fix.
Added source code comment to describe it as well.

[fukino, 2017-03-14 04:02:46]

On 2017/03/14 03:45:12, yamaguchi wrote:
> On 2017/03/14 03:12:11, fukino wrote:
> > Hi yamaguchi@, could you clarify what is wrong with cr.ui.Table?
> > (Maybe passing clientWidth to normalizeWidths() is wrong since it does not
> > consider the scrollbar width?)
> 
> Yes, that's right. That is the essential issue we're going to fix.
> Added source code comment to describe it as well.

What is wrong and how it's going to be fixed are essential to understand the CL,
so describing them in the CL description will be helpful :)

[yamaguchi, 2017-03-14 04:22:13]

Description was changed from

==========
Use actual client width of list excluding scroll bar for adjusting column
widths.

This change will prevent potential UI glitch that happens when the
vertical scrollbar appear or disappear when:
- changing the content of list using batch update, or
- resizing the window

cr.ui.Table is currently only used by the Files app.
This change should not give any visible change in it by itself because
it currently uses overlay scroll bars instead of the normal ones.

This is a preparation before removing the individual implementation of
an overlay scroll bar in file_manager by this change:
  https://codereview.chromium.org/2724303003
After that point, we will display normal solid scroll bar after that,
iff. the native overlay scroll bar is disabled by about:flags.

TEST=tested manually by also patching crrev.com/2724303003
BUG=697057
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Use actual client width of list excluding scroll bar for adjusting column
widths.

The clientWidth of the list can be narrowed by the thickness of the
vertical scrollbar. Existing logic in cr.ui.Table doesn't take this
into account, but assumes that it's always same to the table width.

This change will fix it by always referring the clientWidth of the
list, instead of that of the table. It also handles ON/OFF of the
vertical scrollbar which happens when:
- changing the content of list using batch update, or
- resizing the window


cr.ui.Table is currently only used by the Files app.
This change should not give any visible change in it by itself because
it currently uses overlay scroll bars instead of the normal ones.

This is a preparation before removing the individual implementation of
an overlay scroll bar in file_manager by this change:
  https://codereview.chromium.org/2724303003
After that point, we will display normal solid scroll bar after that,
iff. the native overlay scroll bar is disabled by about:flags.

TEST=tested manually by also patching crrev.com/2724303003
BUG=697057
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[yamaguchi, 2017-03-14 04:22:26]

On 2017/03/14 04:02:46, fukino wrote:
> On 2017/03/14 03:45:12, yamaguchi wrote:
> > On 2017/03/14 03:12:11, fukino wrote:
> > > Hi yamaguchi@, could you clarify what is wrong with cr.ui.Table?
> > > (Maybe passing clientWidth to normalizeWidths() is wrong since it does not
> > > consider the scrollbar width?)
> > 
> > Yes, that's right. That is the essential issue we're going to fix.
> > Added source code comment to describe it as well.
> 
> What is wrong and how it's going to be fixed are essential to understand the
CL,
> so describing them in the CL description will be helpful :)

Done. ptal.

[commit-bot: I haz the power, 2017-03-14 05:02:35]

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

[commit-bot: I haz the power, 2017-03-14 05:02:37]

Dry run: This issue passed the CQ dry run.

[yamaguchi, 2017-03-14 09:07:30]

https://codereview.chromium.org/2745593003/diff/80001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/table.js (right):

https://codereview.chromium.org/2745593003/diff/80001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/table.js:383:
this.columnModel.normalizeWidths(this.list_.clientWidth);
cr.ui.table.TableColumnModel.normalizeWidths only changes the first column.
A good point with it is that it doesn't cause column size drift even when
repeatedly flipping the scroll bar on/off.
Downside of it is that the it shrinks the first column which shows the file
names (relatively important information) in case of the Files app.

Another option would be to resize all columns keeping the ratio.

[yamaguchi, 2017-03-24 12:47:14]

PTAL

As we discussed offline, we worried whether this change causes "drift" of the
column sizes when the scrollbar appear and disappear multiple times.
I confirmed it actually causes drift. The first column continues to shrink
little by little whereas some other columns becomes wider, if I resize the
window vertically to show/hide the vertical scrollbar repeatedly.
However, its root causes resides in the re-calculation of the column sizes
keeping the relative size. The same thing happens even with the existing logic
when resizing the window horizontally (which is more problematic as it happens
even when using overlay scrollbar).

Therefore I think we can go with the current logic.

[yamaguchi, 2017-03-28 14:02:08]

The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 14:02:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 15:02:57]

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

[commit-bot: I haz the power, 2017-03-28 15:02:58]

Dry run: This issue passed the CQ dry run.