New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 767682 link

Starred by 1 user

Issue metadata

Status: Archived
Owner: ----
Closed: Sep 13
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug



Sign in to add a comment

ExclusiveAccessBubbleViews requires |context| to provide an accelerator, but sometimes ignores it

Project Member Reported by tapted@chromium.org, Sep 22 2017

Issue description

Chrome Version       : 63.0.3213.3

ExclusiveAccessBubbleViews does

  // Create the contents view.
  ui::Accelerator accelerator(ui::VKEY_UNKNOWN, ui::EF_NONE);
  bool got_accelerator =
      bubble_view_context_->GetAcceleratorProvider()
          ->GetAcceleratorForCommandId(IDC_FULLSCREEN, &accelerator);
  DCHECK(got_accelerator);
  view_ = new SubtleNotificationView(this);
  browser_fullscreen_exit_accelerator_ = accelerator.GetShortcutText();


So it requires 
 - GetAcceleratorProvider() to return non-null, and
 - that provider to provide an accelerator to populate |browser_fullscreen_exit_accelerator_|


But later on, |browser_fullscreen_exit_accelerator_| may be ignored:

  if (bubble_type ==
          EXCLUSIVE_ACCESS_BUBBLE_TYPE_BROWSER_FULLSCREEN_EXIT_INSTRUCTION ||
      bubble_type ==
          EXCLUSIVE_ACCESS_BUBBLE_TYPE_EXTENSION_FULLSCREEN_EXIT_INSTRUCTION) {
    accelerator = browser_fullscreen_exit_accelerator_;
  } else {
    accelerator = l10n_util::GetStringUTF16(IDS_APP_ESC_KEY);
    if (bubble_type !=
        EXCLUSIVE_ACCESS_BUBBLE_TYPE_FULLSCREEN_EXIT_INSTRUCTION) {
      link_visible = false;
    }
  }

i.e., it's only used for browser/extension fullscreen, but not OS/immersive fullscreen modes, which just clobber |browser_fullscreen_exit_accelerator_| with IDS_APP_ESC_KEY

This came up in https://chromium-review.googlesource.com/c/chromium/src/+/648985/13/chrome/browser/ui/views/apps/chrome_native_app_window_views_aura_ash.cc#462


(there's probably a lot of cleanups around these bubbles once we can ditch the non-simplified codepaths and the chrome://flag).
 

Comment 1 by mgiuca@chromium.org, Sep 22 2017

Cc: robliao@chromium.org
I've had a CL out for awhile to clean this up:
https://crrev.com/c/609821

But the team is potentially bringing back some of the old functionality so I've been holding off on landing it.
Cc: -scheib@chromium.org
Status: Archived (was: Available)
Archiving old bugs that haven't been actively assigned in over 180 days.

If you feel this issue should still be addressed, feel free to reopen it or to file a new issue. Thanks!
Archiving old bugs that haven't been actively assigned in over 180 days.

If you feel this issue should still be addressed, feel free to reopen it or to file a new issue. Thanks!

Sign in to add a comment