MD Settings: Fix search engines page popup menu.

MD Settings: Fix search engines page popup menu.

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

[dpapad, 2016-09-26 17:28:09]

Description was changed from

==========
MD Settings: Fix search engines page popup menu.

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

to

==========
MD Settings: Fix search engines page popup menu.

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

[dpapad, 2016-09-26 18:10:48]

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

[dpapad, 2016-09-26 18:10:49]

https://codereview.chromium.org/2372693002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engines_list.html
(right):

https://codereview.chromium.org/2372693002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engines_list.html:37:
<iron-list items="[[engines]]" selectable scroll-target="outer">
This fixes the clipping of the menu by the iron-list boundary.

[Dan Beam, 2016-09-26 18:12:32]

lgtm but is there a way to fix this for all lists rather than 1-by-1?

[stevenjb, 2016-09-26 18:13:24]

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

[stevenjb, 2016-09-26 18:13:25]

Are we going to need to make a similar change for every iron-list + popup menu?

If so, can we wrap it into a behavior, either the existing
cr_scrollable_behavior or something else?

[dpapad, 2016-09-26 18:46:44]

On 2016/09/26 at 18:13:25, stevenjb wrote:
> Are we going to need to make a similar change for every iron-list + popup
menu?
> 
> If so, can we wrap it into a behavior, either the existing
cr_scrollable_behavior or something else?

All iron-lists that have per-row popup menus need to be fixed in a similar way.
I still have not a clear idea on how to package that workaround in a re-usable
way, so here is my plan.

1) Land this fix for search engines popup menu.
2) Go through remaining iron-list instances in the codebase and find how many
hove per-row popup menus.
3) Depending on the number that I discover from 2 (startup URLs is the one I am
currently aware of), we can decide whether making the workaround re-usable vs
just inlining it is worth it.

[stevenjb, 2016-09-26 20:03:13]

*<iron-list> + <iron-dropdown>:*
startup_urls_page.html
bluetooth_device_dialog.html
bluetooth_page.html
search_engines_page.html
search_engines_list.html

*<iron-list> with no dropdown:*
edit_dictionary_page.html
add_languages_dialog.html
cr_network_list.html (internet page)
passwords_section.html




On Mon, Sep 26, 2016 at 12:46 PM, dpapad@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> On 2016/09/26 at 18:13:25, stevenjb wrote:
> > Are we going to need to make a similar change for every iron-list + popup
> menu?
> >
> > If so, can we wrap it into a behavior, either the existing
> cr_scrollable_behavior or something else?
>
> All iron-lists that have per-row popup menus need to be fixed in a similar
> way.
> I still have not a clear idea on how to package that workaround in a
> re-usable
> way, so here is my plan.
>
> 1) Land this fix for search engines popup menu.
> 2) Go through remaining iron-list instances in the codebase and find how
> many
> hove per-row popup menus.
> 3) Depending on the number that I discover from 2 (startup URLs is the one
> I am
> currently aware of), we can decide whether making the workaround re-usable
> vs
> just inlining it is worth it.
>
> https://codereview.chromium.org/2372693002/
>

-- 
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-09-26 21:26:36]

On 2016/09/26 at 20:03:13, stevenjb wrote:
> *<iron-list> + <iron-dropdown>:*
> startup_urls_page.html
> bluetooth_device_dialog.html
> bluetooth_page.html
> search_engines_page.html
> search_engines_list.html
> 
> *<iron-list> with no dropdown:*
> edit_dictionary_page.html
> add_languages_dialog.html
> cr_network_list.html (internet page)
> passwords_section.html
> 

Thanks for the data Steven. I talked with Dan and will be investigating how to
unify our codepaths for per-row popup action menus (currently we have both
cr-shared-menu and vanilla iron-dorpdown menus deployed), which will include the
zIndex workaround such that it does not have to be repeated.
Let me know if it is OK with you to land this CL as is. The investigation will
take some time (hopefully not too long though).

> 
> 
> 
> On Mon, Sep 26, 2016 at 12:46 PM, dpapad@chromium.org via
> codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:
> 
> > On 2016/09/26 at 18:13:25, stevenjb wrote:
> > > Are we going to need to make a similar change for every iron-list + popup
> > menu?
> > >
> > > If so, can we wrap it into a behavior, either the existing
> > cr_scrollable_behavior or something else?
> >
> > All iron-lists that have per-row popup menus need to be fixed in a similar
> > way.
> > I still have not a clear idea on how to package that workaround in a
> > re-usable
> > way, so here is my plan.
> >
> > 1) Land this fix for search engines popup menu.
> > 2) Go through remaining iron-list instances in the codebase and find how
> > many
> > hove per-row popup menus.
> > 3) Depending on the number that I discover from 2 (startup URLs is the one
> > I am
> > currently aware of), we can decide whether making the workaround re-usable
> > vs
> > just inlining it is worth it.
> >
> > https://codereview.chromium.org/2372693002/
> >
> 
> -- 
> 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-09-26 21:26:37]

On 2016/09/26 at 20:03:13, stevenjb wrote:
> *<iron-list> + <iron-dropdown>:*
> startup_urls_page.html
> bluetooth_device_dialog.html
> bluetooth_page.html
> search_engines_page.html
> search_engines_list.html
> 
> *<iron-list> with no dropdown:*
> edit_dictionary_page.html
> add_languages_dialog.html
> cr_network_list.html (internet page)
> passwords_section.html
> 

Thanks for the data Steven. I talked with Dan and will be investigating how to
unify our codepaths for per-row popup action menus (currently we have both
cr-shared-menu and vanilla iron-dorpdown menus deployed), which will include the
zIndex workaround such that it does not have to be repeated.
Let me know if it is OK with you to land this CL as is. The investigation will
take some time (hopefully not too long though).

> 
> 
> 
> On Mon, Sep 26, 2016 at 12:46 PM, dpapad@chromium.org via
> codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:
> 
> > On 2016/09/26 at 18:13:25, stevenjb wrote:
> > > Are we going to need to make a similar change for every iron-list + popup
> > menu?
> > >
> > > If so, can we wrap it into a behavior, either the existing
> > cr_scrollable_behavior or something else?
> >
> > All iron-lists that have per-row popup menus need to be fixed in a similar
> > way.
> > I still have not a clear idea on how to package that workaround in a
> > re-usable
> > way, so here is my plan.
> >
> > 1) Land this fix for search engines popup menu.
> > 2) Go through remaining iron-list instances in the codebase and find how
> > many
> > hove per-row popup menus.
> > 3) Depending on the number that I discover from 2 (startup URLs is the one
> > I am
> > currently aware of), we can decide whether making the workaround re-usable
> > vs
> > just inlining it is worth it.
> >
> > https://codereview.chromium.org/2372693002/
> >
> 
> -- 
> 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-09-26 22:43:14]

I moved the workaround in a behavior at
https://codereview.chromium.org/2375493003, fixing all iron-lists with per-row
popup menus. I have spotted a discrepancy though. The "scrollable" behavior adds
an "overflow-y: auto" which makes the popup menu to be clipped at the bottom
boundary of the scrollable. See http://imgur.com/a/jOjni, where it happens for
the bluetooth dialog (I have fixed it for the rest).

Let me know if the behavior approach looks better. If so I can land 2375493003
instead of this CL.

[stevenjb, 2016-09-26 22:45:20]

https://codereview.chromium.org/2372693002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js
(right):

https://codereview.chromium.org/2372693002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:15:
var maxZIndex = 0;
Can this just be a member of the Polymer object instead?

https://codereview.chromium.org/2372693002/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/search_engines_page/search_engines_list.html
(right):

https://codereview.chromium.org/2372693002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/search_engines_page/search_engines_list.html:37:
<iron-list items="[[engines]]" selectable scroll-target="outer">
On 2016/09/26 18:10:49, dpapad wrote:
> This fixes the clipping of the menu by the iron-list boundary.

Conveniently this should be redundant with the fix in
https://codereview.chromium.org/2338133011/

[dpapad, 2016-09-26 23:05:49]

FYI, retiring this CL in favor of https://codereview.chromium.org/2375493003
which applies the same workaround but within a behavior.

[stevenjb, 2016-10-13 16:43:26]

On 2016/09/26 at 23:05:49, dpapad wrote:
> FYI, retiring this CL in favor of https://codereview.chromium.org/2375493003
which applies the same workaround but within a behavior.

Can we close this?

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

On 2016/10/13 at 16:43:26, stevenjb wrote:
> On 2016/09/26 at 23:05:49, dpapad wrote:
> > FYI, retiring this CL in favor of https://codereview.chromium.org/2375493003
which applies the same workaround but within a behavior.
> 
> Can we close this?

I will delete this CL once the latest approach lands in the codebase, see
https://codereview.chromium.org/2402553002.

[dpapad, 2016-10-13 19:31:07]

Description was changed from

==========
MD Settings: Fix search engines page popup menu.

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

to

==========
MD Settings: Fix search engines page popup menu.

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