MD Settings: Fix clipping of search bubbles by card boundary.

MD Settings: Fix clipping of search bubbles by card boundary.

This patch also seems to address the case where a settings card is briefly
animated to a 0px height after exiting an animation.

BUG=647487, 649174
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/8c4965f69d6de2d3c4ed9c6cd5b2ccbee1667cad
Cr-Commit-Position: refs/heads/master@{#430816}

[dpapad, 2016-09-21 23:06:34]

Description was changed from

==========
MD Settings: Fix clipping of search bubbles by card boundary.

BUG=647487
==========

to

==========
MD Settings: Fix clipping of search bubbles by card boundary.

BUG=647487
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-11-03 22:09:05]

dpapad@chromium.org changed reviewers:
+ dbeam@chromium.org, michaelpg@chromium.org

[dpapad, 2016-11-03 22:09:06]

See rationale at
https://bugs.chromium.org/p/chromium/issues/detail?id=647487#c15 on why I think
going forward with this CL is a step in the right direction (even though it
introduces some flashing when a subpage is exited).

[dpapad, 2016-11-03 22:09:39]

Description was changed from

==========
MD Settings: Fix clipping of search bubbles by card boundary.

BUG=647487
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Fix clipping of search bubbles by card boundary.

BUG=647487,647487
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-11-03 22:10:09]

Description was changed from

==========
MD Settings: Fix clipping of search bubbles by card boundary.

BUG=647487,647487
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Fix clipping of search bubbles by card boundary.

This patch also seems to address the case where a settings card is briefly
animated to a 0px height after exiting an animation.

BUG=647487,647487
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-11-03 23:01:21]

lgtm

[michaelpg, 2016-11-04 01:03:46]

mind if I take a look at this tomorrow before stamping?

[dpapad, 2016-11-04 01:05:39]

On 2016/11/04 at 01:03:46, michaelpg wrote:
> mind if I take a look at this tomorrow before stamping?

No problem. I was going to wait for your LG anyway.

[michaelpg, 2016-11-04 21:55:48]

lgtm

the flash of subpage content is unfortunate, but we can probably work around
that

more interesting is the highlight bubbles from other sections showing over top
of the expanding/collapsing card. doesn't seem to have been introduced by this
patch, though.

[dpapad, 2016-11-04 21:59:54]

On 2016/11/04 at 21:55:48, michaelpg wrote:
> lgtm
> 
> the flash of subpage content is unfortunate, but we can probably work around
that
> 
> more interesting is the highlight bubbles from other sections showing over top
of the expanding/collapsing card. doesn't seem to have been introduced by this
patch, though.

I think this is already filed as
https://bugs.chromium.org/p/chromium/issues/detail?id=640111.

[dpapad, 2016-11-04 22:30:15]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-11-04 22:30:57]

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

[dpapad, 2016-11-04 22:36:21]

The CQ bit was unchecked by dpapad@chromium.org

[dpapad, 2016-11-04 22:37:07]

Description was changed from

==========
MD Settings: Fix clipping of search bubbles by card boundary.

This patch also seems to address the case where a settings card is briefly
animated to a 0px height after exiting an animation.

BUG=647487,647487
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Fix clipping of search bubbles by card boundary.

This patch also seems to address the case where a settings card is briefly
animated to a 0px height after exiting an animation.

BUG=647487,649174
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-11-04 22:37:31]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-11-04 22:38:41]

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, 2016-11-04 23:18:11]

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

[commit-bot: I haz the power, 2016-11-04 23:18:12]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dpapad, 2016-11-05 01:13:37]

On 2016/11/04 at 23:18:12, commit-bot wrote:
> Try jobs failed on following builders:
>   linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

So the test failure is caused by this line
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...,
which makes me think that is a legit failure and don't really know how to
address it without a greater understanding of the code. Will investigate, but
also if you have suggestions, much appreciated.

[dpapad, 2016-11-07 23:40:11]

On 2016/11/05 at 01:13:37, dpapad wrote:
> On 2016/11/04 at 23:18:12, commit-bot wrote:
> > Try jobs failed on following builders:
> >   linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
> 
> So the test failure is caused by this line
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...,
which makes me think that is a legit failure and don't really know how to
address it without a greater understanding of the code. Will investigate, but
also if you have suggestions, much appreciated.

After investigating this failure, it seems that the test is flaky regardless of
my change. The reason is that the 'freeze-scroll' listener is sometimes
executing after the test has finished and <settings-main> has been removed from
the DOM, making offsetParent return null. Looking for a way to deflake this
test.

[dpapad, 2016-11-08 02:15:11]

On 2016/11/07 at 23:40:11, dpapad wrote:
> On 2016/11/05 at 01:13:37, dpapad wrote:
> > On 2016/11/04 at 23:18:12, commit-bot wrote:
> > > Try jobs failed on following builders:
> > >   linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
> > 
> > So the test failure is caused by this line
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...,
which makes me think that is a legit failure and don't really know how to
address it without a greater understanding of the code. Will investigate, but
also if you have suggestions, much appreciated.
> 
> After investigating this failure, it seems that the test is flaky regardless
of my change. The reason is that the 'freeze-scroll' listener is sometimes
executing after the test has finished and <settings-main> has been removed from
the DOM, making offsetParent return null. Looking for a way to deflake this
test.

Ok, so I clarified a few things.
1) The test does not fail on ToT because it actually does not trigger
fire('freeze-scroll'). All the early returns is collapseSection_, expandSection_
(in main_page_behavior.js) are triggering.
2) With my change, none of the early returns are triggering, so 'freeze-scroll'
event is fired. Even if we move the listeners inside settings-main on |this| the
test still fails, and from console.log debugging
it seems that the offsetParent is null, before <settings-main> is removed from
the DOM.

Q1: Why is the codepath in main_page_behaivor.js changing when I move the
'overflow' CSS rule?
Q2: Why is offsetParent null if we are still in the DOM?

[dpapad, 2016-11-08 23:30:01]

> Ok, so I clarified a few things.
> 1) The test does not fail on ToT because it actually does not trigger
fire('freeze-scroll'). All the early returns is collapseSection_, expandSection_
(in main_page_behavior.js) are triggering.
> 2) With my change, none of the early returns are triggering, so
'freeze-scroll' event is fired. Even if we move the listeners inside
settings-main on |this| the test still fails, and from console.log debugging
> it seems that the offsetParent is null, before <settings-main> is removed from
the DOM.
> 
> Q1: Why is the codepath in main_page_behaivor.js changing when I move the
'overflow' CSS rule?

|clientHeight| is different at
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin....
It was 0 before this CL, and it is a non-zero value with this CL, which causes
the canAnimateExpand() to return true now.

> Q2: Why is offsetParent null if we are still in the DOM?
Got to the bottom of this. The test logs were confusing because code triggered
by a previous test was executing while the next test was running. So at this
point it is pretty clear what is happening.
 1) This CL triggers an animation codepath during testing that was not triggered
before because clientHeight of settingsMain was zero.
 2) The async nature of 'freeze-scroll' events causes code to run after the test
have finished. 'freeze-scroll' listeners are not removed in detach()
unfortunately.

[dpapad, 2016-11-08 23:46:50]

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

[commit-bot: I haz the power, 2016-11-08 23:47:40]

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

[dpapad, 2016-11-08 23:48:07]

On 2016/11/08 at 23:30:01, dpapad wrote:
> > Ok, so I clarified a few things.
> > 1) The test does not fail on ToT because it actually does not trigger
fire('freeze-scroll'). All the early returns is collapseSection_, expandSection_
(in main_page_behavior.js) are triggering.
> > 2) With my change, none of the early returns are triggering, so
'freeze-scroll' event is fired. Even if we move the listeners inside
settings-main on |this| the test still fails, and from console.log debugging
> > it seems that the offsetParent is null, before <settings-main> is removed
from the DOM.
> > 
> > Q1: Why is the codepath in main_page_behaivor.js changing when I move the
'overflow' CSS rule?
> 
> |clientHeight| is different at
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin....
It was 0 before this CL, and it is a non-zero value with this CL, which causes
the canAnimateExpand() to return true now.
> 
> > Q2: Why is offsetParent null if we are still in the DOM?
> Got to the bottom of this. The test logs were confusing because code triggered
by a previous test was executing while the next test was running. So at this
point it is pretty clear what is happening.
>  1) This CL triggers an animation codepath during testing that was not
triggered before because clientHeight of settingsMain was zero.
>  2) The async nature of 'freeze-scroll' events causes code to run after the
test have finished. 'freeze-scroll' listeners are not removed in detach()
unfortunately.


@michaelpg: Uploaded CL that removes freeze-scroll listener on detached. The
previously failing tests pass locally with this change. PTAL

[michaelpg, 2016-11-08 23:50:55]

On 2016/11/08 02:15:11, dpapad wrote:
> On 2016/11/07 at 23:40:11, dpapad wrote:
> > On 2016/11/05 at 01:13:37, dpapad wrote:
> > > On 2016/11/04 at 23:18:12, commit-bot wrote:
> > > > Try jobs failed on following builders:
> > > >   linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
> > > 
> > > So the test failure is caused by this line
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...,
> which makes me think that is a legit failure and don't really know how to
> address it without a greater understanding of the code. Will investigate, but
> also if you have suggestions, much appreciated.
> > 
> > After investigating this failure, it seems that the test is flaky regardless
> of my change. The reason is that the 'freeze-scroll' listener is sometimes
> executing after the test has finished and <settings-main> has been removed
from
> the DOM, making offsetParent return null. Looking for a way to deflake this
> test.
> 
> Ok, so I clarified a few things.
> 1) The test does not fail on ToT because it actually does not trigger
> fire('freeze-scroll'). All the early returns is collapseSection_,
expandSection_
> (in main_page_behavior.js) are triggering.
> 2) With my change, none of the early returns are triggering, so
'freeze-scroll'
> event is fired. Even if we move the listeners inside settings-main on |this|
the
> test still fails, and from console.log debugging
> it seems that the offsetParent is null, before <settings-main> is removed from
> the DOM.
> 
> Q1: Why is the codepath in main_page_behaivor.js changing when I move the
> 'overflow' CSS rule?

Looking. Possibly it changes the outcome of a height check somewhere causing us
to
delay an animation.

> Q2: Why is offsetParent null if we are still in the DOM?

That implies it or a parent is hidden. :-\

[commit-bot: I haz the power, 2016-11-09 00:01:14]

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

[commit-bot: I haz the power, 2016-11-09 00:01:16]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[dpapad, 2016-11-09 00:18:09]

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

[commit-bot: I haz the power, 2016-11-09 00:18:42]

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

[dpapad, 2016-11-09 00:34:45]

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

[commit-bot: I haz the power, 2016-11-09 00:35:20]

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

[michaelpg, 2016-11-09 00:36:30]

lgtm

[dpapad, 2016-11-09 01:04:36]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-11-09 01:04:36]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2365453002/#ps100001
(title: "Use listen/unlisten instead.")

[commit-bot: I haz the power, 2016-11-09 01:05:07]

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, 2016-11-09 01:47:41]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-09 01:51:40]

Description was changed from

==========
MD Settings: Fix clipping of search bubbles by card boundary.

This patch also seems to address the case where a settings card is briefly
animated to a 0px height after exiting an animation.

BUG=647487,649174
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Fix clipping of search bubbles by card boundary.

This patch also seems to address the case where a settings card is briefly
animated to a 0px height after exiting an animation.

BUG=647487,649174
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/8c4965f69d6de2d3c4ed9c6cd5b2ccbee1667cad
Cr-Commit-Position: refs/heads/master@{#430816}
==========

[commit-bot: I haz the power, 2016-11-09 01:51:41]

Patchset 6 (id:??) landed as
https://crrev.com/8c4965f69d6de2d3c4ed9c6cd5b2ccbee1667cad
Cr-Commit-Position: refs/heads/master@{#430816}