Issue metadata
Sign in to add a comment
|
Regression: Image is seen chopped on adding any extension from webstore |
||||||||||||||||||||||||||||||||||||||||
Issue descriptionVersion: 54.0.2829.0 dev OS: Ubuntu 14.04, windows Test URL: https://chrome.google.com/webstore/detail/high-contrast/djcfdncoelnlbldjfhinnjlhdjlikmph What steps will reproduce the problem? (1) Launch chrome and try adding any extension >> Now on add popup observe for icon Expected: Image should be fully seen in add pop up. Actual: Instead only some part of image is seen. This is regression issue broken in M54. Will provide bisect info soon.
,
Aug 16 2016
CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/6ced57adb01ce545d4426e35dcbe9ea64b7acb3a..4b70c528d7a6652e534bc5d9efd19d2d57d3fe4e Suspecting https://codereview.chromium.org/2188133002 from changelog. @tapted: Please confirm the issue and help in re-assigning if it is not related to your change.
,
Aug 16 2016
,
Aug 16 2016
Good Build: 54.0.2828.0 dev Bad Build: 54.0.2829.0 dev
,
Aug 16 2016
,
Aug 17 2016
,
Aug 17 2016
So the real reason behind the bug is that the scroll view and icon view overlap for the no webstore data case. This diagram- // No webstore data (all other types) // +--------------+------+ // | title | icon | // +--------------| | // | scroll_view | | // +--------------+------+ at https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc?q=extension_install_dialog_view.cc&sq=package:chromium&dr&l=202 is a bit misleading, since we give the scroll view a column span of 4 at https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc?q=extension_install_dialog_view.cc&sq=package:chromium&dr&l=293 which causes the scroll view and the icon to overlap. Changing the colspan to 1 for the no webstore data case also doesn't fix the problem, since the scroll layout is explicitly given a padding column to force the scollbar to the right edge of the dialog and this pushes the icon out of the dialog and it gets clipped. I think this overlapping was deliberate but http://crrev.com/2188133002 causes the scroll view to have an opaque background which obscures the icon. I think the right fix is to make the icon and scroll view to not overlap. I think we should follow one of the following layouts- 1) +--------------------+--------------+ | | | | title | icon | | | | | | | +--------------------+--------------+ | +-+ | | | | | | | scroll view | | | | | | | | | +-+ +-----------------------------------+ 2) +------------------------+--------------+ | | | | title | | | | | | | | +------------------------+ | | +-+ icon | | | | | | | | | | scroll view | | | | | | | | | | | | +-+ | +---------------------------------------+ Adding treib@ who wrote crrev.com/1237483005 and finnur@ who reviewed it for their thoughts. Also, we use views::GridLayout::USE_PREF for column sets but give the dialog a fixed width at https://cs.chromium.org/chromium/src/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc?dr&q=extension_install_dialog_view.cc&sq=package:chromium&l=375. Isn't that slightly wrong?
,
Aug 17 2016
In layout 1, the icon would push the scroll view down, so we'd get a big gap between the title and the "body", which would probably look awkward. In layout 2, the scroll view would get much narrower, which also isn't ideal, but maybe acceptable? I'm not a UX person. Not sure what the best solution is. I guess getting the icon to draw on top of the scroll view isn't an option?
,
Aug 17 2016
>>I guess getting the icon to draw on top of the scroll view isn't an option? I don't really understand scroll view internals but here's what tapted@ (currently OOO) who wrote http://crrev.com/2188133002 had to say regarding it - "ideally the icon and scroll view shouldn't overlap at all :/ (if it actually scrolls there will be bugs!)".
,
Aug 19 2016
Screenshots for https://codereview.chromium.org/2259723003. Screenshots with prefix old_ are those of the current master on Linux with https://codereview.chromium.org/2188133002 reverted. Screenshots with prefix new are those with my patch, again on Linux.
,
Aug 19 2016
,
Aug 19 2016
can you also post windows screenshots (or at least linux screenshots with the chrome theme rather than gtk-style buttons)?
,
Aug 23 2016
Windows screenshots. Will land this if there are no objections.
,
Aug 23 2016
I'd like someone from UI to sign off before you land this, since it does seem to make the UI a little bit heavier in some cases (e.g. set 1). But if they're happy, I have no objections.
,
Aug 24 2016
In the longer term with Harmony we'd like to move away from showing icons in dialogs that will be anchored to icons in the toolbar. But that's a ways out. So this seems like a good compromise for now to keep the icons fully visible and avoid clipping them.
,
Aug 24 2016
Yeah on Mac dev and canary, the MacViews finch experiment is on for 50% of the users, hence you may be seeing this. This CL should resolve it for MacViews as well.
,
Aug 25 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/25c3b927b343fb3388eb3f2eacd85e21e60ba9b8 commit 25c3b927b343fb3388eb3f2eacd85e21e60ba9b8 Author: karandeepb <karandeepb@chromium.org> Date: Thu Aug 25 02:18:51 2016 ExtensionInstallDialogView: Make scroll view and icon not overlap. r411889 introduced scrolling with layers and gave the scroll view in ExtensionInstallDialog an opaque background color. This caused the scroll view to obscure the icon for the cases where the extension prompt has no webstore data. This is because for this case, the scroll view and the icon view in the ExtensionInstallDialogView overlap. This CL fixes the regression by making the scroll view and icon view not overlap for this case. The scroll view is extended to span the full content width and a separator is added between the title and the scroll view if the prompt shows permissions. BUG= 638093 Review-Url: https://codereview.chromium.org/2259723003 Cr-Commit-Position: refs/heads/master@{#414259} [modify] https://crrev.com/25c3b927b343fb3388eb3f2eacd85e21e60ba9b8/chrome/browser/ui/views/extensions/extension_install_dialog_view.cc
,
Aug 25 2016
|
|||||||||||||||||||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||||||||||||||||||
Comment 1 by kavvaru@chromium.org
, Aug 16 2016