Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2345183002:
Views: Draw Textfield selected text in gray when top-level Widget loses focus.
|
Created:
4 years, 3 months ago by Patti Lor Modified:
4 years ago CC:
chromium-reviews, tfarina, James Su, shuchen+watch_chromium.org, yusukes+watch_chromium.org, nona+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionViews: Draw Textfield selected text in gray when top-level Widget loses focus.
When a views::Textfield was the last focused View before moving focus off of the
top-level views::Widget they belong to, continue to draw the text selection.
Currently the selection looks like it disappears when focus is moved away, which
can be confusing. In order to do this, a new FocusChangeListener method
OnFocusManagerDestroying was added in order to handle inconsistent destruction
order on Mac vs Windows/Linux.
Note this does not work for the Omnibox.
BUG=645338
TEST=(If testing on Mac, turn on #mac-views-webui-dialogs.) Type some text into
a Textfield in the HTTP authentication dialog, highlight it, and move focus to a
different app or to the desktop. The text selection just made should still be
visible (possibly in a different color). The selected text should also be
draggable with the mouse even if the Textfield doesn't have focus.
Patch Set 1 #Patch Set 2 : Fix up diff & add colors to Windows/Mac. #
Total comments: 8
Patch Set 3 : Refactor to use FocusChangeListener, avoid ClearSelection() in OnBlur(). #Patch Set 4 : Rebase. #Patch Set 5 : Fix NativeThemeAura, NativeThemeChanged, and OnMouseDragged. #Patch Set 6 : Avoid DCHECK with number of observers. #Patch Set 7 : Fix focus changing to other Views. #
Total comments: 12
Patch Set 8 : Review comments, split out bug fix for 331038. #Patch Set 9 : Fix tests failing on TYPE_CONTROL Widgets & method for Windows colors. #Patch Set 10 : Fix DCHECK error from Widgets being deleted with Textfields still listening to their FocusManager. #Patch Set 11 : Re-add missing Windows fix, revert accidental comment change. #Patch Set 12 : Fix TextfieldTest.DestroyingTextfieldFromOnKeyEvent. #Patch Set 13 : Get rid of debugging things, use hashdefine for OnFocusManagerDestroying Textfield code. #Patch Set 14 : Fix up colors for Linux. #
Total comments: 11
Patch Set 15 : Review comments. #Patch Set 16 : Get rid of OnFocusManagerDestroying() & fix tests by closing the Widget manually. #Patch Set 17 : Don't use unique_ptr for Textfields inside Widgets - destruction handled by Widget. #Patch Set 18 : Fix Windows. #Patch Set 19 : Rebase. #
Total comments: 11
Patch Set 20 : Review comments. #Patch Set 21 : Rebase. #Patch Set 22 : Fix rebase. #
Total comments: 5
Patch Set 23 : DCHECK no focus manager at destruction, add NativeViewHierarchyChanged. #Patch Set 24 : Flip if. #Patch Set 25 : Fix DCHECKs. #
Total comments: 15
Patch Set 26 : Review comments. #
Total comments: 6
Patch Set 27 : Rebase. #Patch Set 28 : Fix lifetime issues with BorderView's widget_ and address review comments. #
Total comments: 25
Patch Set 29 : Refactor to use SelectionController(Delegate). Unfinished! #Messages
Total messages: 176 (143 generated)
Description was changed from ========== Views: Draw selected text in the Omnibox/Textfields when focus moved off window. When a views::Textfield (or the OmniboxViewViews) was the last focused View before moving focus off of the views::Widget they belong to, continue to draw the text selection (usually) in a different color. Currently the selection seems to disappear when focus is moved away, which can be confusing for users. Also support the Omnibox for this as it saves the text selection information differently from a typical views::Textfield. BUG=645338 TEST=Type some text into the Omnibox/a regular views::Textfield, highlight it, and move focus to a different app. The text selection just made should still be visible. ========== to ========== Views: Draw selected text in the Omnibox/Textfields when focus moved off window. WIP! When a views::Textfield (or the OmniboxViewViews) was the last focused View before moving focus off of the views::Widget they belong to, continue to draw the text selection (usually) in a different color. Currently the selection seems to disappear when focus is moved away, which can be confusing for users. Also support the Omnibox for this as it saves the text selection information differently from a typical views::Textfield. BUG=645338 TEST=Type some text into the Omnibox/a regular views::Textfield, highlight it, and move focus to a different app. The text selection just made should still be visible. ==========
patricialor@chromium.org changed reviewers: + tapted@chromium.org
Hi Trent, PTAL at this WIP. I'm not sure about the implementation for this - mostly that putting checks for focus things inside OnPaint seems like it might add unnecessary overhead. I have tried making Textfield implement FocusChangeListener to set the flag when focus changes instead, but had trouble getting OnDidChangeFocus to be called (after adding Textfield as a focus listener using View::ViewHierarchyChanged()). Wanted to get your feedback before I went any further, WDYT? Thanks!
https://codereview.chromium.org/2345183002/diff/20001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2345183002/diff/20001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:58: const SkColor kDefaultUnfocusedSelectionBackgroundColor = 0xffdcdcdc; Will this be similar to SK_ColorGRAY anyway? (does anything end up using kDefaultSelectionBackgroundColor for selection_background_focused_color_? maybe it's fine just to use the same constant for both) https://codereview.chromium.org/2345183002/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_mac.mm (right): https://codereview.chromium.org/2345183002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_mac.mm:46: const SkColor kUnfocusedSelectedTextBackgroundColor = Can you use this? https://codereview.chromium.org/2345183002/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/2345183002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_win.cc:540: return system_colors_[COLOR_HIGHLIGHT]; This should maybe be the same as kColorId_TableSelectionBackgroundUnfocused (which is what native_theme_mac uses kUnfocusedSelectedTextBackgroundColor for already). https://codereview.chromium.org/2345183002/diff/20001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/20001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1013: ClearSelection(); There are some comments in http://crbug.com/331038 about ClearSelection() here, including "Not clearing that selection is critically important, it wasn't changed by accident. Re-adding that ClearSelection() call will break stuff." So, I don't think we can take this approach :/ I think we need to feed information about the reason for the focus change into Textfield. Can we leverage FocusManager::FocusChangeReason? See void FocusManager::SetFocusedViewWithReason(..) -- we can make Textfield a FocusChangeListener - it will receive an OnWillChangeFocus message before Blur() is called and it can save set some flag like change_selection_background_color_on_next_blur_ if it knows it's about to be blurerd due to a widget activation change. Note I'm not sure if FocusChangeReason currently includes "widget *de*activation -- we may need to add that. With the `change_selection_background_color_on_next_blur_` approach, OnBlur would then not call set_focused(false), but instead change the selection background color on render text -- rendertext wouldn't need changes. OnFocus would just have to always set the selection background color back to what it should be.
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Views: Draw selected text in the Omnibox/Textfields when focus moved off window. WIP! When a views::Textfield (or the OmniboxViewViews) was the last focused View before moving focus off of the views::Widget they belong to, continue to draw the text selection (usually) in a different color. Currently the selection seems to disappear when focus is moved away, which can be confusing for users. Also support the Omnibox for this as it saves the text selection information differently from a typical views::Textfield. BUG=645338 TEST=Type some text into the Omnibox/a regular views::Textfield, highlight it, and move focus to a different app. The text selection just made should still be visible. ========== to ========== Views: Draw selected text in grey for Textfields when focus moved off Widget. When a views::Textfield was the last focused View before moving focus off of the views::Widget they belong to, continue to draw the text selection. Currently the selection looks like it disappears when focus is moved away, which can be confusing for users. Also fix a bug where clicking a Textfield to focus inside already selected text would make a flash of the previously selected text appear before the selection was cleared in favor putting the cursor in the location where the click occurred. BUG=645338, 331038 TEST=Type some text into the Omnibox/the HTTP authentication dialog, highlight it, and move focus to a different app. The text selection just made should still be visible (possibly in a different color). The selected text should also be draggable with the mouse. ==========
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
groshaft867@gmail.com changed reviewers: + groshaft867@gmail.com
lgtm
groshaft867@gmail.com changed reviewers: + groshaft867@gmail.com
lgtm lgtm
lgtm lgtm
lgtm
lgtm lgtm
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #7 (id:90011) has been deleted
Description was changed from ========== Views: Draw selected text in grey for Textfields when focus moved off Widget. When a views::Textfield was the last focused View before moving focus off of the views::Widget they belong to, continue to draw the text selection. Currently the selection looks like it disappears when focus is moved away, which can be confusing for users. Also fix a bug where clicking a Textfield to focus inside already selected text would make a flash of the previously selected text appear before the selection was cleared in favor putting the cursor in the location where the click occurred. BUG=645338, 331038 TEST=Type some text into the Omnibox/the HTTP authentication dialog, highlight it, and move focus to a different app. The text selection just made should still be visible (possibly in a different color). The selected text should also be draggable with the mouse. ========== to ========== Views: Draw selected text in grey for Textfields when focus moved off Widget. WIP! This approach still has a bug where if the mouse is held down on an unfocused text selection, the text selection will disappear until OnMouseDragged is called or OnMouseReleased is called. This might be a limitation of OnWillChangeFocus/OnFocus/OnDidChangeFocus all being called before OnMousePressed. When a views::Textfield was the last focused View before moving focus off of the views::Widget they belong to, continue to draw the text selection. Currently the selection looks like it disappears when focus is moved away, which can be confusing. Also fix a bug where clicking a Textfield to focus inside already selected text would make a flash of the previously selected text appear before the selection was cleared in favor putting the cursor in the location where the click occurred. BUG=645338, 331038 TEST=Type some text into the Omnibox/the HTTP authentication dialog, highlight it, and move focus to a different app. The text selection just made should still be visible (possibly in a different color). The selected text should also be draggable with the mouse. ==========
The CQ bit was checked by patricialor@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...
patricialor@chromium.org changed reviewers: - groshaft867@gmail.com
Refactored! As discussed offline there is still a bug in this though - see the CL description (2nd paragraph). Not sure if this is OK; if it isn't it might be better to split up this CL to not fix the unwanted flash of selected text bug. https://codereview.chromium.org/2345183002/diff/20001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2345183002/diff/20001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:58: const SkColor kDefaultUnfocusedSelectionBackgroundColor = 0xffdcdcdc; On 2016/09/20 06:57:55, tapted wrote: > Will this be similar to SK_ColorGRAY anyway? (does anything end up using > kDefaultSelectionBackgroundColor for selection_background_focused_color_? maybe > it's fine just to use the same constant for both) It's a bit lighter (I just used the Mac default unfocused selection colour here). But yeah I think there are native_theme_*s for pretty much all the platforms, so will do as you said & deleted this :) https://codereview.chromium.org/2345183002/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_mac.mm (right): https://codereview.chromium.org/2345183002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_mac.mm:46: const SkColor kUnfocusedSelectedTextBackgroundColor = On 2016/09/20 06:57:55, tapted wrote: > Can you use this? Yes! Thanks for finding it, I totally missed it. https://codereview.chromium.org/2345183002/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/2345183002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_win.cc:540: return system_colors_[COLOR_HIGHLIGHT]; On 2016/09/20 06:57:55, tapted wrote: > This should maybe be the same as kColorId_TableSelectionBackgroundUnfocused > (which is what native_theme_mac uses kUnfocusedSelectedTextBackgroundColor for > already). Done. https://codereview.chromium.org/2345183002/diff/20001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/20001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1013: ClearSelection(); On 2016/09/20 06:57:55, tapted wrote: > There are some comments in http://crbug.com/331038 about ClearSelection() here, > including "Not clearing that selection is critically important, it wasn't > changed by accident. Re-adding that ClearSelection() call will break stuff." > So, I don't think we can take this approach :/ > > I think we need to feed information about the reason for the focus change into > Textfield. Can we leverage FocusManager::FocusChangeReason? > > See void FocusManager::SetFocusedViewWithReason(..) -- we can make Textfield a > FocusChangeListener - it will receive an OnWillChangeFocus message before Blur() > is called and it can save set some flag like > change_selection_background_color_on_next_blur_ if it knows it's about to be > blurerd due to a widget activation change. > > Note I'm not sure if FocusChangeReason currently includes "widget *de*activation > -- we may need to add that. > > With the `change_selection_background_color_on_next_blur_` approach, OnBlur > would then not call set_focused(false), but instead change the selection > background color on render text -- rendertext wouldn't need changes. OnFocus > would just have to always set the selection background color back to what it > should be. I don't think we need to change FocusChangeReason (unless that approach is preferred), since there's only one FocusManager per Widget (we can check the newly focused View is nullptr to determine if the Widget was deactivated, which is what I have in patchset 6). But I think adding a flag to RenderText isn't avoidable if we don't clear the text selection - there needs to be a way to distinguish between case B and C below: A: Textfield focused, draw text selection. B: Textfield unfocused, Widget focused. C: Textfield unfocused, Widget unfocused. Previously this was "draw it when you're focused" but that needs to change to "draw it when you're focused and when you were the last focused View with nothing else now focused", so RenderText needs to know focus as well as whether there is no other focused View in the Widget. The only other way I can think of to do this is to give it access to FocusManager, which is probably worse than adding a flag. As for the bug you linked, I ended up using the RenderText flag we were trying to avoid to just mark whether the text selection should be drawn at all times (not just when unfocused). Clearing the selection inside OnMousePressed() as per https://bugs.chromium.org/p/chromium/issues/detail?id=331038#c7 doesn't work because the RenderText/SelectionModel needs to have access to the selection to work out whether middle click should replace the existing selection and if greyed out text should be draggable (they use calls to GetRenderText()->IsPointInSelection()).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Is it possible to split out the fix for 331038? We should land that separately. Then I'll have to you to show me the drag behaviour -- maybe it's not so bad, or we can delay it. E.g. omnibox selection doesn't react when a window doesn't have focus, until a mouse up, which gives the window focus, which should update the selection colour. https://codereview.chromium.org/2345183002/diff/130001/ui/native_theme/native... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/2345183002/diff/130001/ui/native_theme/native... ui/native_theme/native_theme_win.cc:568: : COLOR_BTNFACE]; This comes up for kColorId_TreeSelectionBackgroundUnfocused andkColorId_TreeSelectionBackgroundUnfocused already -- let's add a helper function https://codereview.chromium.org/2345183002/diff/130001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/130001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:282: FocusManager* focus_manager = GetFocusManager(); comment why this might be null? https://codereview.chromium.org/2345183002/diff/130001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1084: else needs curlies (both parts) https://codereview.chromium.org/2345183002/diff/130001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1169: change_selection_background_color_on_next_blur_ = true; what happens if we try to gray it out immediately? (comment?) https://codereview.chromium.org/2345183002/diff/130001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1171: else if (focus_before == this && focus_after != nullptr) needs curlies due to comments https://codereview.chromium.org/2345183002/diff/130001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2345183002/diff/130001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:218: // View: there's a lot of this style still around - usually I just go with what's consistent in the file already. If we want to change a bunch at once, we should do a CL just with the comment changes.
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by patricialor@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...
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Views: Draw selected text in grey for Textfields when focus moved off Widget. WIP! This approach still has a bug where if the mouse is held down on an unfocused text selection, the text selection will disappear until OnMouseDragged is called or OnMouseReleased is called. This might be a limitation of OnWillChangeFocus/OnFocus/OnDidChangeFocus all being called before OnMousePressed. When a views::Textfield was the last focused View before moving focus off of the views::Widget they belong to, continue to draw the text selection. Currently the selection looks like it disappears when focus is moved away, which can be confusing. Also fix a bug where clicking a Textfield to focus inside already selected text would make a flash of the previously selected text appear before the selection was cleared in favor putting the cursor in the location where the click occurred. BUG=645338, 331038 TEST=Type some text into the Omnibox/the HTTP authentication dialog, highlight it, and move focus to a different app. The text selection just made should still be visible (possibly in a different color). The selected text should also be draggable with the mouse. ========== to ========== Views: Draw Textfield selected text in gray when top-level Widget loses focus. When a views::Textfield was the last focused View before moving focus off of the top-level views::Widget they belong to, continue to draw the text selection. Currently the selection looks like it disappears when focus is moved away, which can be confusing. In order to do this, a new FocusChangeListener method OnFocusManagerDestroying was added in order to handle inconsistent destruction order on Mac vs Windows/Linux. BUG=645338 TEST=If testing on Mac, turn on #mac-views-webui-dialogs. Type some text into the Omnibox/the HTTP authentication dialog, highlight it, and move focus to a different app or to the desktop. The text selection just made should still be visible (possibly in a different color). The selected text should also be draggable with the mouse even if the Textfield doesn't have focus. ==========
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Hi Trent, PTAL. Thanks! https://codereview.chromium.org/2345183002/diff/130001/ui/native_theme/native... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/2345183002/diff/130001/ui/native_theme/native... ui/native_theme/native_theme_win.cc:568: : COLOR_BTNFACE]; On 2016/10/13 06:21:35, tapted wrote: > This comes up for kColorId_TreeSelectionBackgroundUnfocused > andkColorId_TreeSelectionBackgroundUnfocused already -- let's add a helper > function Done. https://codereview.chromium.org/2345183002/diff/130001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/130001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:282: FocusManager* focus_manager = GetFocusManager(); On 2016/10/13 06:21:35, tapted wrote: > comment why this might be null? Done. https://codereview.chromium.org/2345183002/diff/130001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1084: else On 2016/10/13 06:21:35, tapted wrote: > needs curlies (both parts) Done. https://codereview.chromium.org/2345183002/diff/130001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1169: change_selection_background_color_on_next_blur_ = true; On 2016/10/13 06:21:35, tapted wrote: > what happens if we try to gray it out immediately? (comment?) Ah, I think that variable was there from your suggestion to avoid changing anything in the RenderText class, but I mentioned in my reply comment that I didn't think that was avoidable, so there isn't any need for it any more. Have deleted it. https://codereview.chromium.org/2345183002/diff/130001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1171: else if (focus_before == this && focus_after != nullptr) On 2016/10/13 06:21:35, tapted wrote: > needs curlies due to comments Done. https://codereview.chromium.org/2345183002/diff/130001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2345183002/diff/130001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:218: // View: On 2016/10/13 06:21:35, tapted wrote: > there's a lot of this style still around - usually I just go with what's > consistent in the file already. If we want to change a bunch at once, we should > do a CL just with the comment changes. Reverted in both the .h and .cc :)
Neat - I think this is on the right track. I need to take a deeper look, but a couple of things stood out. https://codereview.chromium.org/2345183002/diff/290001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1192: // Widget will be destroyed after the FocusManager. Make sure to remove this It might be a bug that Mac is different here, or we might just not test it, having no need for TYPE_CONTROL yet. Can we remove the #ifdef? https://codereview.chromium.org/2345183002/diff/290001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2345183002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:486: FocusManager* focus_manager_; I don't think OWNERS will like this :/ Let's see if we can find a way to remove it. Something interesting I saw: View already has a FocusManager member to deal with accelerators -- // Focus manager accelerators registered on. FocusManager* accelerator_focus_manager_; I don't know if reusing that is the right thing. But the hooks are probably similar. One option might be to have NativeViewHierarchyChanged call something like `OnWidgetChanging` instead of UnregisterAccelerators/RegisterPendingAccelerators and pass the old FocusManager (and the default View::OnWidgetChanging() does the accelerator management). We could also override NativeViewHierarchyChanged, it says " Overriding this method is useful for tracking which FocusManager manages this view." but doesn't give any way to obtain the old focus manager :/ Let's discuss this..
https://codereview.chromium.org/2345183002/diff/290001/ui/native_theme/native... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/2345183002/diff/290001/ui/native_theme/native... ui/native_theme/native_theme_win.cc:149: SkColor GetUnfocusedSelectionBackgroundColor( Can we make this a (const) member function of NativeThemeWin? we shouldn't have to pass arguments then (not |system_colors should be passed by const-reference, but that won't matter if we use a member function) https://codereview.chromium.org/2345183002/diff/290001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1043: GetSelectionBackgroundColor()); Can this move to OnWillChangeFocus? Just to keep the logic in one place.. https://codereview.chromium.org/2345183002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1181: else if (focus_before == this && focus_after != nullptr) { nit: we usually have the `else` on the same line as the closing brace. Perhaps we can break this up a bit with some early returns. E.g. if (focus_after != this && focus_before != this) return; SkColor selection_color = focus_after ? GetSelectionBackgroundColor() : GetUnfocusedSelectionBackgroundColor(); // Selection is drawn if |this| has focus, or the Widget loses activation, but // not if another View in this Widget is gaining focus. GetRenderText()->set_draw_text_selection(!focus_after || focus_after == this); GetRenderText()->(selection_color); https://codereview.chromium.org/2345183002/diff/290001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2345183002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:486: FocusManager* focus_manager_; On 2016/10/24 03:11:19, tapted wrote: > I don't think OWNERS will like this :/ Let's see if we can find a way to remove > it. > > Something interesting I saw: View already has a FocusManager member to deal with > accelerators -- > > // Focus manager accelerators registered on. > FocusManager* accelerator_focus_manager_; > > > I don't know if reusing that is the right thing. But the hooks are probably > similar. One option might be to have NativeViewHierarchyChanged call something > like `OnWidgetChanging` instead of > UnregisterAccelerators/RegisterPendingAccelerators and pass the old FocusManager > (and the default View::OnWidgetChanging() does the accelerator management). > > > We could also override NativeViewHierarchyChanged, it says " Overriding this > method is useful for tracking which FocusManager manages this view." but doesn't > give any way to obtain the old focus manager :/ > > Let's discuss this.. I thought more about this. I think the data member is OK, but we should point it out to OWNERS when going for owners review that it may duplicate a thing in the base class View. But I think there's a way to make it a bit neater. Rather than cleaning up in the destructor, and the extra checks in ViewHierarchyChanged() it should be enough to do something like void Textfield::NativeViewHierarchyChanged() [override] { if (focus_manager_) focus_manager_->RemoveFocusChangeListener(this); focus_manager_ = GetFocusManager(); if (focus_manager_) focus_manager_->AddFocusChangeListener(this); } With this - maybe it's possible to have TYPE_CONTROL Widgets inoke NativeViewHierarchyChanged() before they destroy the FocusManager - then we don't need OnFocusManagerDestroying() --it kinda seems like a bug that it has a different lifetime for TYPE_CONTROL - maybe we can fix it :)
The CQ bit was checked by patricialor@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...
Patchset #17 (id:330001) has been deleted
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by patricialor@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...
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by patricialor@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...
Patchset #20 (id:410001) has been deleted
Patchset #13 (id:250001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Views: Draw Textfield selected text in gray when top-level Widget loses focus. When a views::Textfield was the last focused View before moving focus off of the top-level views::Widget they belong to, continue to draw the text selection. Currently the selection looks like it disappears when focus is moved away, which can be confusing. In order to do this, a new FocusChangeListener method OnFocusManagerDestroying was added in order to handle inconsistent destruction order on Mac vs Windows/Linux. BUG=645338 TEST=If testing on Mac, turn on #mac-views-webui-dialogs. Type some text into the Omnibox/the HTTP authentication dialog, highlight it, and move focus to a different app or to the desktop. The text selection just made should still be visible (possibly in a different color). The selected text should also be draggable with the mouse even if the Textfield doesn't have focus. ========== to ========== Views: Draw Textfield selected text in gray when top-level Widget loses focus. When a views::Textfield was the last focused View before moving focus off of the top-level views::Widget they belong to, continue to draw the text selection. Currently the selection looks like it disappears when focus is moved away, which can be confusing. In order to do this, a new FocusChangeListener method OnFocusManagerDestroying was added in order to handle inconsistent destruction order on Mac vs Windows/Linux. Note this does not work for the Omnibox. BUG=645338 TEST=(If testing on Mac, turn on #mac-views-webui-dialogs.) Type some text into a Textfield in the HTTP authentication dialog, highlight it, and move focus to a different app or to the desktop. The text selection just made should still be visible (possibly in a different color). The selected text should also be draggable with the mouse even if the Textfield doesn't have focus. ==========
Hi Trent, PTAL & thanks for the help! I've updated the CL description to say that this won't work for the Omnibox (this is because it saves the selection separately on blur and the selection model doesn't have access to that) so a separate CL will be needed for that later on. Of course for Mac this doesn't matter until we make the TopChrome MacViews, but not sure how much this will matter for Win/Linux (I'm inclined to say "not really", but others might disagree). https://codereview.chromium.org/2345183002/diff/290001/ui/native_theme/native... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/2345183002/diff/290001/ui/native_theme/native... ui/native_theme/native_theme_win.cc:149: SkColor GetUnfocusedSelectionBackgroundColor( On 2016/10/24 06:04:16, tapted wrote: > Can we make this a (const) member function of NativeThemeWin? we shouldn't have > to pass arguments then (not |system_colors should be passed by const-reference, > but that won't matter if we use a member function) Done. https://codereview.chromium.org/2345183002/diff/290001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1043: GetSelectionBackgroundColor()); On 2016/10/24 06:04:16, tapted wrote: > Can this move to OnWillChangeFocus? Just to keep the logic in one place.. Done. https://codereview.chromium.org/2345183002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1181: else if (focus_before == this && focus_after != nullptr) { On 2016/10/24 06:04:16, tapted wrote: > nit: we usually have the `else` on the same line as the closing brace. Perhaps > we can break this up a bit with some early returns. E.g. > > if (focus_after != this && focus_before != this) > return; > > SkColor selection_color = focus_after > ? GetSelectionBackgroundColor() > : GetUnfocusedSelectionBackgroundColor(); > > // Selection is drawn if |this| has focus, or the Widget loses activation, but > // not if another View in this Widget is gaining focus. > GetRenderText()->set_draw_text_selection(!focus_after || focus_after == this); > GetRenderText()->(selection_color); Done, thanks! https://codereview.chromium.org/2345183002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1192: // Widget will be destroyed after the FocusManager. Make sure to remove this On 2016/10/24 03:11:19, tapted wrote: > It might be a bug that Mac is different here, or we might just not test it, > having no need for TYPE_CONTROL yet. Can we remove the #ifdef? Yes - OnViewHierarchyChanged will get called before the Mac code can get around to destroying the FocusManager, so removing the ifdef just means this will never be called on Mac (at least in the case where TYPE_CONTROL Widgets are being destroyed - there may be another case where this gets hit which I didn't think of). Done. https://codereview.chromium.org/2345183002/diff/290001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2345183002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:486: FocusManager* focus_manager_; On 2016/10/24 06:04:16, tapted wrote: > On 2016/10/24 03:11:19, tapted wrote: > > I don't think OWNERS will like this :/ Let's see if we can find a way to > remove > > it. > > > > Something interesting I saw: View already has a FocusManager member to deal > with > > accelerators -- > > > > // Focus manager accelerators registered on. > > FocusManager* accelerator_focus_manager_; > > > > > > I don't know if reusing that is the right thing. But the hooks are probably > > similar. One option might be to have NativeViewHierarchyChanged call something > > like `OnWidgetChanging` instead of > > UnregisterAccelerators/RegisterPendingAccelerators and pass the old > FocusManager > > (and the default View::OnWidgetChanging() does the accelerator management). > > > > > > We could also override NativeViewHierarchyChanged, it says " Overriding this > > method is useful for tracking which FocusManager manages this view." but > doesn't > > give any way to obtain the old focus manager :/ > > > > Let's discuss this.. > > I thought more about this. I think the data member is OK, but we should point it > out to OWNERS when going for owners review that it may duplicate a thing in the > base class View. > > But I think there's a way to make it a bit neater. Rather than cleaning up in > the destructor, and the extra checks in ViewHierarchyChanged() it should be > enough to do something like > > void Textfield::NativeViewHierarchyChanged() [override] { > if (focus_manager_) > focus_manager_->RemoveFocusChangeListener(this); > focus_manager_ = GetFocusManager(); > if (focus_manager_) > focus_manager_->AddFocusChangeListener(this); > } > > > With this - maybe it's possible to have TYPE_CONTROL Widgets inoke > NativeViewHierarchyChanged() before they destroy the FocusManager - then we > don't need OnFocusManagerDestroying() --it kinda seems like a bug that it has a > different lifetime for TYPE_CONTROL - maybe we can fix it :) The cleanup in the destructor actually didn't have anything to do with the TYPE_CONTROL things, but were actually for TextfieldTest.DestroyingTextfieldFromOnKeyEvent, which creates a custom Textfield controller that destroys the Textfield in its HandleKeyEvent() method. Now that I've looked at it again though it seems it makes more sense for it to be the responsibility of the programmer to ensure the Textfield is cleaned up properly if someone ever wants to make a TextfieldController that does that (since TextfieldDestroyerController calls OnBlur() manually before destruction to avoid the DCHECK in the Textfield destructor). I've replaced the stuff in the destructor with a call to remove the Textfield in the TextfieldDestroyerController class. As discussed offline, the TYPE_CONTROL Widgets not being destroyed properly in Aura was decided to be a problem with the test not calling Widget::CloseNow() to deal with it.
https://codereview.chromium.org/2345183002/diff/430001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/430001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:939: // focus, so don't bother listening if there is no Widget. This should still set focus_manager_ to nullptr, similar to the way view.cc does. (assuming it works) I think it would be cleaner to override NativeViewHierarchyChanged() rather than ViewHierarchyChanged(). Since the documentation for NativeViewHierarchyChanged even says "Overriding this method is useful for tracking which FocusManager manages this view." Since it's simpler, perhaps try the code fragment from before-- if (focus_manager_) focus_manager_->RemoveFocusChangeListener(this); focus_manager_ = GetfocusManager(); if (focus_manager_) focus_manager_->AddFocusChangeListener(this); https://codereview.chromium.org/2345183002/diff/430001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2345183002/diff/430001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:333: target_->parent()->RemoveChildView(target_.get()); This needs a comment (if it's still needed) - the ~View() destructor should be invoked on the next line, and it does RemoveChildView https://codereview.chromium.org/2345183002/diff/430001/ui/views/focus/focus_t... File ui/views/focus/focus_traversal_unittest.cc (right): https://codereview.chromium.org/2345183002/diff/430001/ui/views/focus/focus_t... ui/views/focus/focus_traversal_unittest.cc:140: ~BorderView() override { widget_->CloseNow(); } an OWNER will probably ask why this was necessary. Either comment here, or leave a note on the CL when you send it to review. https://codereview.chromium.org/2345183002/diff/430001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11_interactive_uitest.cc (right): https://codereview.chromium.org/2345183002/diff/430001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_x11_interactive_uitest.cc:274: } Something like `delete textfield` here still needs to work safely - I can't tell if this is related to weird stuff the test does
https://codereview.chromium.org/2345183002/diff/430001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11_interactive_uitest.cc (right): https://codereview.chromium.org/2345183002/diff/430001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_x11_interactive_uitest.cc:274: } On 2016/10/27 00:53:42, tapted wrote: > Something like `delete textfield` here still needs to work safely - I can't tell > if this is related to weird stuff the test does (maybe my first comment "this still needs to set focus_manager_ to nullptr" will fix this and we can revert the diff above)
Patchset #20 (id:450001) has been deleted
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks Trent, PTAL. https://codereview.chromium.org/2345183002/diff/430001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/430001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:939: // focus, so don't bother listening if there is no Widget. On 2016/10/27 00:53:42, tapted wrote: > This should still set focus_manager_ to nullptr, similar to the way view.cc > does. > > (assuming it works) I think it would be cleaner to override > NativeViewHierarchyChanged() rather than ViewHierarchyChanged(). Since the > documentation for NativeViewHierarchyChanged even says "Overriding this method > is useful for tracking which FocusManager manages this view." > > Since it's simpler, perhaps try the code fragment from before-- > > if (focus_manager_) > focus_manager_->RemoveFocusChangeListener(this); > focus_manager_ = GetfocusManager(); > if (focus_manager_) > focus_manager_->AddFocusChangeListener(this); > > Done (for setting focus_manager_ to nullptr). I think NativeViewHierarchyChanged isn't the right hook for this - view.h says "This method is invoked when the [...] and the view hierarchy has not changed", which sounds like not what we want. I double checked this on both Mac / Linux and it seems to match up - ViewHierarchyChanged is called when I open/close the HTTP authentication dialog, but not NativeViewHierarchyChanged. Leaving the rest of this method as is. https://codereview.chromium.org/2345183002/diff/430001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2345183002/diff/430001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:333: target_->parent()->RemoveChildView(target_.get()); On 2016/10/27 00:53:42, tapted wrote: > This needs a comment (if it's still needed) - the ~View() destructor should be > invoked on the next line, and it does RemoveChildView I added a comment, please let me know if it makes sense to you. This behaviour seems actually kinda weird to me - maybe it would be better to have a private View::ViewHierarchyChanged() which calls some sort of virtual OverriddenViewHierarchyChanged() which subclasses can optionally implement? https://codereview.chromium.org/2345183002/diff/430001/ui/views/focus/focus_t... File ui/views/focus/focus_traversal_unittest.cc (right): https://codereview.chromium.org/2345183002/diff/430001/ui/views/focus/focus_t... ui/views/focus/focus_traversal_unittest.cc:140: ~BorderView() override { widget_->CloseNow(); } On 2016/10/27 00:53:42, tapted wrote: > an OWNER will probably ask why this was necessary. Either comment here, or leave > a note on the CL when you send it to review. Done. https://codereview.chromium.org/2345183002/diff/430001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11_interactive_uitest.cc (right): https://codereview.chromium.org/2345183002/diff/430001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_x11_interactive_uitest.cc:274: } On 2016/10/27 00:54:34, tapted wrote: > On 2016/10/27 00:53:42, tapted wrote: > > Something like `delete textfield` here still needs to work safely - I can't > tell > > if this is related to weird stuff the test does > > (maybe my first comment "this still needs to set focus_manager_ to nullptr" will > fix this and we can revert the diff above) AFAICT I think |textfield| is deleted when View::DoRemoveChildView() goes out of scope - it gets put in the unique_ptr |view_to_be_deleted|.
https://codereview.chromium.org/2345183002/diff/430001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11_interactive_uitest.cc (right): https://codereview.chromium.org/2345183002/diff/430001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_x11_interactive_uitest.cc:274: } On 2016/10/30 23:26:04, Patti Lor wrote: > On 2016/10/27 00:54:34, tapted wrote: > > On 2016/10/27 00:53:42, tapted wrote: > > > Something like `delete textfield` here still needs to work safely - I can't > > tell > > > if this is related to weird stuff the test does > > > > (maybe my first comment "this still needs to set focus_manager_ to nullptr" > will > > fix this and we can revert the diff above) > > AFAICT I think |textfield| is deleted when View::DoRemoveChildView() goes out of > scope - it gets put in the unique_ptr |view_to_be_deleted|. Ah, the comment you just added makes it clear why this was necessary. This is actually a really interesting problem :/. Probably foremost, View::~View() shouldn't be invoking virtual methods directly or indirectly, but that's gonna be too hard to fix. I think we still need to mitigate this, but it's worth asking OWNERS for a preference. Alternatives are: (1) this (pathset 22) (con: might leave a trap lurking if someone tries to `delete textfield` (2) add additional handling in ~Textfield (e.g. // ~View() also does this, but Textfield is partially destroyed by then, and // ViewHierarchyChanged won't be invoked. if (parent_) parent_->RemoveChildView(this); (note there's also a thing in ~Textfield for the input method -- we should add a similar `DCHECK(!focus_manager_)` at the end of ~Textfield() as well. - (3) Modify view.cc to always popuplate and manage its focus_manager_ member (i.e. not just when accelerators are used). My preference is probably (2) - but also maybe that won't work. https://codereview.chromium.org/2345183002/diff/510001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/510001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:941: focus_manager_ = nullptr; Ah, so this doesn't look right without the other code fragment. Is there a reason not to use the simpler code fragment? Since, it seems like we also need handling in NativeViewHierarchyChanged (or a modification to view.cc). E.g. probably both ViewHierarchyChanged and NativeViewHierarchyChanged should call a helper method like UpdateFocusChangeListener() which won't have access to ViewHierarchyChangedDetails. (the reason it doesn't look right is that RemoveFocusChangeListener() isn't invoked. I think that's required before ~FocusManager, so instead of `focus_manager_ = nullptr;` it looks like `DCHECK(!focus_manager_)` should work on this line, but I'm less sure about whether that's valid :/. Anyway, that simpler code fragment should deal with these concerns). https://codereview.chromium.org/2345183002/diff/510001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2345183002/diff/510001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:336: // only View::ViewHierarchyChanged() is called. This is a great comment, and a nice find. Things like this are really hard to diagnose.
The CQ bit was checked by patricialor@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...
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #24 (id:550001) has been deleted
Patchset #23 (id:530001) has been deleted
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #23 (id:570001) has been deleted
The CQ bit was checked by patricialor@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...
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_...)
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Trent, thanks for the review! PTAL. https://codereview.chromium.org/2345183002/diff/430001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11_interactive_uitest.cc (right): https://codereview.chromium.org/2345183002/diff/430001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_x11_interactive_uitest.cc:274: } On 2016/10/31 00:30:25, tapted wrote: > On 2016/10/30 23:26:04, Patti Lor wrote: > > On 2016/10/27 00:54:34, tapted wrote: > > > On 2016/10/27 00:53:42, tapted wrote: > > > > Something like `delete textfield` here still needs to work safely - I > can't > > > tell > > > > if this is related to weird stuff the test does > > > > > > (maybe my first comment "this still needs to set focus_manager_ to nullptr" > > will > > > fix this and we can revert the diff above) > > > > AFAICT I think |textfield| is deleted when View::DoRemoveChildView() goes out > of > > scope - it gets put in the unique_ptr |view_to_be_deleted|. > > Ah, the comment you just added makes it clear why this was necessary. This is > actually a really interesting problem :/. Probably foremost, View::~View() > shouldn't be invoking virtual methods directly or indirectly, but that's gonna > be too hard to fix. > > I think we still need to mitigate this, but it's worth asking OWNERS for a > preference. Alternatives are: > (1) this (pathset 22) (con: might leave a trap lurking if someone tries to > `delete textfield` > (2) add additional handling in ~Textfield (e.g. > > // ~View() also does this, but Textfield is partially destroyed by then, and > // ViewHierarchyChanged won't be invoked. > if (parent_) > parent_->RemoveChildView(this); > > > (note there's also a thing in ~Textfield for the input method -- we should add a > similar `DCHECK(!focus_manager_)` at the end of ~Textfield() as well. > > - (3) Modify view.cc to always popuplate and manage its focus_manager_ member > (i.e. not just when accelerators are used). > > > My preference is probably (2) - but also maybe that won't work. (2) also sounds good to me - have tried this and it seems to work, so it is definitely a valid option. Will link this comment thread in an email to OWNERS when this CL is ready for them. Also kinda unrelated - thanks for the tip about not invoking virtual methods in destructors - I hadn't really known about that/why that was and looked it up :) https://codereview.chromium.org/2345183002/diff/510001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/510001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:941: focus_manager_ = nullptr; On 2016/10/31 00:30:25, tapted wrote: > Ah, so this doesn't look right without the other code fragment. > > Is there a reason not to use the simpler code fragment? Since, it seems like we > also need handling in NativeViewHierarchyChanged (or a modification to view.cc). > E.g. probably both ViewHierarchyChanged and NativeViewHierarchyChanged should > call a helper method like UpdateFocusChangeListener() which won't have access to > ViewHierarchyChangedDetails. > > (the reason it doesn't look right is that RemoveFocusChangeListener() isn't > invoked. I think that's required before ~FocusManager, so instead of > `focus_manager_ = nullptr;` it looks like `DCHECK(!focus_manager_)` should work > on this line, but I'm less sure about whether that's valid :/. Anyway, that > simpler code fragment should deal with these concerns). Oops, sorry for the misunderstanding - I thought you meant "use the other code fragment if switching to NativeViewHierarchyChanged() instead of ViewHierarchyChanged()." In any case, using this code snippet doesn't work - on Mac, the FocusManager is destroyed after the Textfield, so GetFocusManager() still returns a valid FocusManager and it never removes itself. On Linux, GetWidget() returns null when ViewHierarchyChanged is called on the remove case and it sets focus_manager_ to null and never removes itself there either. (On Linux, this only happened with the Omnibox - maybe it only affects top-level Widgets? Didn't investigate further.) I've left this code kind of as is minus the focus_manager = null plus also calling it in NativeViewHierarchyChanged(), but if we wanted to use the code you posted, we could alternatively re-add that OnFocusManagerDestroying() notification, this time, passing in the FocusManager as an argument, so this means we can also get rid of |focus_manager_|. Not sure what you would prefer. https://codereview.chromium.org/2345183002/diff/510001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2345183002/diff/510001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:336: // only View::ViewHierarchyChanged() is called. On 2016/10/31 00:30:25, tapted wrote: > This is a great comment, and a nice find. Things like this are really hard to > diagnose. Thank you! :))
lgtm with some tweaks - let's ask OWNERS. I think I'd prefer ~Textfield() to call parent_->Remove(..), but that might be controversial. https://codereview.chromium.org/2345183002/diff/510001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/510001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:941: focus_manager_ = nullptr; On 2016/11/02 03:19:48, Patti Lor wrote: > On 2016/10/31 00:30:25, tapted wrote: > > Ah, so this doesn't look right without the other code fragment. > > > > Is there a reason not to use the simpler code fragment? Since, it seems like > we > > also need handling in NativeViewHierarchyChanged (or a modification to > view.cc). > > E.g. probably both ViewHierarchyChanged and NativeViewHierarchyChanged should > > call a helper method like UpdateFocusChangeListener() which won't have access > to > > ViewHierarchyChangedDetails. > > > > (the reason it doesn't look right is that RemoveFocusChangeListener() isn't > > invoked. I think that's required before ~FocusManager, so instead of > > `focus_manager_ = nullptr;` it looks like `DCHECK(!focus_manager_)` should > work > > on this line, but I'm less sure about whether that's valid :/. Anyway, that > > simpler code fragment should deal with these concerns). > > Oops, sorry for the misunderstanding - I thought you meant "use the other code > fragment if switching to NativeViewHierarchyChanged() instead of > ViewHierarchyChanged()." > > In any case, using this code snippet doesn't work - on Mac, the FocusManager is > destroyed after the Textfield, so GetFocusManager() still returns a valid > FocusManager and it never removes itself. On Linux, GetWidget() returns null > when ViewHierarchyChanged is called on the remove case and it sets > focus_manager_ to null and never removes itself there either. (On Linux, this > only happened with the Omnibox - maybe it only affects top-level Widgets? Didn't > investigate further.) > > I've left this code kind of as is minus the focus_manager = null plus also > calling it in NativeViewHierarchyChanged(), but if we wanted to use the code you > posted, we could alternatively re-add that OnFocusManagerDestroying() > notification, this time, passing in the FocusManager as an argument, so this > means we can also get rid of |focus_manager_|. Not sure what you would prefer. This looks OK. It might be nice to have GetWidget() (and GetFocusManager()) return null during Widget Close, but that would affect a lot of things. https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:957: AddOrRemoveAsFocusChangeListener(true); I think calling with `true` should always behave as if it was called with `false` prior. https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1885: return; Comment when this can occur? https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1888: } else if (focus_manager_) { I think this block can just go at the start of the method, then we don't have to call with false prior to the calling with true. i.e. if (focus_manager_) { focus_manager_->RemoveFocusChangeListener(this); focus_manager = nullptr; } if (add_as_listener) { focus_manager_ = GetFocusManager(); if (!focus_manager) { // This can happen when XXX. return; } focus_manager_->AddFocusChangeListener(this); } https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:381: void AddOrRemoveAsFocusChangeListener(bool add_as_listener); It's generally nice to have a `bool` argument read unambiguously even if you don't know the name of the argument. E.g. I'd suggest ObserveWidgetFocusChanges(true) OWNERS might have other ideas though :/
ooer we should add some test coverage for this too. But let's consult OWNERS to make sure the approach is sound first. It's will probably need something in widget_interactive_uitest.cc https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:941: if (!GetWidget()) actually I think this `if` can be omitted. -- we can rely on GetFocusManager will returning null. Then we don't have to worry about the case that focus_manager_ might be non-nil here https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:949: if (!GetWidget()) { same here. In fact I think we can just make this entire method. ObserveWidgetFocusChanges(true); This should be called only rarely, and it's fine to unsubscribe, then re-subscribe to the same FocusManager https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1885: return; On 2016/11/02 03:45:06, tapted wrote: > Comment when this can occur? (btw, I'm guessing this can occur only when Widget is null, or when Widget::GetTopLevelWidget() returns null (i.e. for a TYPE_CONTROL widget not in a parent Widget) - is that right?). If so, we can also say that we would be expecting a call to NativeViewHierarchyChanged() to follow.
Patchset #26 (id:650001) has been deleted
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hey Trent, thanks for the lgtm - I added a #ifdef + comment in focus_traversal_unittest.cc though, please take a look at that first! This was to address a flaky segfault that would happen on Mac every so often running those tests. Thanks. https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:941: if (!GetWidget()) On 2016/11/02 03:59:14, tapted wrote: > actually I think this `if` can be omitted. -- we can rely on GetFocusManager > will returning null. Then we don't have to worry about the case that > focus_manager_ might be non-nil here Done. https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:949: if (!GetWidget()) { On 2016/11/02 03:59:14, tapted wrote: > same here. In fact I think we can just make this entire method. > > ObserveWidgetFocusChanges(true); > > This should be called only rarely, and it's fine to unsubscribe, then > re-subscribe to the same FocusManager Done. https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:957: AddOrRemoveAsFocusChangeListener(true); On 2016/11/02 03:45:06, tapted wrote: > I think calling with `true` should always behave as if it was called with > `false` prior. Done. https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1885: return; On 2016/11/02 03:59:14, tapted wrote: > On 2016/11/02 03:45:06, tapted wrote: > > Comment when this can occur? > > (btw, I'm guessing this can occur only when Widget is null, or when > Widget::GetTopLevelWidget() returns null (i.e. for a TYPE_CONTROL widget not in > a parent Widget) - is that right?). If so, we can also say that we would be > expecting a call to NativeViewHierarchyChanged() to follow. Yeah, this happens when there's a TYPE_CONTROL Widget. I don't think it matters if it has a parent Widget? I haven't really investigated that. I did add a comment with info about the NativeViewHierarchyChanged() call though. https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1888: } else if (focus_manager_) { On 2016/11/02 03:45:06, tapted wrote: > I think this block can just go at the start of the method, then we don't have to > call with false prior to the calling with true. i.e. > > if (focus_manager_) { > focus_manager_->RemoveFocusChangeListener(this); > focus_manager = nullptr; > } > > if (add_as_listener) { > focus_manager_ = GetFocusManager(); > if (!focus_manager) { > // This can happen when XXX. > return; > } > focus_manager_->AddFocusChangeListener(this); > } Done. https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:381: void AddOrRemoveAsFocusChangeListener(bool add_as_listener); On 2016/11/02 03:45:06, tapted wrote: > It's generally nice to have a `bool` argument read unambiguously even if you > don't know the name of the argument. E.g. I'd suggest > > ObserveWidgetFocusChanges(true) > > > OWNERS might have other ideas though :/ Done.
The #ifdef doesn't look right - there might be a bug. Do you have an `asan = true` build on hand that generated the segfault? I'll take a look. https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:941: if (!GetWidget()) On 2016/11/07 05:34:26, Patti Lor wrote: > On 2016/11/02 03:59:14, tapted wrote: > > actually I think this `if` can be omitted. -- we can rely on GetFocusManager > > will returning null. Then we don't have to worry about the case that > > focus_manager_ might be non-nil here > > Done. ping? (this should be redundant with the changes in ObserveWidgetFocusChanges) But also I think it's important to be calling RemoveFocusChangeListener(this) to handle the case where GetWidget() has _become_ null when previously it wasn't. Otherwise |focus_manager_| may refer to a deleted object. https://codereview.chromium.org/2345183002/diff/670001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/670001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1884: // make this Textfield start listening to the new FocusManager. This isn't quite right. I think we're after something like focus_manager_ will be null if GetWidget() is null (not currently in a Widget), or if the Widget's FocusManager is null and there is no parent Widget yet (e.g. TYPE_CONTROL Widgets). In the latter case, a call to NativeViewHierarchyChanged() should follow. https://codereview.chromium.org/2345183002/diff/670001/ui/views/focus/focus_t... File ui/views/focus/focus_traversal_unittest.cc (right): https://codereview.chromium.org/2345183002/diff/670001/ui/views/focus/focus_t... ui/views/focus/focus_traversal_unittest.cc:142: // TYPE_CONTROL Widget close order is different on macOS vs Windows/Linux. We generally need platforms to behave the same - there may be a hidden bug here. There's a comment in void BridgedNativeWidget::RemoveOrDestroyChildren() { // TODO(tapted): Implement unowned child windows if required. while (!child_windows_.empty()) { But I don't think that's it. NativeWidgetAura distinguishes between owned and unowned widgets via aura::Window::owned_by_parent_. This is set to false by the NativeViewHostAura constructor. But on Mac, NSWindow (i.e. Mac's aura::Window equivalent) is reference counted, so the concept at this level shouldn't apply. https://codereview.chromium.org/2345183002/diff/670001/ui/views/focus/focus_t... ui/views/focus/focus_traversal_unittest.cc:173: Attach(widget_->GetNativeView()); Note there's some weirdness that starts because of this line. `GetNativeView()` is owned by BOTH |widget_| AND BorderView after this call. And that's OK. NativeViewHostMac::native_view is a scoped_nsobject, so GetNativeView() is reference counted -- it's possible for it to have two owners. There is also the complication that |widget_| is owned by `details.parent->GetWidget()`. That means that details.parent->GetWidget() but be destroyed AFTER |this|, since |this| only has a weak pointer to |widget_|.
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Hi Trent, thanks for the help offline. The changes seem to work on Linux fine as well :) PTAL. https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/630001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:941: if (!GetWidget()) On 2016/11/07 06:14:50, tapted wrote: > On 2016/11/07 05:34:26, Patti Lor wrote: > > On 2016/11/02 03:59:14, tapted wrote: > > > actually I think this `if` can be omitted. -- we can rely on GetFocusManager > > > will returning null. Then we don't have to worry about the case that > > > focus_manager_ might be non-nil here > > > > Done. > > ping? (this should be redundant with the changes in ObserveWidgetFocusChanges) > > But also I think it's important to be calling RemoveFocusChangeListener(this) to > handle the case where GetWidget() has _become_ null when previously it wasn't. > Otherwise |focus_manager_| may refer to a deleted object. Oops, thank you - forgot to reapply the latest patch after switching OSes. https://codereview.chromium.org/2345183002/diff/670001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/670001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1884: // make this Textfield start listening to the new FocusManager. On 2016/11/07 06:14:50, tapted wrote: > This isn't quite right. I think we're after something like > > focus_manager_ will be null if GetWidget() is null (not currently in a Widget), > or if the Widget's FocusManager is null and there is no parent Widget yet (e.g. > TYPE_CONTROL Widgets). In the latter case, a call to > NativeViewHierarchyChanged() should follow. Done. https://codereview.chromium.org/2345183002/diff/670001/ui/views/focus/focus_t... File ui/views/focus/focus_traversal_unittest.cc (right): https://codereview.chromium.org/2345183002/diff/670001/ui/views/focus/focus_t... ui/views/focus/focus_traversal_unittest.cc:142: // TYPE_CONTROL Widget close order is different on macOS vs Windows/Linux. On 2016/11/07 06:14:50, tapted wrote: > We generally need platforms to behave the same - there may be a hidden bug here. > > > There's a comment in > > void BridgedNativeWidget::RemoveOrDestroyChildren() { > // TODO(tapted): Implement unowned child windows if required. > while (!child_windows_.empty()) { > > But I don't think that's it. > > NativeWidgetAura distinguishes between owned and unowned widgets via > aura::Window::owned_by_parent_. This is set to false by the NativeViewHostAura > constructor. But on Mac, NSWindow (i.e. Mac's aura::Window equivalent) is > reference counted, so the concept at this level shouldn't apply. Deleted as discussed offline. https://codereview.chromium.org/2345183002/diff/670001/ui/views/focus/focus_t... ui/views/focus/focus_traversal_unittest.cc:173: Attach(widget_->GetNativeView()); On 2016/11/07 06:14:50, tapted wrote: > Note there's some weirdness that starts because of this line. > > `GetNativeView()` is owned by BOTH |widget_| AND BorderView after this call. > > And that's OK. NativeViewHostMac::native_view is a scoped_nsobject, so > GetNativeView() is reference counted -- it's possible for it to have two owners. > > There is also the complication that |widget_| is owned by > `details.parent->GetWidget()`. That means that details.parent->GetWidget() but > be destroyed AFTER |this|, since |this| only has a weak pointer to |widget_|. > Acknowledged.
looks good - let's send to msw - he knows textfield and rendertext well. It does still need tests, but let's make sure the approach is right.
patricialor@chromium.org changed reviewers: + msw@chromium.org
Hi msw, PTAL at the approach taken here. I'll add tests if you think this approach is OK. Thanks! https://codereview.chromium.org/2345183002/diff/710001/ui/views/focus/focus_t... File ui/views/focus/focus_traversal_unittest.cc (right): https://codereview.chromium.org/2345183002/diff/710001/ui/views/focus/focus_t... ui/views/focus/focus_traversal_unittest.cc:173: std::unique_ptr<Widget> widget_; BorderView uses a Widget as convenient way to create a NativeWindow, but it sets another Widget's NativeWindow as the parent while also holding a weak pointer. To safely cleanup, use a unique_ptr instead. This avoids problems when the parent NativeWindow cleans up its native children. https://codereview.chromium.org/2345183002/diff/710001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11_interactive_uitest.cc (right): https://codereview.chromium.org/2345183002/diff/710001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_x11_interactive_uitest.cc:248: Textfield* textfield = new Textfield; Please see comment thread https://codereview.chromium.org/2345183002/diff/430001/ui/views/widget/deskto... for why this is needed and potential alternatives.
https://codereview.chromium.org/2345183002/diff/710001/ui/views/focus/focus_t... File ui/views/focus/focus_traversal_unittest.cc (right): https://codereview.chromium.org/2345183002/diff/710001/ui/views/focus/focus_t... ui/views/focus/focus_traversal_unittest.cc:173: std::unique_ptr<Widget> widget_; On 2016/11/08 04:00:27, Patti Lor wrote: > BorderView uses a Widget as convenient way to create a NativeWindow, but it sets > another Widget's NativeWindow as the parent while also holding a weak pointer. > To safely cleanup, use a unique_ptr instead. This avoids problems when the > parent NativeWindow cleans up its native children. See also http://crbug.com/663418 about this - let's land this part separately as a fix for 663418 and get input from sky on it
https://codereview.chromium.org/2345183002/diff/710001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2345183002/diff/710001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1203: if (!selection().is_empty() && focused()) { Shouldn't this check also be changed to look for |draw_text_selection_| instead of |focused()|? Otherwise we'll use the un-selected text color with the selected-but-unfocused background color... Even if that seems okay with some local testing; I'd encourage you to test with customized gtk/win colors and/or high-contrast-inverted themes... We should avoid painting unreadable text. https://codereview.chromium.org/2345183002/diff/710001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/2345183002/diff/710001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:765: // Whether to draw the selected text regardless of focus. nit: "Whether to draw the unfocused text selection; false by default." https://codereview.chromium.org/2345183002/diff/710001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:766: bool draw_text_selection_; nit: Consider renaming this for clarity, since it doesn't control the drawing of a text selection when focused. eg. |draw_unfocused_selection_| https://codereview.chromium.org/2345183002/diff/710001/ui/native_theme/native... File ui/native_theme/native_theme_dark_aura.cc (right): https://codereview.chromium.org/2345183002/diff/710001/ui/native_theme/native... ui/native_theme/native_theme_dark_aura.cc:121: case kColorId_TextfieldSelectionBackgroundUnfocused: It seems odd that any textfield colors aren't defined here... eg. What do read-only textfields look like with a dark theme? More importantly, what do unfocused text selections look like? https://codereview.chromium.org/2345183002/diff/710001/ui/native_theme/native... File ui/native_theme/native_theme_win.h (right): https://codereview.chromium.org/2345183002/diff/710001/ui/native_theme/native... ui/native_theme/native_theme_win.h:289: // Get the color used for showing the last selection before focus was moved nit: avoid over-specializing the comment regarding the movement of focus (it's not necessarily true that the control was ever focused before). eg. "Get the color used for showing a selection within an unfocused control." https://codereview.chromium.org/2345183002/diff/710001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/710001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:441: SkColor Textfield::GetUnfocusedSelectionBackgroundColor() const { Inline this or make a file-local function, it's not needed on Textfield. https://codereview.chromium.org/2345183002/diff/710001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:953: if (details.parent->Contains(this) && details.move_view == nullptr) Can you explain what's happening here? Shouldn't this be something like: if (details.child == this) ObserveWidgetFocusChanges(details.is_add); https://codereview.chromium.org/2345183002/diff/710001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:958: ObserveWidgetFocusChanges(true); Add a comment explaining why this is necessary; The function comment says it's useful for tracking the view's FocusManager, but it's not obvious... https://codereview.chromium.org/2345183002/diff/710001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1014: if (HasFocus()) { optional nit: use an inlined ternary or something like line 1102: SkColor selection_bg_color = HasFocus() ? GetSelectionBackgroundColor() : GetUnfocusedSelectionBackgroundColor(); https://codereview.chromium.org/2345183002/diff/710001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1098: void Textfield::OnWillChangeFocus(View* focus_before, View* focus_after) { It's really unfortunate that we can't do this via OnFocus/OnBlur... Why was it determined that we shouldn't paint unfocused selections when the focus is transferred to another view in the widget? https://codereview.chromium.org/2345183002/diff/710001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1109: GetRenderText()->set_selection_background_color(selection_bg_color); Should we SchedulePaint after this, just in case this happens after Textfield::OnFocus or Textfield::OnBlur (in which case this might change the appearance without scheduling a paint) https://codereview.chromium.org/2345183002/diff/710001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1888: focus_manager_ = GetFocusManager(); Should this early-return up top if |focus_manager_ == GetFocusManager() && add_as_listener|? https://codereview.chromium.org/2345183002/diff/710001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2345183002/diff/710001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:331: if (target_) Odd, why is this happening on HandleKeyEvent? https://codereview.chromium.org/2345183002/diff/710001/ui/views/focus/focus_t... File ui/views/focus/focus_traversal_unittest.cc (right): https://codereview.chromium.org/2345183002/diff/710001/ui/views/focus/focus_t... ui/views/focus/focus_traversal_unittest.cc:173: std::unique_ptr<Widget> widget_; On 2016/11/08 23:35:19, tapted wrote: > On 2016/11/08 04:00:27, Patti Lor wrote: > > BorderView uses a Widget as convenient way to create a NativeWindow, but it > sets > > another Widget's NativeWindow as the parent while also holding a weak pointer. > > To safely cleanup, use a unique_ptr instead. This avoids problems when the > > parent NativeWindow cleans up its native children. > > See also http://crbug.com/663418 about this - let's land this part separately as > a fix for 663418 and get input from sky on it Scott already has context on this nuanced issue; so that sounds good. That said, could this be a plain member, instead of using unique_ptr? https://codereview.chromium.org/2345183002/diff/710001/ui/views/widget/deskto... File ui/views/widget/desktop_aura/desktop_window_tree_host_x11_interactive_uitest.cc (right): https://codereview.chromium.org/2345183002/diff/710001/ui/views/widget/deskto... ui/views/widget/desktop_aura/desktop_window_tree_host_x11_interactive_uitest.cc:248: Textfield* textfield = new Textfield; On 2016/11/08 04:00:27, Patti Lor wrote: > Please see comment thread > https://codereview.chromium.org/2345183002/diff/430001/ui/views/widget/deskto... > for why this is needed and potential alternatives. This seems reasonable anyway... The View hierarchy usually owns the views, so keeping it in a unique_ptr, as was done here, is odd.
A couple more comments. Also, can you post some screenshots of unfocused selections in a variety of conditions (Win/Linux/CrOS/Mac Views, all with high-contrast-inverse themes if possible)? https://codereview.chromium.org/2345183002/diff/710001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2345183002/diff/710001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1066: draw_text_selection_(true), I don't know if this should be true by default, now that labels are starting to support text selection... Perhaps we should make this a flag like |draw_unfocused_selection_| that defaults to false... then DrawSelection would check (focused() || draw_unfocused_selection_), etc., which matches my expectation when I originally read the header. https://codereview.chromium.org/2345183002/diff/710001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2345183002/diff/710001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:49: public FocusChangeListener, Would it make sense (and/or be easier to manage) to make Textfield a WidgetObserver and handle OnWidgetActivationChanged and check for the focus state (instead of being a FocusChangeListener)
Hi msw@, thanks for your review - just posting a reply to two of your comments only, will work on everything else in the meantime. Just letting you know I might take a while to get back to you on this CL - now that karandeepb@'s making Labels selectable changes have landed I want to see if there needs to be much changed to make this work for Labels as well, plus the switching over to WidgetObserver. https://codereview.chromium.org/2345183002/diff/710001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2345183002/diff/710001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:49: public FocusChangeListener, On 2016/11/09 02:14:54, msw wrote: > Would it make sense (and/or be easier to manage) to make Textfield a > WidgetObserver and handle OnWidgetActivationChanged and check for the focus > state (instead of being a FocusChangeListener) Ah, I hadn't considered using OnWidgetActivationChanged. From first glance, I think the code might be quite similar - the hard part is probably just when to add the Textfield as a observer, which will probably just go into (Native)ViewHierarchyChanged() like now. It might make more sense though (and we could get rid of the |focus_manager_| pointer, I think). I'll try it and see if anything weird breaks. https://codereview.chromium.org/2345183002/diff/710001/ui/views/focus/focus_t... File ui/views/focus/focus_traversal_unittest.cc (right): https://codereview.chromium.org/2345183002/diff/710001/ui/views/focus/focus_t... ui/views/focus/focus_traversal_unittest.cc:173: std::unique_ptr<Widget> widget_; On 2016/11/09 02:08:53, msw wrote: > On 2016/11/08 23:35:19, tapted wrote: > > On 2016/11/08 04:00:27, Patti Lor wrote: > > > BorderView uses a Widget as convenient way to create a NativeWindow, but it > > sets > > > another Widget's NativeWindow as the parent while also holding a weak > pointer. > > > To safely cleanup, use a unique_ptr instead. This avoids problems when the > > > parent NativeWindow cleans up its native children. > > > > See also http://crbug.com/663418 about this - let's land this part separately > as > > a fix for 663418 and get input from sky on it > > Scott already has context on this nuanced issue; so that sounds good. > That said, could this be a plain member, instead of using unique_ptr? Reverted this file, changes are in https://codereview.chromium.org/2482193003/, which has landed already :) Making |widget_| a normal member means there will need to be an explicit delete widget_; in the BorderView destructor.
https://codereview.chromium.org/2345183002/diff/710001/ui/views/focus/focus_t... File ui/views/focus/focus_traversal_unittest.cc (right): https://codereview.chromium.org/2345183002/diff/710001/ui/views/focus/focus_t... ui/views/focus/focus_traversal_unittest.cc:173: std::unique_ptr<Widget> widget_; On 2016/11/10 07:29:09, Patti Lor wrote: > Making |widget_| a normal member means there will need to be an explicit delete > widget_; in the BorderView destructor. If you just do |Widget widget_;|, it'll be automatically constructed in the ctor and automatically destructed in the dtor, right? No need to delete.
Thanks for the clarification! https://codereview.chromium.org/2345183002/diff/710001/ui/views/focus/focus_t... File ui/views/focus/focus_traversal_unittest.cc (right): https://codereview.chromium.org/2345183002/diff/710001/ui/views/focus/focus_t... ui/views/focus/focus_traversal_unittest.cc:173: std::unique_ptr<Widget> widget_; On 2016/11/10 18:45:01, msw wrote: > On 2016/11/10 07:29:09, Patti Lor wrote: > > Making |widget_| a normal member means there will need to be an explicit > delete > > widget_; in the BorderView destructor. > > If you just do |Widget widget_;|, it'll be automatically constructed in the ctor > and automatically destructed in the dtor, right? No need to delete. Oops, sorry - I didn't realise you meant a non-pointer by "plain member"! You're totally right, I will change this in the next patchset.
(just a drive-by comment) https://codereview.chromium.org/2345183002/diff/710001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2345183002/diff/710001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1098: void Textfield::OnWillChangeFocus(View* focus_before, View* focus_after) { On 2016/11/09 02:08:53, msw wrote: > It's really unfortunate that we can't do this via OnFocus/OnBlur... Why was it > determined that we shouldn't paint unfocused selections when the focus is > transferred to another view in the widget? I think this would be inconsistent with the WebContents. Also the "Graphite" option is a popular theme on Mac, which gives an (almost?) identical gray color to unfocused selections (i.e. instead of Aqua/blue). This would make it almost impossible to tell what textfield has focus based on the selection highlight.
The CQ bit was checked by patricialor@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) |
