Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7696013:
Minor fix to touch selection logic. (Closed)
|
Created:
9 years, 4 months ago by varunjain Modified:
9 years, 4 months ago CC:
chromium-reviews, dhollowa Visibility:
Public. |
DescriptionMinor fix to touch selection logic.
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97810
Patch Set 1 #
Total comments: 4
Patch Set 2 : modified according to comments #Patch Set 3 : modified according to comments #
Messages
Total messages: 10 (0 generated)
http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... File views/touchui/touch_selection_controller_impl.cc (right): http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... views/touchui/touch_selection_controller_impl.cc:228: bool TouchSelectionControllerImpl::isPointInClientView(const gfx::Point& p) { is -> Is http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... views/touchui/touch_selection_controller_impl.cc:230: return p.x() >= r.x() && p.x() <= r.x() + r.width() && return r.Contains(p)
http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... File views/touchui/touch_selection_controller_impl.cc (right): http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... views/touchui/touch_selection_controller_impl.cc:228: bool TouchSelectionControllerImpl::isPointInClientView(const gfx::Point& p) { On 2011/08/22 14:21:37, sky wrote: > is -> Is Done. http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... views/touchui/touch_selection_controller_impl.cc:230: return p.x() >= r.x() && p.x() <= r.x() + r.width() && On 2011/08/22 14:21:37, sky wrote: > return r.Contains(p) Rect::Contains checks for strict inclusion and returns false if the point lies on the boundary of the rectangle. Thats the bug this change is fixing.
On Mon, Aug 22, 2011 at 9:58 AM, <varunjain@chromium.org> wrote: > > http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... > File views/touchui/touch_selection_controller_impl.cc (right): > > http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... > views/touchui/touch_selection_controller_impl.cc:228: bool > TouchSelectionControllerImpl::isPointInClientView(const gfx::Point& p) { > On 2011/08/22 14:21:37, sky wrote: >> >> is -> Is > > Done. > > http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... > views/touchui/touch_selection_controller_impl.cc:230: return p.x() >= > r.x() && p.x() <= r.x() + r.width() && > On 2011/08/22 14:21:37, sky wrote: >> >> return r.Contains(p) > > Rect::Contains checks for strict inclusion and returns false if the > point lies on the boundary of the rectangle. Thats the bug this change > is fixing. > > http://codereview.chromium.org/7696013/ > Then I'm confused. If you're widget/view has a width of n pixels, then an x-coordinate of n is not inside the view. What am I missing? -Scott
On Mon, Aug 22, 2011 at 1:11 PM, Scott Violet <sky@chromium.org> wrote: > On Mon, Aug 22, 2011 at 9:58 AM, <varunjain@chromium.org> wrote: > > > > > http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... > > File views/touchui/touch_selection_controller_impl.cc (right): > > > > > http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... > > views/touchui/touch_selection_controller_impl.cc:228: bool > > TouchSelectionControllerImpl::isPointInClientView(const gfx::Point& p) { > > On 2011/08/22 14:21:37, sky wrote: > >> > >> is -> Is > > > > Done. > > > > > http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... > > views/touchui/touch_selection_controller_impl.cc:230: return p.x() >= > > r.x() && p.x() <= r.x() + r.width() && > > On 2011/08/22 14:21:37, sky wrote: > >> > >> return r.Contains(p) > > > > Rect::Contains checks for strict inclusion and returns false if the > > point lies on the boundary of the rectangle. Thats the bug this change > > is fixing. > > > > http://codereview.chromium.org/7696013/ > > > > Then I'm confused. If you're widget/view has a width of n pixels, then > an x-coordinate of n is not inside the view. What am I missing? > Before this CL, it used to be r.Contains(). However, the bug I noticed is that "sometimes" the selection handle would not show up even though the selected region would be completely visible (inside the view). The reason was that the selection endpoint was actually on the bottom boundary of the view. For a textfield, at least, the height of the selection rectangle spans from y-coordinate 0 to height() inclusive. > -Scott >
On Mon, Aug 22, 2011 at 1:45 PM, Varun Jain <varunjain@google.com> wrote: > > > On Mon, Aug 22, 2011 at 1:11 PM, Scott Violet <sky@chromium.org> wrote: > >> On Mon, Aug 22, 2011 at 9:58 AM, <varunjain@chromium.org> wrote: >> > >> > >> http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... >> > File views/touchui/touch_selection_controller_impl.cc (right): >> > >> > >> http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... >> > views/touchui/touch_selection_controller_impl.cc:228: bool >> > TouchSelectionControllerImpl::isPointInClientView(const gfx::Point& p) { >> > On 2011/08/22 14:21:37, sky wrote: >> >> >> >> is -> Is >> > >> > Done. >> > >> > >> http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... >> > views/touchui/touch_selection_controller_impl.cc:230: return p.x() >= >> > r.x() && p.x() <= r.x() + r.width() && >> > On 2011/08/22 14:21:37, sky wrote: >> >> >> >> return r.Contains(p) >> > >> > Rect::Contains checks for strict inclusion and returns false if the >> > point lies on the boundary of the rectangle. Thats the bug this change >> > is fixing. >> > >> > http://codereview.chromium.org/7696013/ >> > >> >> Then I'm confused. If you're widget/view has a width of n pixels, then >> an x-coordinate of n is not inside the view. What am I missing? >> > > Before this CL, it used to be r.Contains(). However, the bug I noticed is > that "sometimes" the selection handle would not show up even though the > selected region would be completely visible (inside the view). The reason > was that the selection endpoint was actually on the bottom boundary of the > view. For a textfield, at least, the height of the selection rectangle spans > from y-coordinate 0 to height() inclusive. > Currently the selection endpoint provided by the textfield is gfx::Point(selection_rect.x(), selection_rect.bottom()). I guess, another way to fix this bug would be to change that to gfx::Point(selection_rect.x(), selection_rect.bottom() - 1) > > >> -Scott >> > >
On Mon, Aug 22, 2011 at 10:49 AM, Varun Jain <varunjain@google.com> wrote: > > > On Mon, Aug 22, 2011 at 1:45 PM, Varun Jain <varunjain@google.com> wrote: >> >> >> On Mon, Aug 22, 2011 at 1:11 PM, Scott Violet <sky@chromium.org> wrote: >>> >>> On Mon, Aug 22, 2011 at 9:58 AM, <varunjain@chromium.org> wrote: >>> > >>> > >>> > http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... >>> > File views/touchui/touch_selection_controller_impl.cc (right): >>> > >>> > >>> > http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... >>> > views/touchui/touch_selection_controller_impl.cc:228: bool >>> > TouchSelectionControllerImpl::isPointInClientView(const gfx::Point& p) >>> > { >>> > On 2011/08/22 14:21:37, sky wrote: >>> >> >>> >> is -> Is >>> > >>> > Done. >>> > >>> > >>> > http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... >>> > views/touchui/touch_selection_controller_impl.cc:230: return p.x() >= >>> > r.x() && p.x() <= r.x() + r.width() && >>> > On 2011/08/22 14:21:37, sky wrote: >>> >> >>> >> return r.Contains(p) >>> > >>> > Rect::Contains checks for strict inclusion and returns false if the >>> > point lies on the boundary of the rectangle. Thats the bug this change >>> > is fixing. >>> > >>> > http://codereview.chromium.org/7696013/ >>> > >>> >>> Then I'm confused. If you're widget/view has a width of n pixels, then >>> an x-coordinate of n is not inside the view. What am I missing? >> >> Before this CL, it used to be r.Contains(). However, the bug I noticed is >> that "sometimes" the selection handle would not show up even though the >> selected region would be completely visible (inside the view). The reason >> was that the selection endpoint was actually on the bottom boundary of the >> view. For a textfield, at least, the height of the selection rectangle spans >> from y-coordinate 0 to height() inclusive. > > Currently the selection endpoint provided by the textfield is > gfx::Point(selection_rect.x(), selection_rect.bottom()). I guess, another > way to fix this bug would be to change that > to gfx::Point(selection_rect.x(), selection_rect.bottom() - 1) This seems like the right fix. Otherwise the textfield isn't providing a point that is part of the selection. This is another reason why I thought the selection should be in terms of a rect and not a point. -Scott
On 2011/08/22 20:21:39, sky wrote: > On Mon, Aug 22, 2011 at 10:49 AM, Varun Jain <mailto:varunjain@google.com> wrote: > > > > > > On Mon, Aug 22, 2011 at 1:45 PM, Varun Jain <mailto:varunjain@google.com> wrote: > >> > >> > >> On Mon, Aug 22, 2011 at 1:11 PM, Scott Violet <mailto:sky@chromium.org> wrote: > >>> > >>> On Mon, Aug 22, 2011 at 9:58 AM, mailto: <varunjain@chromium.org> wrote: > >>> > > >>> > > >>> > > http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... > >>> > File views/touchui/touch_selection_controller_impl.cc (right): > >>> > > >>> > > >>> > > http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... > >>> > views/touchui/touch_selection_controller_impl.cc:228: bool > >>> > TouchSelectionControllerImpl::isPointInClientView(const gfx::Point& p) > >>> > { > >>> > On 2011/08/22 14:21:37, sky wrote: > >>> >> > >>> >> is -> Is > >>> > > >>> > Done. > >>> > > >>> > > >>> > > http://codereview.chromium.org/7696013/diff/1/views/touchui/touch_selection_c... > >>> > views/touchui/touch_selection_controller_impl.cc:230: return p.x() >= > >>> > r.x() && p.x() <= r.x() + r.width() && > >>> > On 2011/08/22 14:21:37, sky wrote: > >>> >> > >>> >> return r.Contains(p) > >>> > > >>> > Rect::Contains checks for strict inclusion and returns false if the > >>> > point lies on the boundary of the rectangle. Thats the bug this change > >>> > is fixing. > >>> > > >>> > http://codereview.chromium.org/7696013/ > >>> > > >>> > >>> Then I'm confused. If you're widget/view has a width of n pixels, then > >>> an x-coordinate of n is not inside the view. What am I missing? > >> > >> Before this CL, it used to be r.Contains(). However, the bug I noticed is > >> that "sometimes" the selection handle would not show up even though the > >> selected region would be completely visible (inside the view). The reason > >> was that the selection endpoint was actually on the bottom boundary of the > >> view. For a textfield, at least, the height of the selection rectangle spans > >> from y-coordinate 0 to height() inclusive. > > > > Currently the selection endpoint provided by the textfield is > > gfx::Point(selection_rect.x(), selection_rect.bottom()). I guess, another > > way to fix this bug would be to change that > > to gfx::Point(selection_rect.x(), selection_rect.bottom() - 1) > > This seems like the right fix. Otherwise the textfield isn't providing > a point that is part of the selection. This is another reason why I > thought the selection should be in terms of a rect and not a point. > > -Scott Done
LGTM
Change committed as 97810 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
