Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2729563003:
Dismiss popups on Tab deselected (Closed)
DescriptionDismiss popups on Tab deselected
Popups are ephemeral views used to send commands.
When tab is deselected, they should be dismissed, specially popups relative to
the tab (page info).
BUG=697506, 697470
Review-Url: https://codereview.chromium.org/2729563003
Cr-Commit-Position: refs/heads/master@{#457442}
Committed: https://chromium.googlesource.com/chromium/src/+/3420248539cc1176ac975d7a87adb67937c7d8a8
Patch Set 1 #
Total comments: 1
Patch Set 2 : dismiss popups on tab deselected #Messages
Total messages: 23 (7 generated)
olivierrobin@chromium.org changed reviewers: + eugenebut@chromium.org, lpromero@chromium.org, marq@chromium.org
Hi I am aware that this may seem a little to much, and I am ready to put this only on certain commands (at least all navigation commands). But in my opinion, any user action should dismiss the popups (they are not modal), so I am suggesting this solution.
On 2017/03/01 18:20:09, Olivier Robin wrote: > Hi > > I am aware that this may seem a little to much, and I am ready to put this only > on certain commands (at least all navigation commands). > > But in my opinion, any user action should dismiss the popups (they are not > modal), so I am suggesting this solution. Can we dismiss them when ProvisionalNavigationStarted is called? If ProvisionalNavigationStarted is not called for reload then it's probably a bug.
On 2017/03/01 18:44:05, Eugene But wrote: > On 2017/03/01 18:20:09, Olivier Robin wrote: > > Hi > > > > I am aware that this may seem a little to much, and I am ready to put this > only > > on certain commands (at least all navigation commands). > > > > But in my opinion, any user action should dismiss the popups (they are not > > modal), so I am suggesting this solution. > Can we dismiss them when ProvisionalNavigationStarted is called? If > ProvisionalNavigationStarted is not called for reload then it's probably a bug. Thanks We could call it in ProvisionalNavigationStarted. But if by instance, you use keyboard shortcut to change tab, I think ProvisionalNavigationStarted will not be called. And we still want to dismiss the popups.
On 2017/03/01 20:56:03, Olivier Robin wrote: > On 2017/03/01 18:44:05, Eugene But wrote: > > On 2017/03/01 18:20:09, Olivier Robin wrote: > > > Hi > > > > > > I am aware that this may seem a little to much, and I am ready to put this > > only > > > on certain commands (at least all navigation commands). > > > > > > But in my opinion, any user action should dismiss the popups (they are not > > > modal), so I am suggesting this solution. > > Can we dismiss them when ProvisionalNavigationStarted is called? If > > ProvisionalNavigationStarted is not called for reload then it's probably a > bug. > > Thanks > > We could call it in ProvisionalNavigationStarted. > But if by instance, you use keyboard shortcut to change tab, I think > ProvisionalNavigationStarted will not be called. And we still want to dismiss > the popups. Ultimately, that's not up to me to decide if we should dismiss popup for all Chrome commands. But if we choose to do so, then this fix will not work in the new architecture. Mark, WDYT?
On 2017/03/01 21:04:46, Eugene But wrote: > On 2017/03/01 20:56:03, Olivier Robin wrote: > > On 2017/03/01 18:44:05, Eugene But wrote: > > > On 2017/03/01 18:20:09, Olivier Robin wrote: > > > > Hi > > > > > > > > I am aware that this may seem a little to much, and I am ready to put this > > > only > > > > on certain commands (at least all navigation commands). > > > > > > > > But in my opinion, any user action should dismiss the popups (they are not > > > > modal), so I am suggesting this solution. > > > Can we dismiss them when ProvisionalNavigationStarted is called? If > > > ProvisionalNavigationStarted is not called for reload then it's probably a > > bug. > > > > Thanks > > > > We could call it in ProvisionalNavigationStarted. > > But if by instance, you use keyboard shortcut to change tab, I think > > ProvisionalNavigationStarted will not be called. And we still want to dismiss > > the popups. > Ultimately, that's not up to me to decide if we should dismiss popup for all > Chrome commands. But if we choose to do so, then this fix will not work in the > new architecture. Mark, WDYT? This approach, and this CL, is 100% specific to the old architecture.
https://codereview.chromium.org/2729563003/diff/1/ios/chrome/browser/ui/brows... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2729563003/diff/1/ios/chrome/browser/ui/brows... ios/chrome/browser/ui/browser_view_controller.mm:3843: [self dismissPopups]; I can't verify that this is correct. What are all the paths by which the BVC's chromeExecuteCommand can be called? Is it correct in every one of those cases to dismiss popups? I don't know. Remember that this code path can be triggered in many different ways.
On 2017/03/02 06:07:40, marq (Slow 24 Feb - 2 March) wrote: > https://codereview.chromium.org/2729563003/diff/1/ios/chrome/browser/ui/brows... > File ios/chrome/browser/ui/browser_view_controller.mm (right): > > https://codereview.chromium.org/2729563003/diff/1/ios/chrome/browser/ui/brows... > ios/chrome/browser/ui/browser_view_controller.mm:3843: [self dismissPopups]; > I can't verify that this is correct. > > What are all the paths by which the BVC's chromeExecuteCommand can be called? Is > it correct in every one of those cases to dismiss popups? I don't know. > > Remember that this code path can be triggered in many different ways. OK, I will do a more conservative solution because the bug I am fixing is more specific than that. But this is a problem, and we should solve it, either now or in the new architecture. If not, this can lead to situations like this https://screenshot.googleplex.com/UAA8ouNyD4T
Description was changed from ========== Dismiss popups on receiving commands Popups are ephemeral views used to send commands. When commands are received, they should be dismissed. BUG=697506, 697470 ========== to ========== Dismiss popups on Tab deselected Popups are ephemeral views used to send commands. When tab is deselected, they should be dismissed, specially popups relative to the tab (page info). BUG=697506, 697470 ==========
This seems to only fix the case where you switch tab, no? Reload, goBack, goForward, etc. don't trigger a tab deselection, do they?
On 2017/03/02 10:40:30, lpromero wrote: > This seems to only fix the case where you switch tab, no? Reload, goBack, > goForward, etc. don't trigger a tab deselection, do they? Correct. This will only partially solve crbug.com/697506
lgtm
lgtm
The CQ bit was checked by olivierrobin@chromium.org
The CQ bit was unchecked by olivierrobin@chromium.org
marq, any comment on this new version?
LGTM
The CQ bit was checked by olivierrobin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489677222979860,
"parent_rev": "9c7221f573308689650a423da0e02744c8b1d02a", "commit_rev":
"3420248539cc1176ac975d7a87adb67937c7d8a8"}
Message was sent while issue was closed.
Description was changed from ========== Dismiss popups on Tab deselected Popups are ephemeral views used to send commands. When tab is deselected, they should be dismissed, specially popups relative to the tab (page info). BUG=697506, 697470 ========== to ========== Dismiss popups on Tab deselected Popups are ephemeral views used to send commands. When tab is deselected, they should be dismissed, specially popups relative to the tab (page info). BUG=697506, 697470 Review-Url: https://codereview.chromium.org/2729563003 Cr-Commit-Position: refs/heads/master@{#457442} Committed: https://chromium.googlesource.com/chromium/src/+/3420248539cc1176ac975d7a87ad... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/3420248539cc1176ac975d7a87ad... |
