Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2053213002:
Don't suspend caret blinking on long press (Closed)
|
Created:
4 years, 6 months ago by amaralp Modified:
4 years, 6 months ago CC:
blink-reviews, chromium-reviews, dtapuska+blinkwatch_chromium.org, nzolghadr+blinkwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't suspend caret blinking on long press
Don't suspend caret if the mouse click comes from touch.
BUG=618888
Committed: https://crrev.com/ab92f3d125a34a504610c2412ae8d0bdfcb91f34
Cr-Commit-Position: refs/heads/master@{#399323}
Patch Set 1 #Patch Set 2 : Don't suspend caret blinking on long press #Patch Set 3 : missed a "!" #Patch Set 4 : fixed unit test #
Messages
Total messages: 23 (7 generated)
Description was changed from ========== Don't suspend caret blinking on long press Don't suspend caret if the mouse click comes from touch. BUG=618888 ========== to ========== Don't suspend caret blinking on long press Don't suspend caret if the mouse click comes from touch. BUG=618888 ==========
amaralp@chromium.org changed reviewers: + aelias@chromium.org
On 2016/06/10 00:32:15, amaralp wrote: > mailto:amaralp@chromium.org changed reviewers: > + mailto:aelias@chromium.org PTAL
Could you add a new test case for WebViewTest.BlinkCaretOnTypingAfterLongPress?
On 2016/06/10 00:46:56, aelias wrote: > Could you add a new test case for WebViewTest.BlinkCaretOnTypingAfterLongPress? Done
aelias@chromium.org changed reviewers: + bokan@chromium.org
lgtm, adding bokan@ for Source/core OWNERS
Presumably we only want this to apply on Android but this will currently affect laptops with touch screen as well. This begs the question, why not just remove the "suspend caret blinking" flag altogether? This seems to be a Mac UXism; it was added to WebKit before Chrome's existence but no other platform does this. I think the complexity outweighs its benefit (the top 3 hits on Google for "blinking caret context menu" are Chromium bugs) and can't imagine anyone relying on it. aelias@, WDYT?
aelias@chromium.org changed reviewers: + thakis@chromium.org
On 2016/06/10 at 15:50:21, bokan wrote: > Presumably we only want this to apply on Android but this will currently affect laptops with touch screen as well. > > This begs the question, why not just remove the "suspend caret blinking" flag altogether? This seems to be a Mac UXism; it was added to WebKit before Chrome's existence but no other platform does this. I think the complexity outweighs its benefit (the top 3 hits on Google for "blinking caret context menu" are Chromium bugs) and can't imagine anyone relying on it. aelias@, WDYT? [+cc thakis@ to speak for Mac UX]. Nico, is this a UX convention that Mac folks consider important to preserve, or does no one care? I personally am OK with simply deleting the cursor blink suppression on all platforms. But if Mac wants to preserve it, I think the approach in this patch is the best way to do that (since OS X doesn't support touchsreens, it's not affected by this change). I don't really want to introduce a Mac-specific setting and have all other Chrome platforms differ, since Mac-specific settings are always a bit painful/confusing to deal with.
On 2016/06/10 18:34:53, aelias wrote: > On 2016/06/10 at 15:50:21, bokan wrote: > > Presumably we only want this to apply on Android but this will currently > affect laptops with touch screen as well. > > > > This begs the question, why not just remove the "suspend caret blinking" flag > altogether? This seems to be a Mac UXism; it was added to WebKit before Chrome's > existence but no other platform does this. I think the complexity outweighs its > benefit (the top 3 hits on Google for "blinking caret context menu" are Chromium > bugs) and can't imagine anyone relying on it. aelias@, WDYT? > > [+cc thakis@ to speak for Mac UX]. Nico, is this a UX convention that Mac folks > consider important to preserve, or does no one care? "this" being?
On 2016/06/10 18:35:42, Nico (traveling...slow) wrote: > On 2016/06/10 18:34:53, aelias wrote: > > On 2016/06/10 at 15:50:21, bokan wrote: > > > Presumably we only want this to apply on Android but this will currently > > affect laptops with touch screen as well. > > > > > > This begs the question, why not just remove the "suspend caret blinking" > flag > > altogether? This seems to be a Mac UXism; it was added to WebKit before > Chrome's > > existence but no other platform does this. I think the complexity outweighs > its > > benefit (the top 3 hits on Google for "blinking caret context menu" are > Chromium > > bugs) and can't imagine anyone relying on it. aelias@, WDYT? > > > > [+cc thakis@ to speak for Mac UX]. Nico, is this a UX convention that Mac > folks > > consider important to preserve, or does no one care? > > "this" being? Suspending the caret from blinking when the context menu is shown.
On 2016/06/10 18:36:29, bokan wrote: > On 2016/06/10 18:35:42, Nico (traveling...slow) wrote: > > On 2016/06/10 18:34:53, aelias wrote: > > > On 2016/06/10 at 15:50:21, bokan wrote: > > > > Presumably we only want this to apply on Android but this will currently > > > affect laptops with touch screen as well. > > > > > > > > This begs the question, why not just remove the "suspend caret blinking" > > flag > > > altogether? This seems to be a Mac UXism; it was added to WebKit before > > Chrome's > > > existence but no other platform does this. I think the complexity outweighs > > its > > > benefit (the top 3 hits on Google for "blinking caret context menu" are > > Chromium > > > bugs) and can't imagine anyone relying on it. aelias@, WDYT? > > > > > > [+cc thakis@ to speak for Mac UX]. Nico, is this a UX convention that Mac > > folks > > > consider important to preserve, or does no one care? > > > > "this" being? > > Suspending the caret from blinking when the context menu is shown. It looks like all OS X apps do that, yes.
On 2016/06/10 18:38:13, Nico (traveling...slow) wrote: > It looks like all OS X apps do that, yes. Ok, if we strive for full platform UX conformance than I suppose we should keep it. > But if Mac wants to preserve it, I think the approach in this patch > is the best way to do that (since OS X doesn't support touchsreens, it's not > affected by this change). This does affect non-Mac touchscreen laptops though. The behavior would be a bit weird if the blinking caret would depend on whether the context menu was brought up via long press or right click on CrOS/Win. I think the reason we stop blinking the caret is to hint to the user that text input wont go to the textbox.
On 2016/06/10 18:46:16, bokan wrote: > On 2016/06/10 18:38:13, Nico (traveling...slow) wrote: > > It looks like all OS X apps do that, yes. > > Ok, if we strive for full platform UX conformance than I suppose we should keep > it. > > > But if Mac wants to preserve it, I think the approach in this patch > > is the best way to do that (since OS X doesn't support touchsreens, it's not > > affected by this change). > > This does affect non-Mac touchscreen laptops though. The behavior would be a bit > weird if the blinking caret would depend on whether the context menu was brought > up via long press or right click on CrOS/Win. I think the reason we stop > blinking > the caret is to hint to the user that text input wont go to the textbox. If we're going to keep this I think the embedder needs to tell Blink whether context menus block text input, On Android they don't, which is presumably why we want this change, but on other desktop platforms they do. This CL trades correctness on desktop for correctness on Android, the difference being that I can see this as being useful on Android while a minor polish issue on desktop so I'm fine with the tradeoff. If someone's going to be doing work here it would be nice to clean this up though... tl;dr: lgtm as-is
On 2016/06/10 at 18:58:21, bokan wrote: > This CL trades correctness on desktop for correctness on Android, the difference being > that I can see this as being useful on Android while a minor polish issue on desktop so > I'm fine with the tradeoff. If someone's going to be doing work here it would be nice > to clean this up though... > > tl;dr: lgtm as-is We just tried a Windows 10 device locally and the behavior w.r.t. cursor blinking seems all over the place on that platform. Edge's behavior is different from system Windows textboxes (the system Windows textboxes *always* blink, where Edge sometimes hides the cursor entirely, and additionally the keyboard context menu key behaves differently from right-click on Chrome). So I think it's fair to say there is no definition of correctness on platforms other than Mac and Android. The policy as of this patch is correct for Mac and Android and we aren't really constrained elsewhere, so lgtm again for me.
The CQ bit was checked by amaralp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2053213002/60001
Message was sent while issue was closed.
Description was changed from ========== Don't suspend caret blinking on long press Don't suspend caret if the mouse click comes from touch. BUG=618888 ========== to ========== Don't suspend caret blinking on long press Don't suspend caret if the mouse click comes from touch. BUG=618888 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Don't suspend caret blinking on long press Don't suspend caret if the mouse click comes from touch. BUG=618888 ========== to ========== Don't suspend caret blinking on long press Don't suspend caret if the mouse click comes from touch. BUG=618888 Committed: https://crrev.com/ab92f3d125a34a504610c2412ae8d0bdfcb91f34 Cr-Commit-Position: refs/heads/master@{#399323} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ab92f3d125a34a504610c2412ae8d0bdfcb91f34 Cr-Commit-Position: refs/heads/master@{#399323} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
