MD Settings: Fix rendering of popup menus inside iron-lists.

MD Settings: Fix rendering of popup menus inside iron-lists.

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

[dpapad, 2016-09-26 22:05:40]

Description was changed from

==========
MD Settings: Fix rendering of popup menus inside iron-lists.

BUG=639718
==========

to

==========
MD Settings: Fix rendering of popup menus inside iron-lists.

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

[stevenjb, 2016-09-26 22:51:59]

stevenjb@chromium.org changed reviewers:
+ stevenjb@chromium.org

[stevenjb, 2016-09-26 22:52:00]

lgtm w/ nits

https://codereview.chromium.org/2375493003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/list_item_behavior.js (right):

https://codereview.chromium.org/2375493003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/list_item_behavior.js:5: cr.define('settings',
function() {
Optional: It seems like this might be generally useful. Maybe move it out of the
namespace / directory and call it CrListItemBehavior?

https://codereview.chromium.org/2375493003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/list_item_behavior.js:11: var maxZIndex = 0;
nit: maxZIndex_ since it's private

https://codereview.chromium.org/2375493003/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js
(left):

https://codereview.chromium.org/2375493003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:15:
var maxZIndex = 0;
Rebase?

[dpapad, 2016-09-26 23:04:31]

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

[dpapad, 2016-09-26 23:04:32]

+dbeam

FYI, I am waiting on Steven's https://codereview.chromium.org/2338133011 to
land, because most likely there will be some conflicts (related to iron-list
scroll-target elements).

https://codereview.chromium.org/2375493003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/list_item_behavior.js (right):

https://codereview.chromium.org/2375493003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/list_item_behavior.js:5: cr.define('settings',
function() {
On 2016/09/26 at 22:52:00, stevenjb wrote:
> Optional: It seems like this might be generally useful. Maybe move it out of
the namespace / directory and call it CrListItemBehavior?

Prefer to wait until the 2nd client of this class outside of settings/ is found,
before promoting to a shared directory.

https://codereview.chromium.org/2375493003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/list_item_behavior.js:11: var maxZIndex = 0;
On 2016/09/26 at 22:52:00, stevenjb wrote:
> nit: maxZIndex_ since it's private

Done.

>Can this just be a member of the Polymer object instead?
Responding to the question from the other CL: This can't be moved on the Polymer
object itself. Apparently it is not equivalent to a static member variable.

https://codereview.chromium.org/2375493003/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js
(left):

https://codereview.chromium.org/2375493003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:15:
var maxZIndex = 0;
On 2016/09/26 at 22:52:00, stevenjb wrote:
> Rebase?

Done.

[dpapad, 2016-09-27 00:17:02]

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

[commit-bot: I haz the power, 2016-09-27 00:17:55]

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, 2016-09-27 03:30:08]

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

[commit-bot: I haz the power, 2016-09-27 03:30:09]

Dry run: 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...)

[stevenjb, 2016-09-27 16:43:30]

On 2016/09/26 23:04:32, dpapad wrote:
> +dbeam
> 
> FYI, I am waiting on Steven's https://codereview.chromium.org/2338133011 to
> land, because most likely there will be some conflicts (related to iron-list
> scroll-target elements).
> 
>
https://codereview.chromium.org/2375493003/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/settings/list_item_behavior.js (right):
> 
>
https://codereview.chromium.org/2375493003/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/list_item_behavior.js:5:
cr.define('settings',
> function() {
> On 2016/09/26 at 22:52:00, stevenjb wrote:
> > Optional: It seems like this might be generally useful. Maybe move it out of
> the namespace / directory and call it CrListItemBehavior?
> 
> Prefer to wait until the 2nd client of this class outside of settings/ is
found,
> before promoting to a shared directory.
> 
>
https://codereview.chromium.org/2375493003/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/list_item_behavior.js:11: var maxZIndex = 0;
> On 2016/09/26 at 22:52:00, stevenjb wrote:
> > nit: maxZIndex_ since it's private
> 
> Done.
> 
> >Can this just be a member of the Polymer object instead?
> Responding to the question from the other CL: This can't be moved on the
Polymer
> object itself. Apparently it is not equivalent to a static member variable.
> 
>
https://codereview.chromium.org/2375493003/diff/40001/chrome/browser/resource...
> File
> chrome/browser/resources/settings/search_engines_page/search_engine_entry.js
> (left):
> 
>
https://codereview.chromium.org/2375493003/diff/40001/chrome/browser/resource...
>
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:15:
> var maxZIndex = 0;
> On 2016/09/26 at 22:52:00, stevenjb wrote:
> > Rebase?
> 
> Done.

https://codereview.chromium.org/2338133011 has landed, FYI.

[Dan Beam, 2016-09-27 23:02:04]

https://codereview.chromium.org/2375493003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/list_item_behavior.js (right):

https://codereview.chromium.org/2375493003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/list_item_behavior.js:14: showActionMenu:
function() {
this really doesn't show any action menu.  it really just ensures this item is
on top?  can we rename it to something closer to what it's currently doing and
rename it later if it needs to be more generic?

https://codereview.chromium.org/2375493003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/list_item_behavior.js:22: };
can we make this static and private?

ListItemBehavior.maxZIndex_ = 0;

[dpapad, 2016-09-28 00:13:35]

FWIW, I am experimenting with a centralized approach for showing popup menus
(see https://codereview.chromium.org/2373793003). That approach works well so
far, and if
we decide to adopt it, the work in this CL (2375493003) will be thrown away.

The last thing I am figuring out is how to test a popup menu under the new
centralized
mechanism, since it breaks existing tests. Given that the new approach has a big
potential
benefit (performance, 1 menu template for N list item rows), I think it is
better to hold
up this fix, until we have a clear idea of our 2 options (let's call them
ListItemBehavior vs PopupMenuManager).

https://codereview.chromium.org/2375493003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/list_item_behavior.js (right):

https://codereview.chromium.org/2375493003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/list_item_behavior.js:14: showActionMenu:
function() {
On 2016/09/27 at 23:02:04, Dan Beam wrote:
> this really doesn't show any action menu.  it really just ensures this item is
on top?  can we rename it to something closer to what it's currently doing and
rename it later if it needs to be more generic?

Renamed.

https://codereview.chromium.org/2375493003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/list_item_behavior.js:22: };
On 2016/09/27 at 23:02:04, Dan Beam wrote:
> can we make this static and private?
> 
> ListItemBehavior.maxZIndex_ = 0;

Already responded to Steven's similar question. Tried this and it did not work
as expected (the counter is not unique among behavior instances).

[Dan Beam, 2016-09-28 01:32:07]

https://codereview.chromium.org/2375493003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/list_item_behavior.js (right):

https://codereview.chromium.org/2375493003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/list_item_behavior.js:22: };
On 2016/09/28 00:13:35, dpapad wrote:
> On 2016/09/27 at 23:02:04, Dan Beam wrote:
> > can we make this static and private?
> > 
> > ListItemBehavior.maxZIndex_ = 0;
> 
> Already responded to Steven's similar question. Tried this and it did not work
> as expected (the counter is not unique among behavior instances). 

I can see how this would look a little confusing.  What I meant was:

/** @polymerBehavior */
var ListItemBehavior = {
  ensureOnTop: function() {
    this.style.zIndex = ++ListItemBehavior.maxZIndex_;
  },
};

/** @private {number} */
ListItemBehavior.maxZIndex_ = 0;

where there's only 1, globally static max z-index.

[Dan Beam, 2016-09-28 01:33:21]

On 2016/09/28 00:13:35, dpapad wrote:
> FWIW, I am experimenting with a centralized approach for showing popup menus
> (see https://codereview.chromium.org/2373793003). That approach works well so
> far, and if
> we decide to adopt it, the work in this CL (2375493003) will be thrown away.
> 
> The last thing I am figuring out is how to test a popup menu under the new
> centralized
> mechanism, since it breaks existing tests. Given that the new approach has a
big
> potential
> benefit (performance, 1 menu template for N list item rows), I think it is
> better to hold
> up this fix, until we have a clear idea of our 2 options (let's call them
> ListItemBehavior vs PopupMenuManager).

do whatever you want here.  if this fix results in less bugs, it's fine by me. 
I can also deal with them for the next few days / week or however long it takes
you to investigate a shared-menu framework

[Dan Beam, 2016-09-28 01:33:32]

lgtm

[dpapad, 2016-09-28 01:50:32]

https://codereview.chromium.org/2375493003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/list_item_behavior.js (right):

https://codereview.chromium.org/2375493003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/list_item_behavior.js:22: };
On 2016/09/28 at 01:32:07, Dan Beam wrote:
> On 2016/09/28 00:13:35, dpapad wrote:
> > On 2016/09/27 at 23:02:04, Dan Beam wrote:
> > > can we make this static and private?
> > > 
> > > ListItemBehavior.maxZIndex_ = 0;
> > 
> > Already responded to Steven's similar question. Tried this and it did not
work
> > as expected (the counter is not unique among behavior instances). 
> 
> I can see how this would look a little confusing.  What I meant was:
> 
> /** @polymerBehavior */
> var ListItemBehavior = {
>   ensureOnTop: function() {
>     this.style.zIndex = ++ListItemBehavior.maxZIndex_;
>   },
> };
> 
> /** @private {number} */
> ListItemBehavior.maxZIndex_ = 0;
> 
> where there's only 1, globally static max z-index.

Done. Thanks for the clarification. I misunderstood the original suggestion.

[stevenjb, 2016-10-13 16:49:52]

https://codereview.chromium.org/2375493003/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/on_startup_page/startup_urls_page.html
(right):

https://codereview.chromium.org/2375493003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/on_startup_page/startup_urls_page.html:24: }
Is there a reason not to apply this to all .scroll-container divs?

https://codereview.chromium.org/2375493003/diff/120001/chrome/browser/resourc...
File
chrome/browser/resources/settings/search_engines_page/search_engines_list.html
(right):

https://codereview.chromium.org/2375493003/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/search_engines_page/search_engines_list.html:38:
}
Is it intentional that this is overflow-y and startup_urls_page is overflow?

[dpapad, 2016-10-13 17:04:00]

On 2016/10/13 at 16:49:52, stevenjb wrote:
>
https://codereview.chromium.org/2375493003/diff/120001/chrome/browser/resourc...
> File chrome/browser/resources/settings/on_startup_page/startup_urls_page.html
(right):
> 
>
https://codereview.chromium.org/2375493003/diff/120001/chrome/browser/resourc...
> chrome/browser/resources/settings/on_startup_page/startup_urls_page.html:24: }
> Is there a reason not to apply this to all .scroll-container divs?
> 
>
https://codereview.chromium.org/2375493003/diff/120001/chrome/browser/resourc...
> File
chrome/browser/resources/settings/search_engines_page/search_engines_list.html
(right):
> 
>
https://codereview.chromium.org/2375493003/diff/120001/chrome/browser/resourc...
>
chrome/browser/resources/settings/search_engines_page/search_engines_list.html:38:
}
> Is it intentional that this is overflow-y and startup_urls_page is overflow?

This CL is obsolete. The latest approach for fixing popup menus is in review at
https://codereview.chromium.org/2402553002.

[stevenjb, 2016-10-13 18:56:04]

Ugh, You know you can close these without deleting them :) Anyway, no
worries, sgtm.


On Thu, Oct 13, 2016 at 11:04 AM, dpapad@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> On 2016/10/13 at 16:49:52, stevenjb wrote:
> >
> https://codereview.chromium.org/2375493003/diff/120001/
> chrome/browser/resources/settings/on_startup_page/startup_urls_page.html
> > File chrome/browser/resources/settings/on_startup_page/
> startup_urls_page.html
> (right):
> >
> >
> https://codereview.chromium.org/2375493003/diff/120001/
> chrome/browser/resources/settings/on_startup_page/startup_urls_page.html#
> newcode24
> >
chrome/browser/resources/settings/on_startup_page/startup_urls_page.html:24:
> }
> > Is there a reason not to apply this to all .scroll-container divs?
> >
> >
> https://codereview.chromium.org/2375493003/diff/120001/
> chrome/browser/resources/settings/search_engines_page/
> search_engines_list.html
> > File
> chrome/browser/resources/settings/search_engines_page/
> search_engines_list.html
> (right):
> >
> >
> https://codereview.chromium.org/2375493003/diff/120001/
> chrome/browser/resources/settings/search_engines_page/
> search_engines_list.html#newcode38
> >
> chrome/browser/resources/settings/search_engines_page/
> search_engines_list.html:38:
> }
> > Is it intentional that this is overflow-y and startup_urls_page is
> overflow?
>
> This CL is obsolete. The latest approach for fixing popup menus is in
> review at
> https://codereview.chromium.org/2402553002.
>
> https://codereview.chromium.org/2375493003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dpapad, 2016-10-13 19:30:49]

Description was changed from

==========
MD Settings: Fix rendering of popup menus inside iron-lists.

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

to

==========
MD Settings: Fix rendering of popup menus inside iron-lists.

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