Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
769393002:
Minor visual improvements to text selection UI (Closed)
|
Created:
6 years ago by mfomitchev Modified:
6 years ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionMinor visual improvements to text selection UI
Getting rid of the blue cursor lines drawn at the edges of text selection
highlight (when selecting) and on top of the blinking black cursor (when
editing).
Shifting the selection handles 2pts below the text baseline.
Offsetting the touch selection context menu by 2pts from the text highlight.
BUG=436047
Committed: https://crrev.com/1437062436f052c2ec5fb27437af07e1cf0717c5
Cr-Commit-Position: refs/heads/master@{#307784}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressing feedback #
Total comments: 4
Patch Set 3 : Couple of small changes. #Patch Set 4 : Rebase #Patch Set 5 : Fixing the failing test #
Messages
Total messages: 26 (9 generated)
mfomitchev@chromium.org changed reviewers: + mohsen@chromium.org
https://codereview.chromium.org/769393002/diff/1/ui/views/touchui/touch_selec... File ui/views/touchui/touch_selection_controller_impl.cc (right): https://codereview.chromium.org/769393002/diff/1/ui/views/touchui/touch_selec... ui/views/touchui/touch_selection_controller_impl.cc:34: // the offset in pixels above the bottom of the selection (see pic below). This nit: Please also remove the extra space in "in pixels" https://codereview.chromium.org/769393002/diff/1/ui/views/touchui/touch_selec... ui/views/touchui/touch_selection_controller_impl.cc:45: // ___________ nit: Apparently, this line has trailing spaces. Can we get rid of them? https://codereview.chromium.org/769393002/diff/1/ui/views/touchui/touch_selec... ui/views/touchui/touch_selection_controller_impl.cc:308: return GetSelectionWidgetBounds(selection_bound_).size(); What about adding a helper function that calculates the size and use it both here and in GetSelectionWidgetBounds()? (I don't like the fact that GetSelectionWidgetBounds() does some calculations that is thrown away here!)
Also, note that we need to move the quick menu two pixels down, in cases where quick menu is shown under the cursor or it is shown under the selection and it does not fit between the handles. On 2014/12/02 22:34:57, mohsen wrote: > https://codereview.chromium.org/769393002/diff/1/ui/views/touchui/touch_selec... > File ui/views/touchui/touch_selection_controller_impl.cc (right): > > https://codereview.chromium.org/769393002/diff/1/ui/views/touchui/touch_selec... > ui/views/touchui/touch_selection_controller_impl.cc:34: // the offset in pixels > above the bottom of the selection (see pic below). This > nit: Please also remove the extra space in "in pixels" > > https://codereview.chromium.org/769393002/diff/1/ui/views/touchui/touch_selec... > ui/views/touchui/touch_selection_controller_impl.cc:45: // > ___________ > nit: Apparently, this line has trailing spaces. Can we get rid of them? > > https://codereview.chromium.org/769393002/diff/1/ui/views/touchui/touch_selec... > ui/views/touchui/touch_selection_controller_impl.cc:308: return > GetSelectionWidgetBounds(selection_bound_).size(); > What about adding a helper function that calculates the size and use it both > here and in GetSelectionWidgetBounds()? (I don't like the fact that > GetSelectionWidgetBounds() does some calculations that is thrown away here!)
https://codereview.chromium.org/769393002/diff/1/ui/views/touchui/touch_selec... File ui/views/touchui/touch_selection_controller_impl.cc (right): https://codereview.chromium.org/769393002/diff/1/ui/views/touchui/touch_selec... ui/views/touchui/touch_selection_controller_impl.cc:34: // the offset in pixels above the bottom of the selection (see pic below). This On 2014/12/02 22:34:57, mohsen wrote: > nit: Please also remove the extra space in "in pixels" Done. https://codereview.chromium.org/769393002/diff/1/ui/views/touchui/touch_selec... ui/views/touchui/touch_selection_controller_impl.cc:45: // ___________ I don't see any. There's a bunch of underscores. https://codereview.chromium.org/769393002/diff/1/ui/views/touchui/touch_selec... ui/views/touchui/touch_selection_controller_impl.cc:308: return GetSelectionWidgetBounds(selection_bound_).size(); This would be worthwhile if this was called a bunch, but that's not the case. In fact I put a log statement in GetPreferredSize and it never hit. Maybe it gets called under some conditions, but I couldn't hit it. Also, if we add this helper function (lets say GetSelectionWidgetSize), presumably we would want to call it from GetSelectionWidgetBounds to avoid code duplication. Both GetSelectionWidgetBounds and GetSelectionWidgetSize need to calculate the image size, so we'd be calculating the image size twice when we call GetSelectionWidgetBounds. There's also a minor increase in code size.
LGTM https://codereview.chromium.org/769393002/diff/1/ui/views/touchui/touch_selec... File ui/views/touchui/touch_selection_controller_impl.cc (right): https://codereview.chromium.org/769393002/diff/1/ui/views/touchui/touch_selec... ui/views/touchui/touch_selection_controller_impl.cc:45: // ___________ On 2014/12/03 23:57:20, mfomitchev wrote: > I don't see any. There's a bunch of underscores. Yep, you're right. https://codereview.chromium.org/769393002/diff/1/ui/views/touchui/touch_selec... ui/views/touchui/touch_selection_controller_impl.cc:308: return GetSelectionWidgetBounds(selection_bound_).size(); On 2014/12/03 23:57:20, mfomitchev wrote: > This would be worthwhile if this was called a bunch, but that's not the case. In > fact I put a log statement in GetPreferredSize and it never hit. Maybe it gets > called under some conditions, but I couldn't hit it. > Also, if we add this helper function (lets say GetSelectionWidgetSize), > presumably we would want to call it from GetSelectionWidgetBounds to avoid code > duplication. Both GetSelectionWidgetBounds and GetSelectionWidgetSize need to > calculate the image size, so we'd be calculating the image size twice when we > call GetSelectionWidgetBounds. There's also a minor increase in code size. I see. OK. https://codereview.chromium.org/769393002/diff/20001/ui/views/touchui/touch_s... File ui/views/touchui/touch_selection_controller_impl.cc (right): https://codereview.chromium.org/769393002/diff/20001/ui/views/touchui/touch_s... ui/views/touchui/touch_selection_controller_impl.cc:137: kSelectionHandleVertPadding; At least, line 144 has the same indentation issue. https://codereview.chromium.org/769393002/diff/20001/ui/views/touchui/touch_s... ui/views/touchui/touch_selection_controller_impl.cc:667: 0, -kSelectionHandleVerticalVisualOffset); I think you can use Rect::Inset(int horizontal, int vertical) overload.
https://codereview.chromium.org/769393002/diff/20001/ui/views/touchui/touch_s... File ui/views/touchui/touch_selection_controller_impl.cc (right): https://codereview.chromium.org/769393002/diff/20001/ui/views/touchui/touch_s... ui/views/touchui/touch_selection_controller_impl.cc:137: kSelectionHandleVertPadding; On 2014/12/04 00:34:56, mohsen wrote: > At least, line 144 has the same indentation issue. Done. https://codereview.chromium.org/769393002/diff/20001/ui/views/touchui/touch_s... ui/views/touchui/touch_selection_controller_impl.cc:667: 0, -kSelectionHandleVerticalVisualOffset); On 2014/12/04 00:34:56, mohsen wrote: > I think you can use Rect::Inset(int horizontal, int vertical) overload. Done.
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/769393002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/769393002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
mfomitchev@chromium.org changed reviewers: + creis@chromium.org
creis@chromium.org: Please review changes in content/browser/web_contents/touch_editable_impl_aura_browsertest.cc
touch_editable_impl_aura_browsertest.cc LGTM
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/769393002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/769393002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1437062436f052c2ec5fb27437af07e1cf0717c5 Cr-Commit-Position: refs/heads/master@{#307784} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
