New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 639718 link

Starred by 15 users
Status: Fixed
Owner:
dpa...@chromium.org
Closed: Oct 2016
Cc:
█ krajshree@chromium.org
ajha@chromium.org
█ kavvaru@chromium.org
nyerramilli@chromium.org
█ egarciad@chromium.org
█ brajkumar@chromium.org
dbeam@chromium.org
dpa...@chromium.org
valdrin@google.com
█ dschuyler@chromium.org
michae...@chromium.org
█ ranjitkan@chromium.org
steve...@chromium.org
█ durga.behera@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 1
Type: Bug

Blocking:
issue 614588



Sign in to add a comment

Weird Actions Menu is seen in chrome://md-settings page

Project Member Reported by mm00333...@techmahindra.com, Aug 22 2016 
Version: 54.0.2835.0
OS: Ubuntu 14.04,Windows

What steps will reproduce the problem?
(1)Launch chrome -> Go to chrome://md-settings/onStartup page
(2)Now select 'Open a specific page or set of pages' option ->Select 'Use Current pages' Option ->Now Click on 'More Actions' Icon 
and observe (Please refer Video)

Expected:   Menu should not be seen overlapped with Actions Icon  
Actual: Instead  Weird Actions Menu is seen 

This is Regression Issue broken in M-54
will provide Bisect info soon
Actual_Menu.ogv
2.3 MB View Download
Actual_Menu.png
158 KB View Download

Comment 1 by ajha@chromium.org, Aug 22 2016

Labels: OS-Mac OS-Windows
Status: Untriaged (was: Unconfirmed)
Reproducible on 54.0.2835.0 on Mac OS 10.11.6 as well.

Comment 2 by mm00333...@techmahindra.com, Aug 22 2016

Labels: -Needs-Bisect hasbisect
Owner: steve...@chromium.org
Status: Assigned (was: Untriaged)
Manual good and Bad Builds:
Good Build: 54.0.2832.0
Bad Build:  54.0.2833.0

Below is the Tool Bisect info:

CHANGELOG  :
https://chromium.googlesource.com/chromium/src/+log/5b9a251b92cd505c03c401cf8b4f070698d833dd..feeb114ee91d6bb97f162f2c05f66f6d3ef84c15

Suspecting https://codereview.chromium.org/2256753003  from Changelog

@stevenjb: Please feel free to re-assign if its not related to your change
Expected_Menu.ogv
828 KB View Download

Comment 3 by mm00333...@techmahindra.com, Aug 22 2016 

Issue is seen for "Manage search Engines" Menu also

Comment 4 by ajha@chromium.org, Aug 22 2016

Cc: nyerramilli@chromium.org ranjitkan@chromium.org
 Issue 639721  has been merged into this issue.

Comment 5 by ajha@chromium.org, Aug 22 2016

Labels: Proj-MaterialDesign-WebUI

Comment 6 by ranjitkan@chromium.org, Aug 29 2016 

 Issue 641829  has been merged into this issue.

Comment 7 by steve...@chromium.org, Aug 29 2016

Labels: -Type-Bug-Regression Type-Bug
Summary: Weird Actions Menu is seen in chrome://md-settings page (was: Regression : Weird Actions Menu is seen in chrome://md-settings page)
MD-Settings is still in active development, we shouldn't be labeling them Regression yet.

Comment 8 by steve...@chromium.org, Sep 15 2016

Labels: -M-54 M-55

Comment 9 by steve...@chromium.org, Sep 15 2016

Cc: steve...@chromium.org
 Issue 644315  has been merged into this issue.

Comment 10 by steve...@chromium.org, Sep 15 2016 

 Issue 647023  has been merged into this issue.

Comment 11 by steve...@chromium.org, Sep 15 2016

Cc: egarciad@chromium.org dbeam@chromium.org valdrin@google.com dschuyler@chromium.org
 Issue 643394  has been merged into this issue.

Comment 12 by steve...@chromium.org, Sep 15 2016

Cc: michae...@chromium.org dpa...@chromium.org
Status: Started (was: Assigned)
From  issue 643394 , michaelpg@ wrote:

Quoting egarciad:

yeah that's a known issue
the problem is stacking contexts
every list item has transform:translate3d which creates a layer so those layer aren't on the same stacking context.  
and their z position depends on DOM ordering.
I would suggest moving the dropdown outside the list item.

Comment 13 by steve...@chromium.org, Sep 15 2016 

 Issue 642257  has been merged into this issue.

Comment 14 by steve...@chromium.org, Sep 15 2016 

I have been playing with a solution that moves the iron-dropdown from the iron-list entry to the parent element (i.e. the one containing the iron-list). This addresses the layering issues and has the advantage of not including the dropdown in every element.

The only hiccup I have founds so far is correctly anchoring the menu, but i'm sure that is solvable (but I have to run for now).

If anyone has any concerns or suggestions regarding that approach, please chime in.

Comment 15 by dpa...@chromium.org, Sep 15 2016 

For the certificate_manager, the dropdowns are per-item, but they are dom-ifed, so I don't think there is any performance benefit to gain by moving it one level up (see https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/certificate_manager_page/certificate_subentry.html?l=42). Similar can be done for search_engines_page, but currently those are not dom-ifed.

My concern is that the dropdowns always need to refer to the same model that is feeding its corresponding row, so moving it one level up sounds that it will complicate the row->dropdown interaction, but I might be wrong. I would only try that, if visually fixing the dropdown at its current location is not possible.

Comment 16 by valdrin@google.com, Sep 15 2016 

Pairing iron-list and iron-dropdown can be done like this: http://jsbin.com/jokifet/4/edit?html,output

You'd have a single dropdown & use the data binding to update its content ;)

Comment 17 by steve...@chromium.org, Sep 15 2016 

Yes, the problem with this approach is that it requires a bit of a re-factor.

Search engines I think actually benefits from the re-factor, here is the CL, LMKWYT:
https://codereview.chromium.org/2331423008/

I will now try a less invasive approach for startup urls.

Comment 18 by valdrin@google.com, Sep 15 2016 

here a more complete version with callbacks to actions: http://jsbin.com/jokifet/6/edit?html,output

Comment 19 by steve...@chromium.org, Sep 16 2016 

Yeah, so, that definitely works, but it is inconvenient when the menus are tied to the model associated with each list item. I am investigating potential alternatives.

Comment 20 by steve...@chromium.org, Sep 16 2016 

So, I tried a few different things, and read the discussion here:

https://github.com/PolymerElements/iron-list/issues/242

It doesn't look like there is a maintainable solution that doesn't involve moving the menus outside of the list items.

I don't feel too bad about this, because even employing dom-if around the menus, there is still some load time overhead for that for each item in the list. Extracting the menu into a separate element that exists at the same level as the iron-list seems reasonable, we only have a handful of them.

I'm out Friday but can discuss this at the standup on Monday.

Comment 21 by valdrin@google.com, Sep 16 2016 

If the lists are contained in another element that creates a new stacking context, you might still experience weird layouts. If that's the case, the best place where to host the dropdown is document.body

Comment 22 by dpa...@chromium.org, Sep 21 2016 

 Issue 644298  has been merged into this issue.

Comment 23 by dpa...@chromium.org, Sep 24 2016 

@valdrin: Putting the dropdown menu outside of the iron-list items has the drawback that the menu does no longer follow the item when scrolling, see https://jsfiddle.net/hpzfv3se/.

Repro:
1) Open a dropdown menu.
2) Scroll the page by grabbing the scrollbar with the mouse.
3) Observe that the dropdown menu neither closes itself, nor does it follow the list item it corresponds too.

Is there a known workaround for this?

Comment 24 by dpa...@chromium.org, Sep 26 2016 

I have found a workaround that makes the menus display correctly even if they reside inside the iron-list's items. This has the benefit of not suffering from the problems mentioned in comment#23 above.

Here is the minimal repro of the workaround, https://jsfiddle.net/3L3dw2Le/5/. I have not tried it within the Settings code yet, will do shortly, hopefully the workaround works there too.

Comment 25 by dpa...@chromium.org, Sep 26 2016 

It is easier to see the exact fix at https://codereview.chromium.org/2372693002. 

Basically my conclusions after investigating.

1) translate3d() does not put elements in a new stacking context, it simply changes the natural order (the order when no z-index is specified) by putting the last painted element in the front.
2) Increasing the z-index every time a menu is opened guarantees that the row that holds the menu is brought in the very front.

The only thing that still does not 100% right is that the right border of the menu is clipped (see attachment). This is probably a separate issue that I am still investigating.
fixed_menu.png
17.8 KB View Download

Comment 26 by valdrin@google.com, Sep 26 2016 

Wow...a `position: fixed` element inside a shadow root seems to be able to break the stacking context constraints *_* but that looks more like a bug on chrome I'm afraid..
Will look more into it..!

Comment 27 by valdrin@google.com, Sep 26 2016 

dpadad@ clipping is caused by overflow: auto + stacking context created by `will-change` done by the list (e.g. http://jsbin.com/yomuqoy/2/edit?html,output).
Unfortunately I don't see other ways around this issue. If you want to keep the dropdown inside the list, you can give a margin/padding to the list so it doesn't clip the dropdown (but that's hacky)..

The scrolling caused by grabbing the scrollbar is due to the fact that Settings scroll happens in another element and not document.body (see issue https://github.com/PolymerElements/iron-dropdown/issues/107).

Comment 28 by dpapad@google.com, Sep 26 2016 

@valdrin: Based on what I see in the new "layers" panel of the Devtools (see attachment), it does not look like a bug. All the items of the iron-list are in the same stacking context. And with the z-index manipulation you can bring whichever element at the very top of the context. In the attachment you can clearly see that the element that was clicked is in the top, even though it is not the last element on the list.

Regarding the issue of the menu being clipped by the iron-list boundary as a whole, need to find a workaround for that too, otherwise it looks very broken when clicking on the last elements of the list (see 2nd attachment).
layers.png
79.7 KB View Download
iron-list-clip.png
13.9 KB View Download

Comment 29 by steve...@chromium.org, Sep 26 2016

Owner: dpa...@chromium.org
->dpapad@ since he is actively working on it

Comment 30 by tbuck...@chromium.org, Sep 26 2016 

 Issue 649743  has been merged into this issue.

Comment 31 by dpa...@chromium.org, Sep 28 2016 

 Issue 651030  has been merged into this issue.

Comment 32 by dpa...@chromium.org, Sep 28 2016 

Status update: There is a candidate fix for this at https://codereview.chromium.org/2375493003. I am holding this fix up for a bit, because at the same time I am exploring a different (and possibly better) fix. Will keep this thread posted as progress is made.

Comment 33 by dpa...@chromium.org, Oct 7 2016 

So, I've been experimenting with implementing popup/action menus using a native <dialog> under the covers, which seems to be working great so far (CL still WIP) https://codereview.chromium.org/2402553002.

This approach can be used regardless of whether the action menus are 1 per list row, or 1 for the entire list. Will refine this approach tomorrow, but so far seems the most promising and simple of all previous approaches.

Comment 34 by dpa...@chromium.org, Oct 8 2016

Cc: krajshree@chromium.org
 Issue 648433  has been merged into this issue.
Project Member

Comment 35 by bugdroid1@chromium.org, Oct 15 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/46b5ca1056271d70ed5e264612aafe9fa948a2a4

commit 46b5ca1056271d70ed5e264612aafe9fa948a2a4
Author: dpapad <dpapad@chromium.org>
Date: Sat Oct 15 02:11:42 2016

MD Settings: Implementing modal popup/action menus.

Specifically, introducing a new settings-action-menu element implemented
as a native <dialog>. This solves a suite of problems that current action
menu implementations suffer from (cr-shared-menu, iron-dropdown), such as
scrolling, focus and z-index/stacking context problems.

Migrating two such menus (search engines, languages). Remaining menus
will be migrated in follow up CLs.

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

Review-Url: https://codereview.chromium.org/2402553002
Cr-Commit-Position: refs/heads/master@{#425534}

[modify] https://crrev.com/46b5ca1056271d70ed5e264612aafe9fa948a2a4/chrome/browser/resources/settings/compiled_resources2.gyp
[modify] https://crrev.com/46b5ca1056271d70ed5e264612aafe9fa948a2a4/chrome/browser/resources/settings/languages_page/compiled_resources2.gyp
[modify] https://crrev.com/46b5ca1056271d70ed5e264612aafe9fa948a2a4/chrome/browser/resources/settings/languages_page/languages_page.html
[modify] https://crrev.com/46b5ca1056271d70ed5e264612aafe9fa948a2a4/chrome/browser/resources/settings/languages_page/languages_page.js
[modify] https://crrev.com/46b5ca1056271d70ed5e264612aafe9fa948a2a4/chrome/browser/resources/settings/search_engines_page/compiled_resources2.gyp
[modify] https://crrev.com/46b5ca1056271d70ed5e264612aafe9fa948a2a4/chrome/browser/resources/settings/search_engines_page/search_engine_entry.html
[modify] https://crrev.com/46b5ca1056271d70ed5e264612aafe9fa948a2a4/chrome/browser/resources/settings/search_engines_page/search_engine_entry.js
[add] https://crrev.com/46b5ca1056271d70ed5e264612aafe9fa948a2a4/chrome/browser/resources/settings/settings_action_menu.html
[add] https://crrev.com/46b5ca1056271d70ed5e264612aafe9fa948a2a4/chrome/browser/resources/settings/settings_action_menu.js
[modify] https://crrev.com/46b5ca1056271d70ed5e264612aafe9fa948a2a4/chrome/browser/resources/settings/settings_resources.grd
[modify] https://crrev.com/46b5ca1056271d70ed5e264612aafe9fa948a2a4/chrome/test/data/webui/settings/cr_settings_browsertest.js
[modify] https://crrev.com/46b5ca1056271d70ed5e264612aafe9fa948a2a4/chrome/test/data/webui/settings/languages_page_browsertest.js
[modify] https://crrev.com/46b5ca1056271d70ed5e264612aafe9fa948a2a4/chrome/test/data/webui/settings/search_engines_page_test.js
[add] https://crrev.com/46b5ca1056271d70ed5e264612aafe9fa948a2a4/chrome/test/data/webui/settings/settings_action_menu_test.js

Comment 36 by dpa...@chromium.org, Oct 17 2016

Blocking: 614588
Project Member

Comment 40 by bugdroid1@chromium.org, Oct 18 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2a040cae1c0b74ab0cd585d40f45d4d266fdc3bc

commit 2a040cae1c0b74ab0cd585d40f45d4d266fdc3bc
Author: dpapad <dpapad@chromium.org>
Date: Tue Oct 18 20:47:46 2016

MD Settings: Migrating autofill cr-shared-menu to settings-action-menu.

 - Stop interpreting |activeAddress|, |activeCreditCard| as a signal to open
   dialogs.
 - Use |activeAddress|, |activeCreditCard| as the model for open dialogs and
   shared action menus.
 - Use data binding to hide/show action menu entries, instead of setting
   'hidden' programmatically.

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

Review-Url: https://codereview.chromium.org/2428733002
Cr-Commit-Position: refs/heads/master@{#426039}

[modify] https://crrev.com/2a040cae1c0b74ab0cd585d40f45d4d266fdc3bc/chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html
[modify] https://crrev.com/2a040cae1c0b74ab0cd585d40f45d4d266fdc3bc/chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js
[modify] https://crrev.com/2a040cae1c0b74ab0cd585d40f45d4d266fdc3bc/chrome/browser/resources/settings/passwords_and_forms_page/compiled_resources2.gyp
[modify] https://crrev.com/2a040cae1c0b74ab0cd585d40f45d4d266fdc3bc/chrome/test/data/webui/settings/settings_autofill_section_browsertest.js
Project Member

Comment 42 by bugdroid1@chromium.org, Oct 20 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7d40a5bcfefafa85f491f80b35af27c51d914de7

commit 7d40a5bcfefafa85f491f80b35af27c51d914de7
Author: stevenjb <stevenjb@chromium.org>
Date: Thu Oct 20 03:35:38 2016

MD Settings: Bluetooth: Use settings-action-menu dialog

Use settings-action-menu dialog instead of iron-dropdown in
bluetooth page.

This also introduces a helper behavior to settings_action_menu.

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

Review-Url: https://chromiumcodereview.appspot.com/2431583005
Cr-Commit-Position: refs/heads/master@{#426389}

[modify] https://crrev.com/7d40a5bcfefafa85f491f80b35af27c51d914de7/chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html
[modify] https://crrev.com/7d40a5bcfefafa85f491f80b35af27c51d914de7/chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js
[modify] https://crrev.com/7d40a5bcfefafa85f491f80b35af27c51d914de7/chrome/browser/resources/settings/bluetooth_page/compiled_resources2.gyp

Comment 45 by dpa...@chromium.org, Oct 21 2016

Status: Fixed (was: Started)
Closing this issue. Everything has been migrated, except one CrOS occurrence which is now tracked by https://bugs.chromium.org/p/chromium/issues/detail?id=658369.

List of bugs that have been addressed/fixed by this migration:
621731, 622657, 623525, 629460, 636282, 639718, 651308, 652646, 653265.
►

Sign in to add a comment