Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2925773002:
Permission prompts shouldn't cause browser to be focused (Closed)
|
Created:
3 years, 6 months ago by Timothy Loh Modified:
3 years, 6 months ago CC:
chrome-apps-syd-reviews_chromium.org, chromium-reviews, tfarina Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPermission prompts shouldn't cause browser to be focused
BUG=729495
Review-Url: https://codereview.chromium.org/2925773002
Cr-Commit-Position: refs/heads/master@{#481430}
Committed: https://chromium.googlesource.com/chromium/src/+/43c4d230102a42febaa1e2102d075426c1141a55
Patch Set 1 #
Total comments: 3
Patch Set 2 : upd #Messages
Total messages: 30 (12 generated)
The CQ bit was checked by timloh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== wip; not for review BUG= ========== to ========== wip; Permission prompts shouldn't cause browser to be focused BUG=729495 ==========
Description was changed from ========== wip; Permission prompts shouldn't cause browser to be focused BUG=729495 ========== to ========== Permission prompts shouldn't cause browser to be focused BUG=729495 ==========
timloh@chromium.org changed reviewers: + jochen@chromium.org
I'm not sure there's a good way to test that a browser *isn't* activated. Waiting on (e.g.) PermissionRequestManager::Observer::OnBubbleAdded (which is called after Show()) isn't enough for the browser requesting the permission to become active (w/o this patch), since activation is asynchronous. Someone suggested opening a third browser and waiting on that, which I didn't try yet. Any thoughts?
I wonder whether there's some kind of point where you know that the bubble is fully constructed. We could then post a task and check that at least then, the browser is the current expected one?
On 2017/06/08 16:56:46, jochen wrote: > I wonder whether there's some kind of point where you know that the bubble is > fully constructed. We could then post a task and check that at least then, the > browser is the current expected one? Perhaps I'm misunderstanding something but I can't manage to make a non-activation test which fails (without this change), even with Sleep()ing for a while. The Show() call currently doesn't seem to mark the browser as the last active browser. The last-active check is this patch is the same check as for prompt() in javascript_dialog_tab_helper.cc, so if there's some bugginess in marking the last active browser, that will have the same issue.
on what OS did you try? It should only fail on Mac :/ OTOH, I wonder why Show() activates at all. Is there any case where we want a bubble to activate its parent window?
On 2017/06/14 08:22:42, jochen wrote: > on what OS did you try? It should only fail on Mac :/ I've been testing on Windows. The pop-under works on both Mac and Windows and this patch seems to fix it on both. > OTOH, I wonder why Show() activates at all. Is there any case where we want a > bubble to activate its parent window? Probably need to ask someone who knows about views :)
On 2017/06/14 at 08:41:47, timloh wrote: > On 2017/06/14 08:22:42, jochen wrote: > > on what OS did you try? It should only fail on Mac :/ > > I've been testing on Windows. The pop-under works on both Mac and Windows and this patch seems to fix it on both. > > > OTOH, I wonder why Show() activates at all. Is there any case where we want a > > bubble to activate its parent window? > > Probably need to ask someone who knows about views :) would it work to just always ShowInactive()?
On 2017/06/14 09:00:19, jochen wrote: > On 2017/06/14 at 08:41:47, timloh wrote: > > On 2017/06/14 08:22:42, jochen wrote: > > > on what OS did you try? It should only fail on Mac :/ > > > > I've been testing on Windows. The pop-under works on both Mac and Windows and > this patch seems to fix it on both. > > > > > OTOH, I wonder why Show() activates at all. Is there any case where we want > a > > > bubble to activate its parent window? > > > > Probably need to ask someone who knows about views :) > > would it work to just always ShowInactive()? I think Show() is needed for accessibility and letting users cancel a prompt with escape. I do wonder if there are many other widgets using Show() with a similar issue, e.g. I noticed <input type=color> let's you steal focus (but at least on Windows killing the element doesn't kill the chooser)
timloh@chromium.org changed reviewers: + raymes@chromium.org
jochen/raymes, do you think it'd be ok to land this without a test?
lg but might be good to have someone with views expertise take a look. Based on our discussions offline it sounds like testing this properly isn't possible so I'm ok without a test. https://codereview.chromium.org/2925773002/diff/1/chrome/browser/ui/views/per... File chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc (right): https://codereview.chromium.org/2925773002/diff/1/chrome/browser/ui/views/per... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:283: if (browser_ == BrowserList::GetInstance()->GetLastActive()) { nit: no {} needed
timloh@chromium.org changed reviewers: + tapted@chromium.org
+tapted@, I think I showed you the change offline but just adding you here to double-check this is sensible.
On 2017/06/19 06:39:21, Timothy Loh wrote: > +tapted@, I think I showed you the change offline but just adding you here to > double-check this is sensible. yup - seems sensible
hum, I'd argue that also for an accessibility perspective, I don't want to website to be able to move my focus to native UI.
huh - I just noticed I had a draft sitting on this that didn't go out with my last reply :/ On 2017/06/19 13:12:36, jochen wrote: > hum, I'd argue that also for an accessibility perspective, I don't want to > website to be able to move my focus to native UI. There needs to be a way for a non-sighted user to notice that the bubble has appeared, and taking focus is the way that's currently done. It is also possible for the bubble to be announced. We still want to do that for the "press esc to exit fullscreen" bubble (http://crbug.com/577011 ) but it ran into a lot of issues. A possible improvement could be to detect if a screen reader is present/running and --if not-- only take focus if the prompt can be linked with a user gesture/click from the web page. I'd deal with this separately though. I think this CL is a step in the right direction. https://codereview.chromium.org/2925773002/diff/1/chrome/browser/ui/views/per... File chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc (right): https://codereview.chromium.org/2925773002/diff/1/chrome/browser/ui/views/per... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:283: if (browser_ == BrowserList::GetInstance()->GetLastActive()) { This should have a comment - it's pretty subtle why it's being done this way. E.g. // If a browser window (or popup) other than the bubble parent has focus, don't take focus. also is the choice of BrowserList intentional, rather than just using browser_->window()->IsActive()? Using BrowserList means that windows from other apps, or Packaged App windows, won't cause taking focus to be suppressed.
https://codereview.chromium.org/2925773002/diff/1/chrome/browser/ui/views/per... File chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc (right): https://codereview.chromium.org/2925773002/diff/1/chrome/browser/ui/views/per... chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc:283: if (browser_ == BrowserList::GetInstance()->GetLastActive()) { On 2017/06/20 11:11:32, tapted wrote: > This should have a comment - it's pretty subtle why it's being done this way. > E.g. > > // If a browser window (or popup) other than the bubble parent has focus, don't > take focus. > > also is the choice of BrowserList intentional, rather than just using > browser_->window()->IsActive()? Using BrowserList means that windows from other > apps, or Packaged App windows, won't cause taking focus to be suppressed. I copied the BrowserList check from app_modal_dialog_helper.cc/javascript_dialog_tab_helper.cc, but it sounds like just checking if it's active is better so I changed it.
lgtm
lgtm
The CQ bit was checked by timloh@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": 1498099514465740,
"parent_rev": "4b2aa9627becb3d376aad991c3eb7969c5d070d9", "commit_rev":
"43c4d230102a42febaa1e2102d075426c1141a55"}
Message was sent while issue was closed.
Description was changed from ========== Permission prompts shouldn't cause browser to be focused BUG=729495 ========== to ========== Permission prompts shouldn't cause browser to be focused BUG=729495 Review-Url: https://codereview.chromium.org/2925773002 Cr-Commit-Position: refs/heads/master@{#481430} Committed: https://chromium.googlesource.com/chromium/src/+/43c4d230102a42febaa1e2102d07... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/43c4d230102a42febaa1e2102d07... |
