Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2737913002:
Fix missing buttons on the Edit Bookmark dialog. (Closed)
|
Created:
3 years, 9 months ago by tapted Modified:
3 years, 9 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix missing buttons on the Edit Bookmark dialog.
This regressed in r454823.
The buttons disappear because a spurious Layout() is triggered when
adding a LabelButton as an extra view on Windows. The GridLayout is
still being set up at this point, so the Layout attempt gets horribly
confused and thinks the button row only needs to be a few pixels high.
The Layout() results when the views::Label inside the dialog's extra
view does a PreferredSizeChanged() when shadows are added to the text on
Windows. MD text buttons do not have shadows, and Mac and Linux never
have text shadows, so they are not affected.
Fix by extending the scope of a bool that tracks when DialogClientView
is manipulating the view hierarchy, and ignore calls to
ChildPreferredSizeChanged() in that case.
BUG=699390
TEST=On Windows, right-click a bookmark and select "Edit". The dialog
should have buttons.
R=pkasting@chromium.org
Review-Url: https://codereview.chromium.org/2737913002 .
Cr-Commit-Position: refs/heads/master@{#455438}
Committed: https://chromium.googlesource.com/chromium/src/+/1ffcf3d8575a7cea0a759b7df23e5b45138ed0dd
Patch Set 1 #
Total comments: 5
Patch Set 2 : update comment #
Messages
Total messages: 18 (12 generated)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix BUG=699390 ========== to ========== Fix missing buttons on the Edit Bookmark dialog. The buttons disappear because a Layout() is triggered when adding a LabelButton as an extra view on Windows. The GridLayout is still being set up at this point, so the Layout attempt gets horribly confused and thinks the button row only needs to be a few pixels high. The Layout() results when the views::Label inside the dialog's extra view does a PreferredSizeChanged() when shadows are added to the text on Windows. MD text buttons do not have shadows, and Mac and Linux never have text shadows, so they are not affected. Fix by extending the scope of a bool that tracks when DialogClientView is manipulating the view hierarchy, and ignore calls to ChildPreferredSizeChanged() in that case. BUG=699390 ==========
tapted@chromium.org changed reviewers: + pkasting@chromium.org
Description was changed from ========== Fix missing buttons on the Edit Bookmark dialog. The buttons disappear because a Layout() is triggered when adding a LabelButton as an extra view on Windows. The GridLayout is still being set up at this point, so the Layout attempt gets horribly confused and thinks the button row only needs to be a few pixels high. The Layout() results when the views::Label inside the dialog's extra view does a PreferredSizeChanged() when shadows are added to the text on Windows. MD text buttons do not have shadows, and Mac and Linux never have text shadows, so they are not affected. Fix by extending the scope of a bool that tracks when DialogClientView is manipulating the view hierarchy, and ignore calls to ChildPreferredSizeChanged() in that case. BUG=699390 ========== to ========== Fix missing buttons on the Edit Bookmark dialog. This regressed in r454823. The buttons disappear because a spurious Layout() is triggered when adding a LabelButton as an extra view on Windows. The GridLayout is still being set up at this point, so the Layout attempt gets horribly confused and thinks the button row only needs to be a few pixels high. The Layout() results when the views::Label inside the dialog's extra view does a PreferredSizeChanged() when shadows are added to the text on Windows. MD text buttons do not have shadows, and Mac and Linux never have text shadows, so they are not affected. Fix by extending the scope of a bool that tracks when DialogClientView is manipulating the view hierarchy, and ignore calls to ChildPreferredSizeChanged() in that case. BUG=699390 ==========
Hi Peter, please take a look. This is to fix a bad dev blocker.. I want to add a test, but won't have time tonight :/. Too much time spent fighting Windows compile errors. (ended up just going back a few days in git). https://codereview.chromium.org/2737913002/diff/1/ui/views/window/dialog_clie... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2737913002/diff/1/ui/views/window/dialog_clie... ui/views/window/dialog_client_view.cc:244: Layout(); This can probably just be InvalidateLayout()... but it wasn't before the refactor, so there might be a more subtle reason not to do that.
LGTM https://codereview.chromium.org/2737913002/diff/1/ui/views/window/dialog_clie... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2737913002/diff/1/ui/views/window/dialog_clie... ui/views/window/dialog_client_view.cc:244: Layout(); On 2017/03/08 11:25:26, tapted wrote: > This can probably just be InvalidateLayout()... but it wasn't before the > refactor, so there might be a more subtle reason not to do that. Would that even do anything? IIRC, InvalidateLayout() doesn't force a future layout, it just says layouts should propagate to this object. https://codereview.chromium.org/2737913002/diff/1/ui/views/window/dialog_clie... File ui/views/window/dialog_client_view.h (right): https://codereview.chromium.org/2737913002/diff/1/ui/views/window/dialog_clie... ui/views/window/dialog_client_view.h:131: // and spurious triggers to Layout() from extra views. This comment is now overly specific, since the member affects more than ViewHierarchyChanged(). Maybe "Used to prevent unnecessary/potentially harmful changes during SetupLayout(). Everything will be manually updated afterwards."
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Fix missing buttons on the Edit Bookmark dialog. This regressed in r454823. The buttons disappear because a spurious Layout() is triggered when adding a LabelButton as an extra view on Windows. The GridLayout is still being set up at this point, so the Layout attempt gets horribly confused and thinks the button row only needs to be a few pixels high. The Layout() results when the views::Label inside the dialog's extra view does a PreferredSizeChanged() when shadows are added to the text on Windows. MD text buttons do not have shadows, and Mac and Linux never have text shadows, so they are not affected. Fix by extending the scope of a bool that tracks when DialogClientView is manipulating the view hierarchy, and ignore calls to ChildPreferredSizeChanged() in that case. BUG=699390 ========== to ========== Fix missing buttons on the Edit Bookmark dialog. This regressed in r454823. The buttons disappear because a spurious Layout() is triggered when adding a LabelButton as an extra view on Windows. The GridLayout is still being set up at this point, so the Layout attempt gets horribly confused and thinks the button row only needs to be a few pixels high. The Layout() results when the views::Label inside the dialog's extra view does a PreferredSizeChanged() when shadows are added to the text on Windows. MD text buttons do not have shadows, and Mac and Linux never have text shadows, so they are not affected. Fix by extending the scope of a bool that tracks when DialogClientView is manipulating the view hierarchy, and ignore calls to ChildPreferredSizeChanged() in that case. BUG=699390 TEST=On Windows, right-click a bookmark and select "Edit". The dialog should have buttons. ==========
Thanks Peter! https://codereview.chromium.org/2737913002/diff/1/ui/views/window/dialog_clie... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2737913002/diff/1/ui/views/window/dialog_clie... ui/views/window/dialog_client_view.cc:244: Layout(); On 2017/03/08 11:33:17, Peter Kasting wrote: > On 2017/03/08 11:25:26, tapted wrote: > > This can probably just be InvalidateLayout()... but it wasn't before the > > refactor, so there might be a more subtle reason not to do that. > > Would that even do anything? IIRC, InvalidateLayout() doesn't force a future > layout, it just says layouts should propagate to this object. Yeah it would just ensure a future call to Layout() at a higher level actually re-layouts this view. But I think Layout() here is bad, because it can't be coalesced. This probably got added because a button changed its text - maybe it was from a time the focused button had bold text or something. But in that case (e.g. tabbing between buttons) there are 2 buttons that need to be updated - they shouldn't both trigger a re-layout. But also finding who "should" be responsible for the final layout would require a lot of digging - maybe this is the right place after all :/ https://codereview.chromium.org/2737913002/diff/1/ui/views/window/dialog_clie... File ui/views/window/dialog_client_view.h (right): https://codereview.chromium.org/2737913002/diff/1/ui/views/window/dialog_clie... ui/views/window/dialog_client_view.h:131: // and spurious triggers to Layout() from extra views. On 2017/03/08 11:33:17, Peter Kasting wrote: > This comment is now overly specific, since the member affects more than > ViewHierarchyChanged(). > > Maybe "Used to prevent unnecessary/potentially harmful changes during > SetupLayout(). Everything will be manually updated afterwards." Done.
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2737913002/#ps20001 (title: "update comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Fix missing buttons on the Edit Bookmark dialog. This regressed in r454823. The buttons disappear because a spurious Layout() is triggered when adding a LabelButton as an extra view on Windows. The GridLayout is still being set up at this point, so the Layout attempt gets horribly confused and thinks the button row only needs to be a few pixels high. The Layout() results when the views::Label inside the dialog's extra view does a PreferredSizeChanged() when shadows are added to the text on Windows. MD text buttons do not have shadows, and Mac and Linux never have text shadows, so they are not affected. Fix by extending the scope of a bool that tracks when DialogClientView is manipulating the view hierarchy, and ignore calls to ChildPreferredSizeChanged() in that case. BUG=699390 TEST=On Windows, right-click a bookmark and select "Edit". The dialog should have buttons. ========== to ========== Fix missing buttons on the Edit Bookmark dialog. This regressed in r454823. The buttons disappear because a spurious Layout() is triggered when adding a LabelButton as an extra view on Windows. The GridLayout is still being set up at this point, so the Layout attempt gets horribly confused and thinks the button row only needs to be a few pixels high. The Layout() results when the views::Label inside the dialog's extra view does a PreferredSizeChanged() when shadows are added to the text on Windows. MD text buttons do not have shadows, and Mac and Linux never have text shadows, so they are not affected. Fix by extending the scope of a bool that tracks when DialogClientView is manipulating the view hierarchy, and ignore calls to ChildPreferredSizeChanged() in that case. BUG=699390 TEST=On Windows, right-click a bookmark and select "Edit". The dialog should have buttons. R=pkasting@chromium.org Review-Url: https://codereview.chromium.org/2737913002 . Cr-Commit-Position: refs/heads/master@{#455438} Committed: https://chromium.googlesource.com/chromium/src/+/1ffcf3d8575a7cea0a759b7df23e... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as 1ffcf3d8575a7cea0a759b7df23e5b45138ed0dd (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
