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

Issue 576643002: Mac: Ignore sheets when deciding whether to dismiss a browser action popup. (Closed)

Created:
6 years, 3 months ago by tapted
Modified:
6 years, 3 months ago
Reviewers:
Robert Sesek
CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Mac: Ignore sheets when deciding whether to dismiss a browser action popup. Since Chrome 8, invoking <input type="file"> in a browser action popup on OSX would show, then immediately close the sheet (and the popup). This CL fixes it by ignoring loss of key status on the bubble window when it has an attached sheet. Loss of key status (or clicks) on windows that are sheets themselves are also ignored, otherwise dismissing the sheet would close the bubble as well, since it's already lost its "sheet attached" status by that stage. BUG=61632 Committed: https://crrev.com/780dfa3dcc6fc09b3ea4999ec6a4db640556e287 Cr-Commit-Position: refs/heads/master@{#295198}

Patch Set 1 #

Patch Set 2 : with a test #

Patch Set 3 : -dot notation.. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+117 lines, -65 lines) Patch
M chrome/browser/ui/cocoa/base_bubble_controller.mm View 1 2 4 chunks +28 lines, -16 lines 0 comments Download
M chrome/browser/ui/cocoa/base_bubble_controller_unittest.mm View 1 2 5 chunks +89 lines, -49 lines 0 comments Download

Messages

Total messages: 12 (5 generated)
tapted
Hi Robert, please take a look. It's a ~3 line change with a bit of ...
6 years, 3 months ago (2014-09-16 08:54:51 UTC) #3
Robert Sesek
LGTM. +10 points for fixing such a low bug number :).
6 years, 3 months ago (2014-09-16 14:27:37 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/576643002/60001
6 years, 3 months ago (2014-09-16 21:27:57 UTC) #6
commit-bot: I haz the power
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromium_gn_compile_rel/builds/16093) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_rel/builds/16115)
6 years, 3 months ago (2014-09-16 21:41:29 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/576643002/60001
6 years, 3 months ago (2014-09-16 23:10:58 UTC) #10
commit-bot: I haz the power
Committed patchset #3 (id:60001) as 0e624b68bbb85499ccfada925236f076bac6b997
6 years, 3 months ago (2014-09-17 00:54:49 UTC) #11
commit-bot: I haz the power
6 years, 3 months ago (2014-09-17 00:55:42 UTC) #12
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/780dfa3dcc6fc09b3ea4999ec6a4db640556e287
Cr-Commit-Position: refs/heads/master@{#295198}

Powered by Google App Engine
This is Rietveld 408576698