Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(271)

Issue 147623004: Tab close buttons can only be targeted with touch if the whole button is visible (Closed)

Created:
6 years, 10 months ago by tdanderson
Modified:
6 years, 10 months ago
Reviewers:
sky
CC:
chromium-reviews, tfarina
Visibility:
Public.

Description

Tab close buttons can only be targeted with touch if the whole button is visible As a result of r246686 landing, tabs can be too easy to close with touch in stacked tab mode when rect-based targeting is enabled. This patch changes the behaviour so that: (1) Hit tests in response to a mouse event will always consider the region of the tab close button that is visible. (2) Hit tests in response to a gesture will only consider the tab close button to be a valid target if the entire button is visible to the user. Note that if there is any vertical clipping of the tab close button, we do not consider the clipped region to be a part of the button's bounds. BUG=338516, 332334 TEST=TabStripTest.ClippedTabCloseButton, also manually tested on Windows when tab close buttons are vertically clipped Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249331

Patch Set 1 #

Total comments: 2

Patch Set 2 : Revised patch #

Patch Set 3 : TODO added #

Total comments: 6

Patch Set 4 : comments addressed #

Unified diffs Side-by-side diffs Delta from patch set Stats (+92 lines, -16 lines) Patch
M chrome/browser/ui/views/tabs/tab.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/views/tabs/tab.cc View 1 2 3 1 chunk +22 lines, -8 lines 0 comments Download
M chrome/browser/ui/views/tabs/tab_strip.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/views/tabs/tab_strip_unittest.cc View 1 2 3 4 chunks +68 lines, -8 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
tdanderson
Scott, can you please take a look? This is my idea for how to address ...
6 years, 10 months ago (2014-01-31 18:32:04 UTC) #1
sky
https://codereview.chromium.org/147623004/diff/1/chrome/browser/ui/views/tabs/tab.cc File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/147623004/diff/1/chrome/browser/ui/views/tabs/tab.cc#newcode433 chrome/browser/ui/views/tabs/tab.cc:433: #if !defined(OS_WIN) Why the ifdef? I also don't understand ...
6 years, 10 months ago (2014-02-03 20:55:47 UTC) #2
tdanderson
https://codereview.chromium.org/147623004/diff/1/chrome/browser/ui/views/tabs/tab.cc File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/147623004/diff/1/chrome/browser/ui/views/tabs/tab.cc#newcode433 chrome/browser/ui/views/tabs/tab.cc:433: #if !defined(OS_WIN) On 2014/02/03 20:55:47, sky wrote: > Why ...
6 years, 10 months ago (2014-02-03 23:08:13 UTC) #3
sky
On Mon, Feb 3, 2014 at 3:08 PM, <tdanderson@chromium.org> wrote: > > https://codereview.chromium.org/147623004/diff/1/chrome/browser/ui/views/tabs/tab.cc > File ...
6 years, 10 months ago (2014-02-04 00:49:57 UTC) #4
tdanderson
Scott, can you please take another look? I added a new unit test for partially-occluded ...
6 years, 10 months ago (2014-02-05 22:29:33 UTC) #5
tdanderson
On 2014/02/05 22:29:33, tdanderson wrote: > Scott, can you please take another look? I added ...
6 years, 10 months ago (2014-02-05 22:58:15 UTC) #6
sky
https://codereview.chromium.org/147623004/diff/170001/chrome/browser/ui/views/tabs/tab.cc File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/147623004/diff/170001/chrome/browser/ui/views/tabs/tab.cc#newcode438 chrome/browser/ui/views/tabs/tab.cc:438: } else if (source == HIT_TEST_SOURCE_TOUCH) { nit: style ...
6 years, 10 months ago (2014-02-06 00:24:09 UTC) #7
tdanderson
Scott, please have another look. https://codereview.chromium.org/147623004/diff/170001/chrome/browser/ui/views/tabs/tab.cc File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/147623004/diff/170001/chrome/browser/ui/views/tabs/tab.cc#newcode438 chrome/browser/ui/views/tabs/tab.cc:438: } else if (source ...
6 years, 10 months ago (2014-02-06 01:19:55 UTC) #8
sky
LGTM
6 years, 10 months ago (2014-02-06 01:40:29 UTC) #9
tdanderson
The CQ bit was checked by tdanderson@chromium.org
6 years, 10 months ago (2014-02-06 01:42:12 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tdanderson@chromium.org/147623004/300001
6 years, 10 months ago (2014-02-06 01:45:37 UTC) #11
commit-bot: I haz the power
6 years, 10 months ago (2014-02-06 10:17:25 UTC) #12
Message was sent while issue was closed.
Change committed as 249331

Powered by Google App Engine
This is Rietveld 408576698