Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
933163002:
Remove default button highlighting from extension info dialog (Closed)
DescriptionRemove default button highlighting from extension info dialog
This dialog does not have a single call-to-action button. Additionally,
the existing highlight implementation is a side-effect of using
DialogClientView, which is not well suited to this container. This CL
changes NativeDialogContainer to use a regular ClientView, removing the
default highlighting from the dialog, and refactors the two containers
to use a common base class.
BUG=445695
Committed: https://crrev.com/b5bbc014ae5602c7333fe995619418846e418adb
Cr-Commit-Position: refs/heads/master@{#318206}
Patch Set 1 #Patch Set 2 : Move CreateClientView #
Total comments: 6
Patch Set 3 : Refactor out base dialog class #Patch Set 4 : Add comment #
Total comments: 1
Patch Set 5 : Update comment #
Total comments: 2
Patch Set 6 : Remove GetInitiallyFocusedView #Messages
Total messages: 19 (6 generated)
tsergeant@chromium.org changed reviewers: + sashab@chromium.org, tapted@chromium.org
New version of this bug, following on from https://crrev.com/865873005. See the bug for justification for the change in approach.
Looking good! Some stuff to think about :) https://codereview.chromium.org/933163002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/app_list/app_list_dialog_container.cc (right): https://codereview.chromium.org/933163002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/app_list/app_list_dialog_container.cc:153: class NativeDialogContainer : public views::DialogDelegateView { It feels like this and the other one are getting so similar now... I wonder if there's some way we can merge the common bits... https://codereview.chromium.org/933163002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/app_list/app_list_dialog_container.cc:163: ui::Accelerator escape(ui::VKEY_ESCAPE, ui::EF_NONE); Maybe add a comment here, // Since we are using a regular ClientView, we need to manually bind the escape key to close the dialog. https://codereview.chromium.org/933163002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/app_list/app_list_dialog_container.cc:172: bool AcceleratorPressed(const ui::Accelerator& accelerator) override { Why is this needed here, but not in AppListDialogContainer (which also closes the dialog on escape-press)? https://codereview.chromium.org/933163002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/app_list/app_list_dialog_container.cc:195: return new views::ClientView(widget, GetContentsView()); Does this auto-focus the first button in the dialog? I think for accessibility reasons we need this to happen. I get the feeling this logic is already in AppInfoDialogViews.
Updated! I've refactored the common parts of the two classes out into a base class (this is something I thought about doing yesterday anyway). https://codereview.chromium.org/933163002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/app_list/app_list_dialog_container.cc (right): https://codereview.chromium.org/933163002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/app_list/app_list_dialog_container.cc:172: bool AcceleratorPressed(const ui::Accelerator& accelerator) override { On 2015/02/18 22:16:46, sasha_b wrote: > Why is this needed here, but not in AppListDialogContainer (which also closes > the dialog on escape-press)? It wasn't needed because AppListDialogContainer binds escape to the close button, rather than to the container itself. I've normalized this anyway. https://codereview.chromium.org/933163002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/app_list/app_list_dialog_container.cc:195: return new views::ClientView(widget, GetContentsView()); On 2015/02/18 22:16:46, sasha_b wrote: > Does this auto-focus the first button in the dialog? I think for accessibility > reasons we need this to happen. I get the feeling this logic is already in > AppInfoDialogViews. Auto focusing is performed by ViewHierarchyChanged (line 182), so this behaviour isn't changed.
tsergeant@chromium.org changed reviewers: + benwells@chromium.org - tapted@chromium.org
Adding benwells@ as a reviewer and moving tapted@ to CC, since benwells originally reviewed this code.
sashab@chromium.org changed reviewers: + tapted@chromium.org - benwells@chromium.org
This lgtm, but see what benwells thinks of the code structure. When I originally wrote this it I think I had a common class but broke it up, but I there was a lot less common code in the parent class, so it might make more sense now. Either way the parent class might not make sense. https://codereview.chromium.org/933163002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/app_list/app_list_dialog_container.cc (right): https://codereview.chromium.org/933163002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/app_list/app_list_dialog_container.cc:56: // Base container for App List dialogs. Encases a content view in a modal dialog Not "App List" dialogs, maybe modal dialogs.
benwells@chromium.org changed reviewers: + benwells@chromium.org - tapted@chromium.org
Adding myself as reviewer since I've been asked to look. Is the description for this change still accurate?
On 2015/02/24 07:44:41, benwells (OOO to 24 Feb) wrote: > Adding myself as reviewer since I've been asked to look. > > Is the description for this change still accurate? I've added a note about the refactor to the description, but the remainder of the description is still accurate.
Overall this is a lovely change. Just one question... https://codereview.chromium.org/933163002/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/app_list/app_list_dialog_container.cc (right): https://codereview.chromium.org/933163002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/app_list/app_list_dialog_container.cc:91: int GetDialogButtons() const override { return ui::DIALOG_BUTTON_NONE; } I think this was only present for one of the dialog containers previously. Why didn't the other one need it before?
I've uploaded a new patch with the single-line change from below. https://codereview.chromium.org/933163002/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/app_list/app_list_dialog_container.cc (right): https://codereview.chromium.org/933163002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/app_list/app_list_dialog_container.cc:91: int GetDialogButtons() const override { return ui::DIALOG_BUTTON_NONE; } On 2015/02/26 01:27:29, benwells wrote: > I think this was only present for one of the dialog containers previously. Why > didn't the other one need it before? Practically, the reason NativeDialogContainer container had it was because without it the DialogClientView would add a weird empty footer to the dialog. Now that we're not using a DialogClientView, neither class needs it for that anymore. Instead, keeping it prevents the same crash as described in https://codereview.chromium.org/689833004/#msg10 In fact, doing it this way means I can get rid of the GetInitiallyFocusedView override on line 138. Doing it this way and telling the dialog it has no buttons to begin with makes more sense than stopping it from accessing buttons which don't exist.
nice, i like removing code we don't need. still lgtm.
The CQ bit was checked by tsergeant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org Link to the patchset: https://codereview.chromium.org/933163002/#ps100001 (title: "Remove GetInitiallyFocusedView")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/933163002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b5bbc014ae5602c7333fe995619418846e418adb Cr-Commit-Position: refs/heads/master@{#318206} |
