FillLayout does not adjust dialog size if the child element's preferred size changes |
||
Issue descriptionI noticed this when I had a Checkbox as the only child element of a DialogDelegateView and used FillLayout as the LayoutManager. The checkbox's border isn't initialized until OnNativeThemeChanged is called, which affects its preferred size. The result is the attached screenshot, where the checkbox image is cut off. Merely changing to BoxLayout fixes this problem, so I think there's a bug with FillLayout somewhere.
,
Aug 30 2017
LayoutManager itself doesn't really update in response to views changing. It's expected that the View the LayoutManager is associated with calls to the LayoutManager at the appropriate point. I'm not sure you would see different behavior with BoxLayout. It may be BoxLayout is calling more than FillLayout, which is triggering something to update in the checkbox? That's just a guess.
,
Nov 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e8d9275821f0da2980a75510113fb7a31b1c89ed commit e8d9275821f0da2980a75510113fb7a31b1c89ed Author: Peter Boström <pbos@chromium.org> Date: Mon Nov 06 22:37:19 2017 Remove BoxLayout in ExternalProtocolDialog. This was used as a replacement for a FillLayout as this exercised a bug with views::Checkbox. views::Checkbox now includes the image height as well as the LabelButton insets and this item is no longer clipped with a FillLayout. Bug: chromium:760651 Change-Id: I285485f10ee02b4c7bacdc78de3fa32d95e36717 Reviewed-on: https://chromium-review.googlesource.com/755496 Reviewed-by: Bret Sepulveda <bsep@chromium.org> Commit-Queue: Peter Boström <pbos@chromium.org> Cr-Commit-Position: refs/heads/master@{#514265} [modify] https://crrev.com/e8d9275821f0da2980a75510113fb7a31b1c89ed/chrome/browser/ui/views/external_protocol_dialog.cc
,
Nov 6 2017
I believe this is related to GetHeightForWidth in LabelButton which previously didn't account for both image height + inset height, this wasn't a problem pre-Harmony as the image was smaller than the text, so it became tall enough anyways. I had a BoxLayout dialog that exercised this problem, but I think kVertical and kHorizontal makes use of GetHeightForWidth() differently, so kHorizontal might not have hit this path. I believe it might've gone away if I didn't use the default stretch preference of the layout, since that might call GetPreferredSize() before GetHeightForWidth(), which has mutable min_size_-related side effects. That's as far as my guesstimate goes, either way it looked fixed in tip of tree, so I removed the BoxLayout. Short version: views::Checkbox GetHeightForWidth() is correctly inferred with chromium:779102. |
||
►
Sign in to add a comment |
||
Comment 1 by bsep@chromium.org
, Aug 30 2017Owner: bsep@chromium.org
Status: Assigned (was: Available)