Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
103293004:
No touch handles when Views textfield cannot get focused (Closed)
|
Created:
7 years ago by mohsen Modified:
6 years, 4 months ago Reviewers:
sky CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, tfarina, penghuang+watch_chromium.org, nona+watch_chromium.org, James Su Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionNo touch handles when Views textfield cannot get focused
Whenever a Views textfield is going to show touch handles, it should
first check that it has focus or not. If not, the handles should not be
shown. This might be because the textfield is disabled or it is
explicitly set to be non-focusable, among other reasons.
BUG=323956
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289137
Patch Set 1 #
Total comments: 2
Patch Set 2 : Updated after r288781 #
Messages
Total messages: 14 (0 generated)
Please take a look.
https://codereview.chromium.org/103293004/diff/1/ui/views/controls/textfield/... File ui/views/controls/textfield/native_textfield_views.cc (right): https://codereview.chromium.org/103293004/diff/1/ui/views/controls/textfield/... ui/views/controls/textfield/native_textfield_views.cc:1522: if (!textfield_->IsFocusable() || !textfield_->HasFocus()) Why do you need to check IsFocusable? Don't you only care about HasFocus()?
https://codereview.chromium.org/103293004/diff/1/ui/views/controls/textfield/... File ui/views/controls/textfield/native_textfield_views.cc (right): https://codereview.chromium.org/103293004/diff/1/ui/views/controls/textfield/... ui/views/controls/textfield/native_textfield_views.cc:1522: if (!textfield_->IsFocusable() || !textfield_->HasFocus()) On 2013/12/04 03:56:06, sky wrote: > Why do you need to check IsFocusable? Don't you only care about HasFocus()? The textfield might have focus before getting disabled/non-focusable. In that case HasFocus() will be true and if we don't check IsFocusable (which is `focusable_ && enabled_ && IsDrawn()') handles will be shown on tap.
Again, why does it matter? The view has focus. -Scott On Tue, Dec 3, 2013 at 9:22 PM, <mohsen@chromium.org> wrote: > > https://codereview.chromium.org/103293004/diff/1/ui/views/controls/textfield/... > File ui/views/controls/textfield/native_textfield_views.cc (right): > > https://codereview.chromium.org/103293004/diff/1/ui/views/controls/textfield/... > ui/views/controls/textfield/native_textfield_views.cc:1522: if > (!textfield_->IsFocusable() || !textfield_->HasFocus()) > On 2013/12/04 03:56:06, sky wrote: >> >> Why do you need to check IsFocusable? Don't you only care about > > HasFocus()? > > The textfield might have focus before getting disabled/non-focusable. In > that case HasFocus() will be true and if we don't check IsFocusable > (which is `focusable_ && enabled_ && IsDrawn()') handles will be shown > on tap. > > https://codereview.chromium.org/103293004/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I'll also say that if a view has focus and is disabled it should give up focus. -Scott On Wed, Dec 4, 2013 at 6:31 AM, Scott Violet <sky@chromium.org> wrote: > Again, why does it matter? The view has focus. > > -Scott > > On Tue, Dec 3, 2013 at 9:22 PM, <mohsen@chromium.org> wrote: >> >> https://codereview.chromium.org/103293004/diff/1/ui/views/controls/textfield/... >> File ui/views/controls/textfield/native_textfield_views.cc (right): >> >> https://codereview.chromium.org/103293004/diff/1/ui/views/controls/textfield/... >> ui/views/controls/textfield/native_textfield_views.cc:1522: if >> (!textfield_->IsFocusable() || !textfield_->HasFocus()) >> On 2013/12/04 03:56:06, sky wrote: >>> >>> Why do you need to check IsFocusable? Don't you only care about >> >> HasFocus()? >> >> The textfield might have focus before getting disabled/non-focusable. In >> that case HasFocus() will be true and if we don't check IsFocusable >> (which is `focusable_ && enabled_ && IsDrawn()') handles will be shown >> on tap. >> >> https://codereview.chromium.org/103293004/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/12/04 14:31:54, sky wrote: > I'll also say that if a view has focus and is disabled it should give up focus. > > -Scott > > On Wed, Dec 4, 2013 at 6:31 AM, Scott Violet <mailto:sky@chromium.org> wrote: > > Again, why does it matter? The view has focus. > > > > -Scott > > > > On Tue, Dec 3, 2013 at 9:22 PM, <mailto:mohsen@chromium.org> wrote: > >> > >> > https://codereview.chromium.org/103293004/diff/1/ui/views/controls/textfield/... > >> File ui/views/controls/textfield/native_textfield_views.cc (right): > >> > >> > https://codereview.chromium.org/103293004/diff/1/ui/views/controls/textfield/... > >> ui/views/controls/textfield/native_textfield_views.cc:1522: if > >> (!textfield_->IsFocusable() || !textfield_->HasFocus()) > >> On 2013/12/04 03:56:06, sky wrote: > >>> > >>> Why do you need to check IsFocusable? Don't you only care about > >> > >> HasFocus()? > >> > >> The textfield might have focus before getting disabled/non-focusable. In > >> that case HasFocus() will be true and if we don't check IsFocusable > >> (which is `focusable_ && enabled_ && IsDrawn()') handles will be shown > >> on tap. > >> > >> https://codereview.chromium.org/103293004/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. It matters because otherwise we can have touch editing handles on a focused but disabled textfield. Isn't it a bad thing? And, yes, I think if the view gives up focus upon being disabled the problem will be solved, too. (Also, it should give up focus on set_focusable(false) and maybe some other places.) Do you think this is the right way rather than checking IsFocusable?
On Wed, Dec 4, 2013 at 9:28 AM, <mohsen@chromium.org> wrote: > On 2013/12/04 14:31:54, sky wrote: >> >> I'll also say that if a view has focus and is disabled it should give up > > focus. > >> -Scott > > >> On Wed, Dec 4, 2013 at 6:31 AM, Scott Violet <mailto:sky@chromium.org> >> wrote: >> > Again, why does it matter? The view has focus. >> > >> > -Scott >> > >> > On Tue, Dec 3, 2013 at 9:22 PM, <mailto:mohsen@chromium.org> wrote: >> >> >> >> > > > https://codereview.chromium.org/103293004/diff/1/ui/views/controls/textfield/... >> >> >> File ui/views/controls/textfield/native_textfield_views.cc (right): >> >> >> >> > > > https://codereview.chromium.org/103293004/diff/1/ui/views/controls/textfield/... >> >> >> ui/views/controls/textfield/native_textfield_views.cc:1522: if >> >> (!textfield_->IsFocusable() || !textfield_->HasFocus()) >> >> On 2013/12/04 03:56:06, sky wrote: >> >>> >> >>> Why do you need to check IsFocusable? Don't you only care about >> >> >> >> HasFocus()? >> >> >> >> The textfield might have focus before getting disabled/non-focusable. >> >> In >> >> that case HasFocus() will be true and if we don't check IsFocusable >> >> (which is `focusable_ && enabled_ && IsDrawn()') handles will be shown >> >> on tap. >> >> >> >> https://codereview.chromium.org/103293004/ > > >> To unsubscribe from this group and stop receiving emails from it, send an > > email >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > It matters because otherwise we can have touch editing handles on a focused > but > disabled textfield. Isn't it a bad thing? I think the bad thing here is focus on a disabled view. Fix that, then you won't need additional checks. -Scott > And, yes, I think if the view gives up focus upon being disabled the problem > will be solved, too. (Also, it should give up focus on set_focusable(false) > and > maybe some other places.) Do you think this is the right way rather than > checking IsFocusable? > > > https://codereview.chromium.org/103293004/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/12/04 21:24:16, sky wrote: > On Wed, Dec 4, 2013 at 9:28 AM, <mailto:mohsen@chromium.org> wrote: > > On 2013/12/04 14:31:54, sky wrote: > >> > >> I'll also say that if a view has focus and is disabled it should give up > > > > focus. > > > >> -Scott > > > > > >> On Wed, Dec 4, 2013 at 6:31 AM, Scott Violet <mailto:sky@chromium.org> > >> wrote: > >> > Again, why does it matter? The view has focus. > >> > > >> > -Scott > >> > > >> > On Tue, Dec 3, 2013 at 9:22 PM, <mailto:mohsen@chromium.org> wrote: > >> >> > >> >> > > > > > > > https://codereview.chromium.org/103293004/diff/1/ui/views/controls/textfield/... > >> > >> >> File ui/views/controls/textfield/native_textfield_views.cc (right): > >> >> > >> >> > > > > > > > https://codereview.chromium.org/103293004/diff/1/ui/views/controls/textfield/... > >> > >> >> ui/views/controls/textfield/native_textfield_views.cc:1522: if > >> >> (!textfield_->IsFocusable() || !textfield_->HasFocus()) > >> >> On 2013/12/04 03:56:06, sky wrote: > >> >>> > >> >>> Why do you need to check IsFocusable? Don't you only care about > >> >> > >> >> HasFocus()? > >> >> > >> >> The textfield might have focus before getting disabled/non-focusable. > >> >> In > >> >> that case HasFocus() will be true and if we don't check IsFocusable > >> >> (which is `focusable_ && enabled_ && IsDrawn()') handles will be shown > >> >> on tap. > >> >> > >> >> https://codereview.chromium.org/103293004/ > > > > > >> To unsubscribe from this group and stop receiving emails from it, send an > > > > email > >> > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > It matters because otherwise we can have touch editing handles on a focused > > but > > disabled textfield. Isn't it a bad thing? > > I think the bad thing here is focus on a disabled view. Fix that, then > you won't need additional checks. > > -Scott > > > And, yes, I think if the view gives up focus upon being disabled the problem > > will be solved, too. (Also, it should give up focus on set_focusable(false) > > and > > maybe some other places.) Do you think this is the right way rather than > > checking IsFocusable? > > > > > > https://codereview.chromium.org/103293004/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. So, I have uploaded a fix for giving up focus when a view becomes disabled/hidden/unfocusable in a separate CL as it is going to touch a lot of files (because some renaming is required): https://codereview.chromium.org/108063004/ Please review that one first, when it lands, I will come back to this CL. Thanks
Now that https://codereview.chromium.org/108063004/ is landed, this CL is ready for review. Please take a look...
LGTM
The CQ bit was checked by mohsen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mohsen@chromium.org/103293004/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Message was sent while issue was closed.
Change committed as 289137 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
