Alert users to replacement shortcut for backspace-goes-back |
||||||||||||||||||||||||||||||
Issue descriptionIf we remove backspace-goes-back as a shortcut, we should try to alert its users to replacement shortcuts so it's clear the browser is not broken and there is a substitute they can use. Proposal: * Reuse the UI (and code) from fullscreen mode (partly opaque grey box near top of screen with white message, which fades out after a few seconds). * If user hits backspace two times within four seconds, on the same page, with no text control selected (i.e. backspace would previously have gone back), show UI saying "Press Alt+Left Arrow to go back". (Match the wording for the "Back" keyboard shortcut in the page context menu.) * Do this the first 5 times this happens, then never again. * Also show "Press Alt+Right Arrow to go forward" for analogous situation with shift-backspace. I think we should aim for this to be in the same release where we try removing the backspace shortcut. This would be suitable for a newer contributor to tackle, I think.
Showing comments 15 - 114
of 114
Older ›
,
May 13 2016
I could definitely do 6 (it's a few lines of code), definitely NOT do 5, and I have no idea on 4 -- it depends how precisely the backspace accelerator is handled. If it's handled by the views accelerator system, it'd be pretty easy to do 4; if views knows nothing about this and it's entirely processed by Blink, I'm not terribly sure what to do.
,
May 13 2016
#15: Who actually removed the shortcut? It seems that person would know those answers.
,
May 13 2016
Ojan, but in our meeting discussing this he confessed that it was not obvious to him how the control flow here worked, and though I can tell you the patch was Blink-only, that doesn't mean views isn't _also_ aware of this shortcut (or potentially aware of it). I would have to peer more closely at the code to figure out precisely how this works.
,
May 13 2016
Oh OK I thought we would hook this up from the same place it was hooked to trigger the back, but if that's not an option, I guess we can hook it in the same way the fullscreen system knows when you're pressing Esc: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/browser.cc&l=1239 (A bit messy to insert some code at the top level of the browser keyboard handler to do this, but there's precedent.) I guess that's pretty easy so I could do 4. I'll try to hack it together and then decide how much effort it will be to land it.
,
May 13 2016
Be careful. You don't want to consume backspace when it wouldn't have been treated as a shortcut, e.g. when a textarea has focus. That's why I was concerned about this.
,
May 13 2016
#19 Ah, that is a very good point. In that case it seems very likely that we'll need to get that event (with the added context of whether a textarea is focused) from the renderer to the browser, which means we may need to add a new Mojo service or similar. In that case, probably not worth it (given #8 and #9 suggest this is not critical to launching the removal of the shortcut). ojan@: Perhaps you can comment on the implementation. If you can get a signal to the UI thread on the browser process that says "the user pressed backspace while a textarea was not focused", then we can do it.
,
May 13 2016
There were two patches. You can ignore the blink-side one entirely. https://codereview.chromium.org/1922993002 is the chromium side one. If you revert that, you'll get backspace goes back on all platforms. The blink-side code was entirely redundant as best I can tell. I'll try to work on adding those back in as a commandline flag next week unless someone else beats me too it (please do!). I think the thing we need to do is create a new command IDC_BACKSPACE_BACK and use it in all the places we're adding IDC_BACK back in. Then we can control IDC_BACKSPACE_BACK from the commandline. You don't need to worry about textareas and whatnot. The browser side keyboard accelerator code only gets hit if the renderer didn't consume the backspace.
,
May 13 2016
That patch makes so much more sense to me than the Blink one I saw earlier... it actually does precisely what I'd have expected it to do :). It will be really easy to implement #4 given that. I might be able to fit this into my schedule somewhere (though help would certainly be welcomed; I can definitely review if I don't write the code).
,
May 16 2016
#21 Sounds good. OK if we're going to go ahead with this then I'll do the UI code but I'd prefer if someone else (ojan or pkasting) do the hookup (Step 4). I will probably add a method to BrowserView that triggers the bubble. Assigning this to pkasting who is driving the effort.
,
May 16 2016
OK I've refactored the code and whirlwind-hacked the new backspace notification view into BrowserWindow. It is not submittable but should be good to patch in locally and then hook up the appropriate method call. You need to apply: 1. https://codereview.chromium.org/1971033003 2. https://codereview.chromium.org/1979193002 3. https://codereview.chromium.org/1983803002 Then you can call BrowserWindow::ShowBackspaceNewShortcutBubble(false/true); false for Back and true for Forward. As I said in #23, if Ojan or Peter could do this hookup work, that would be great. Let me know if there are any problems getting the bubble to show up. (It's also temporarily hooked up so that if you go into fullscreen, it shows the Backspace prompt, just for testing purposes. But the code is actually neatly factored out.)
,
May 16 2016
,
May 16 2016
#CBC-RS/TC-watchlist
,
May 17 2016
An update on the UX mock: the notification will actually be in the same place as with pointer lock (not full screen), which slightly overlaps the address bar. I think this helps with Bruno's concern that it will be "too busy" as it is not likely to overlap much text. It also means that users can distinguish it from page content. See attachment.
,
May 17 2016
I think that helps a lot. Something I just realized: we should probably use Option + <- in the notification when in MacOS. Alt should work for Windows, Linux and ChromeOS though.
,
May 17 2016
,
May 18 2016
I don't know what the story is on Mac but the new shortcut has been in for awhile. This bug is just about the notification. (And as per #15 this will probably not get done on Mac because it's complicated.)
,
May 18 2016
,
May 18 2016
So I'm not sure who is going to do the hookup. In #21 Ojan pseudo-volunteered and in #22 Peter pseudo-volunteered. If neither of you are going to do it then I can have a look but I'm not clear on the best course of action... Should we just be adding a new accelerator but not adding it to any menus? (Just making the backspace key trigger it.) Let me see how easy that is to do...
,
May 18 2016
I would do a partial revert of the commit Ojan references in comment 21. That CL pulls out pretty much exactly the code that would be needed here, I'd think, except for the bits about "pressed twice" and "five time limit" and so forth.
,
May 18 2016
I got the basic functionality working: https://codereview.chromium.org/1983803002
,
May 18 2016
Now ready for review: 1. https://codereview.chromium.org/1979193002 2. https://codereview.chromium.org/1983803002 As I said on the second review there, I really don't think there's time to get this in before branch point. There's a lot of code to review, it's been written in a hurry (but I tried to clean things up and not cut corners), and branch point is in 1.5 days. And it doesn't implement some of the features you requested. And it's not on Mac. One option is to just land strings and merge later. Another option is to just skip 52 and add the notification in 53. I'll leave it up to you to decide what the best course of action is.
,
May 18 2016
Let's get the strings in immediately, since it's pretty clear what those are going to be and that gives us maximum flexibility. This isn't actually all that big ("big" to me is like 1500 lines), so we should have no trouble making it in either before or shortly after the branch.
I don't have an answer on Mac, though -- I don't have the ability to write that side :P
,
May 18 2016
Removing the Backspace shortcut that has been used for 10+ years in various browsers and for 20+ years on Windows OS seems nonsensical from here. As for Alt-Left, it's quite awkward to press with one hand compared to Backspace. Of course the expected/normal behavior of Backspace can be restored with a userscript/extension on normal web pages but this method won't work on chrome*:// urls.
,
May 18 2016
This bug is not about debating the removal of backspace as a shortcut.
,
May 18 2016
I have a patch adding this code back as a default disable finch trial. Should be pretty straightforward to then hook at that up to this UI code. Will send for review once it passes trybots. I think the only thing missing is storing when the last backspace happened so that you get the heuristic right. Peter, you willing to add the heuristic + hook this up to mguica's code?
,
May 18 2016
I'm wondering if we can in fact be even simpler and just show the warning all the time. I think the right thing to do here is for you to land your patch, then we land the patch that hooks this up to the notification UI with no heuristics, then we test locally a bit and verify whether we need the heuristics to prevent annoyance (my guess is, we probably do need a clamp on how many times this is shown, but we might not need the sophistication of checking for multiple presses within a short time period) and add that.
,
May 18 2016
I would much prefer we gate on multiple presses. The UI appearing for users who didn't intend it would be very unexpected and confusing.
,
May 18 2016
I'm very prepared to believe we need that, I mostly just want to make sure there's a sequential set of changes planned that will get us there, and that I test things out as I go to make sure the UI all feels right.
,
May 19 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/238b3392aa0f9cb91aeff8cb147810030b1775af commit 238b3392aa0f9cb91aeff8cb147810030b1775af Author: mgiuca <mgiuca@chromium.org> Date: Thu May 19 04:00:40 2016 Factor ExclusiveAccessView out to SubtleNotificationView. This makes the subtle white-on-black fullscreen notification code available for general use, by removing all fullscreen-specific code from the view. Planned for use as a dialog that tells you the new shortcut key for Back and Forward now that Backspace and Shift+Backspace no longer work. BUG= 610039 Review-Url: https://codereview.chromium.org/1979193002 Cr-Commit-Position: refs/heads/master@{#394662} [modify] https://crrev.com/238b3392aa0f9cb91aeff8cb147810030b1775af/chrome/browser/ui/views/exclusive_access_bubble_views.cc [modify] https://crrev.com/238b3392aa0f9cb91aeff8cb147810030b1775af/chrome/browser/ui/views/exclusive_access_bubble_views.h [add] https://crrev.com/238b3392aa0f9cb91aeff8cb147810030b1775af/chrome/browser/ui/views/subtle_notification_view.cc [add] https://crrev.com/238b3392aa0f9cb91aeff8cb147810030b1775af/chrome/browser/ui/views/subtle_notification_view.h [modify] https://crrev.com/238b3392aa0f9cb91aeff8cb147810030b1775af/chrome/chrome_browser_ui.gypi
,
May 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8386be414ec1537ad2c033889996f2419b330c42 commit 8386be414ec1537ad2c033889996f2419b330c42 Author: mgiuca <mgiuca@chromium.org> Date: Fri May 20 02:51:11 2016 Added strings for notification when user presses backspace. These will be used in a future CL (https://codereview.chromium.org/1983803002). The strings are being checked in early to ensure they are in before branch point. Notry rationale: Tree is broken due to https://crbug.com/613258. All other tests have passed on trybots. BUG= 610039 NOTRY=true Review-Url: https://codereview.chromium.org/1995893002 Cr-Commit-Position: refs/heads/master@{#394968} [modify] https://crrev.com/8386be414ec1537ad2c033889996f2419b330c42/chrome/app/generated_resources.grd [modify] https://crrev.com/8386be414ec1537ad2c033889996f2419b330c42/ui/strings/ui_strings.grd
,
May 20 2016
Can we get a screenshot of what this looks like on Mac, esp since the option key text will need to be different?
,
May 20 2016
@46: I don't think we have anyone slated to do the Mac work; is there someone who could chip in here?
,
May 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c7e4899df2f7e590de44237b4a2fdfd978ec70df commit c7e4899df2f7e590de44237b4a2fdfd978ec70df Author: ojan <ojan@chromium.org> Date: Fri May 20 19:50:18 2016 Re-add backspace-goes-back as a default disabled finch trial. This is just so that we have a kill switch in case this makes it to stable and there is then unexpected outcry. BUG= 610039 Review-Url: https://codereview.chromium.org/1992003002 Cr-Commit-Position: refs/heads/master@{#395141} [modify] https://crrev.com/c7e4899df2f7e590de44237b4a2fdfd978ec70df/chrome/app/chrome_command_ids.h [modify] https://crrev.com/c7e4899df2f7e590de44237b4a2fdfd978ec70df/chrome/browser/app_mode/app_mode_utils.cc [modify] https://crrev.com/c7e4899df2f7e590de44237b4a2fdfd978ec70df/chrome/browser/global_keyboard_shortcuts_mac.mm [modify] https://crrev.com/c7e4899df2f7e590de44237b4a2fdfd978ec70df/chrome/browser/global_keyboard_shortcuts_mac_unittest.mm [modify] https://crrev.com/c7e4899df2f7e590de44237b4a2fdfd978ec70df/chrome/browser/ui/browser_command_controller.cc [modify] https://crrev.com/c7e4899df2f7e590de44237b4a2fdfd978ec70df/chrome/browser/ui/views/accelerator_table.cc [modify] https://crrev.com/c7e4899df2f7e590de44237b4a2fdfd978ec70df/chrome/browser/ui/views/constrained_window_views_browsertest.cc
,
May 23 2016
#46: Here's what it looks like "on Mac" (in Linux) ... I haven't got it working on an actual Mac yet but this is what the bubble looks like with OS_MACOSX defined. tapted@ (Mac Views guy) seems to think this hookup will be fairly easy. I'll discuss with him about doing it this week and hopefully merging it.
,
May 24 2016
Requesting merge of 8386be414ec1537ad2c033889996f2419b330c42 / r394968 for M52. This is a strings change that just missed the cutoff. I am talking with tinazh@ about whther we can do this.
,
May 24 2016
Your change meets the bar and is auto-approved for M52 (branch: 2743)
,
May 24 2016
Please merge your change to M52 branch 2743 before 4:00 PM PST tomorrow, Wednesday (05/25).So we can take it for this week last M52 Dev release on Thursday (05/26).Thank you.
,
May 25 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/38619b874e6aaeb1765a4b41943cd7171179ef97 commit 38619b874e6aaeb1765a4b41943cd7171179ef97 Author: Matt Giuca <mgiuca@chromium.org> Date: Wed May 25 03:42:39 2016 Added strings for notification when user presses backspace. These will be used in a future CL (https://codereview.chromium.org/1983803002). The strings are being checked in early to ensure they are in before branch point. Notry rationale: Tree is broken due to https://crbug.com/613258. All other tests have passed on trybots. BUG= 610039 NOTRY=true Review-Url: https://codereview.chromium.org/1995893002 Cr-Commit-Position: refs/heads/master@{#394968} (cherry picked from commit 8386be414ec1537ad2c033889996f2419b330c42) Review URL: https://codereview.chromium.org/2009503004 . Cr-Commit-Position: refs/branch-heads/2743@{#48} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/38619b874e6aaeb1765a4b41943cd7171179ef97/chrome/app/generated_resources.grd [modify] https://crrev.com/38619b874e6aaeb1765a4b41943cd7171179ef97/ui/strings/ui_strings.grd
,
May 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7f3ddc6b70a09c7ae3bcd0d45007e415857fd09c commit 7f3ddc6b70a09c7ae3bcd0d45007e415857fd09c Author: mgiuca <mgiuca@chromium.org> Date: Thu May 26 07:22:22 2016 Added notification when user presses backspace to use new shortcut. This adds a new type of notification bubble (based on the "Press Esc to exit fullscreen" notification) that teaches users the new shortcuts for Back and Forward when they press Back or Shift+Back. Only implemented on Views systems (Windows, Linux, Chrome OS). BUG= 610039 Review-Url: https://codereview.chromium.org/1983803002 Cr-Commit-Position: refs/heads/master@{#396138} [modify] https://crrev.com/7f3ddc6b70a09c7ae3bcd0d45007e415857fd09c/chrome/app/chrome_command_ids.h [modify] https://crrev.com/7f3ddc6b70a09c7ae3bcd0d45007e415857fd09c/chrome/browser/ui/browser_command_controller.cc [modify] https://crrev.com/7f3ddc6b70a09c7ae3bcd0d45007e415857fd09c/chrome/browser/ui/browser_window.h [modify] https://crrev.com/7f3ddc6b70a09c7ae3bcd0d45007e415857fd09c/chrome/browser/ui/cocoa/browser_window_cocoa.h [modify] https://crrev.com/7f3ddc6b70a09c7ae3bcd0d45007e415857fd09c/chrome/browser/ui/cocoa/browser_window_cocoa.mm [modify] https://crrev.com/7f3ddc6b70a09c7ae3bcd0d45007e415857fd09c/chrome/browser/ui/views/frame/browser_view.cc [modify] https://crrev.com/7f3ddc6b70a09c7ae3bcd0d45007e415857fd09c/chrome/browser/ui/views/frame/browser_view.h [add] https://crrev.com/7f3ddc6b70a09c7ae3bcd0d45007e415857fd09c/chrome/browser/ui/views/new_back_shortcut_bubble.cc [add] https://crrev.com/7f3ddc6b70a09c7ae3bcd0d45007e415857fd09c/chrome/browser/ui/views/new_back_shortcut_bubble.h [modify] https://crrev.com/7f3ddc6b70a09c7ae3bcd0d45007e415857fd09c/chrome/browser/ui/views/subtle_notification_view.cc [modify] https://crrev.com/7f3ddc6b70a09c7ae3bcd0d45007e415857fd09c/chrome/chrome_browser_ui.gypi [modify] https://crrev.com/7f3ddc6b70a09c7ae3bcd0d45007e415857fd09c/chrome/test/base/test_browser_window.cc [modify] https://crrev.com/7f3ddc6b70a09c7ae3bcd0d45007e415857fd09c/chrome/test/base/test_browser_window.h
,
May 26 2016
Requesting a merge of 7f3ddc6b70a09c7ae3bcd0d45007e415857fd09c / r396138 to M52. Rationale: Requested by odean@ and others for informing users about the backspace change that is already in M52. (Somewhat large change, hopefully this is OK as it's early in the period.) I deleted the existing Merge-Approved and Merge-Merged labels to avoid confusion.
,
May 26 2016
We also need to merge https://codereview.chromium.org/1992003002/, which gives us a kill switch that restores the backspace shortcut.
,
May 27 2016
Hmm. In my local trunk build, triggering this UI shows a flash of the correct content in the bubble, but it's almost immediately redrawn totally-white (and stays that way until it disappears). Matt, did you see anything like this locally? Do you have a Windows box to test (maybe this is OS-specific)?
,
May 27 2016
Found it. AnimationProgressed() was miscoded and used [0, 255] instead of [0.0, 1.0]. I will send a CL for this fix. This should have been visible in local testing, though.
,
May 27 2016
#58 Oh... facepalm: https://codereview.chromium.org/2009333002 This was changed from [0, 255] to [0.0, 1.0] by sky about 10 hours before my CL landed. He updated ExclusiveAccessBubbleViews but (obviously) not NewBackShortcutBubble. That's why I didn't see this in local testing. Let me know if you put up a CL to fix this. It's actually good that we are fixing it in a separate CL because we'll want to merge my CL but not your fix. (And yes, this wouldn't have happened if ExclusiveAccessBubbleViews and NewBackShortcutBubble were sharing this code. Not sure it's worth refactoring given that we'll delete NBSB soon, but might be a good idea.)
,
May 27 2016
Ah, I wondered how this could have been missed :) CL is up. https://codereview.chromium.org/2016203002/
,
May 27 2016
Not closing, as still needed on Mac. I'm looking into that now. Also still not implemented: a) minimum of 2 presses required to see notice, and b) maximum of 5 shows of the notice. (If we do those, we should probably open a new bug to reduce the clutter on this one.)
,
May 27 2016
Your change meets the bar and is auto-approved for M52 (branch: 2743)
,
May 27 2016
Started Mac hookup: https://codereview.chromium.org/2017813004 (No idea if it works or even compiles; WIP.)
,
May 27 2016
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 27 2016
Please have a the CL merged by EOD today(05/27), so it gets tested for dev channel release scheduled on 06/02.
,
May 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d28e06bee3c0807483aed283bafd4c6f72198961 commit d28e06bee3c0807483aed283bafd4c6f72198961 Author: pkasting <pkasting@chromium.org> Date: Fri May 27 22:50:17 2016 Widget opacity goes from 0 to 1, not 0 to 255. This change was made in r395980, but missed a lot of callsites. This also adds DCHECKs for this to the compositor in hopes of catching similar errors sooner in the future. These DCHECKs caught most of the places touched by this patch. BUG= 610039 TEST=Trigger new backspace shortcut bubble; it should fade in and out, not flicker and mostly be white. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Review-Url: https://codereview.chromium.org/2016203002 Cr-Commit-Position: refs/heads/master@{#396590} [modify] https://crrev.com/d28e06bee3c0807483aed283bafd4c6f72198961/ash/drag_drop/drag_image_view.cc [modify] https://crrev.com/d28e06bee3c0807483aed283bafd4c6f72198961/ash/system/user/user_view.cc [modify] https://crrev.com/d28e06bee3c0807483aed283bafd4c6f72198961/ash/wm/overview/window_grid.cc [modify] https://crrev.com/d28e06bee3c0807483aed283bafd4c6f72198961/cc/layers/layer.cc [modify] https://crrev.com/d28e06bee3c0807483aed283bafd4c6f72198961/chrome/browser/ui/views/download/download_started_animation_views.cc [modify] https://crrev.com/d28e06bee3c0807483aed283bafd4c6f72198961/chrome/browser/ui/views/new_back_shortcut_bubble.cc [modify] https://crrev.com/d28e06bee3c0807483aed283bafd4c6f72198961/ui/message_center/views/toast_contents_view.cc [modify] https://crrev.com/d28e06bee3c0807483aed283bafd4c6f72198961/ui/views/widget/widget_unittest.cc
,
May 30 2016
,
May 30 2016
#46: I got this working properly on Mac. Here's a real screenshot. (And I just want to toot my own horn here... I have no Mac and I never use or program on Mac. I wrote this CL on my Linux workstation, transferred it to the test Mac here, and it compiled and worked the first time #selfhighfive.) https://codereview.chromium.org/2017813004
,
May 31 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d8fdd17d87b906d6a0e21bd6366dface55132440 commit d8fdd17d87b906d6a0e21bd6366dface55132440 Author: mgiuca <mgiuca@chromium.org> Date: Tue May 31 00:22:42 2016 Added popup that shows the new Back shortcut on Mac. Implements BrowserWindowCocoa::ShowNewBackShortcutBubble to open the Views implementation of this bubble (similar to how the fullscreen bubble works on Mac). BUG= 610039 Review-Url: https://codereview.chromium.org/2017813004 Cr-Commit-Position: refs/heads/master@{#396759} [modify] https://crrev.com/d8fdd17d87b906d6a0e21bd6366dface55132440/chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.h [modify] https://crrev.com/d8fdd17d87b906d6a0e21bd6366dface55132440/chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm [modify] https://crrev.com/d8fdd17d87b906d6a0e21bd6366dface55132440/chrome/browser/ui/cocoa/browser_window_cocoa.mm
,
May 31 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/021778e93de332bc674d30c81edf813f86a0ae26 commit 021778e93de332bc674d30c81edf813f86a0ae26 Author: Matt Giuca <mgiuca@chromium.org> Date: Tue May 31 02:37:55 2016 Re-add backspace-goes-back as a default disabled finch trial. This is just so that we have a kill switch in case this makes it to stable and there is then unexpected outcry. BUG= 610039 Review-Url: https://codereview.chromium.org/1992003002 Cr-Commit-Position: refs/heads/master@{#395141} (cherry picked from commit c7e4899df2f7e590de44237b4a2fdfd978ec70df) TBR=ojan@chromium.org Review URL: https://codereview.chromium.org/2024763002 . Cr-Commit-Position: refs/branch-heads/2743@{#131} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/021778e93de332bc674d30c81edf813f86a0ae26/chrome/app/chrome_command_ids.h [modify] https://crrev.com/021778e93de332bc674d30c81edf813f86a0ae26/chrome/browser/app_mode/app_mode_utils.cc [modify] https://crrev.com/021778e93de332bc674d30c81edf813f86a0ae26/chrome/browser/global_keyboard_shortcuts_mac.mm [modify] https://crrev.com/021778e93de332bc674d30c81edf813f86a0ae26/chrome/browser/global_keyboard_shortcuts_mac_unittest.mm [modify] https://crrev.com/021778e93de332bc674d30c81edf813f86a0ae26/chrome/browser/ui/browser_command_controller.cc [modify] https://crrev.com/021778e93de332bc674d30c81edf813f86a0ae26/chrome/browser/ui/views/accelerator_table.cc [modify] https://crrev.com/021778e93de332bc674d30c81edf813f86a0ae26/chrome/browser/ui/views/constrained_window_views_browsertest.cc
,
May 31 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0cf3ec2939b1c4ec43b4aa7570ecb80ac1fbb1fb commit 0cf3ec2939b1c4ec43b4aa7570ecb80ac1fbb1fb Author: Matt Giuca <mgiuca@chromium.org> Date: Tue May 31 02:43:54 2016 Added notification when user presses backspace to use new shortcut. This adds a new type of notification bubble (based on the "Press Esc to exit fullscreen" notification) that teaches users the new shortcuts for Back and Forward when they press Back or Shift+Back. Only implemented on Views systems (Windows, Linux, Chrome OS). BUG= 610039 Review-Url: https://codereview.chromium.org/1983803002 Cr-Commit-Position: refs/heads/master@{#396138} (cherry picked from commit 7f3ddc6b70a09c7ae3bcd0d45007e415857fd09c) Review URL: https://codereview.chromium.org/2023763004 . Cr-Commit-Position: refs/branch-heads/2743@{#132} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/0cf3ec2939b1c4ec43b4aa7570ecb80ac1fbb1fb/chrome/app/chrome_command_ids.h [modify] https://crrev.com/0cf3ec2939b1c4ec43b4aa7570ecb80ac1fbb1fb/chrome/browser/ui/browser_command_controller.cc [modify] https://crrev.com/0cf3ec2939b1c4ec43b4aa7570ecb80ac1fbb1fb/chrome/browser/ui/browser_window.h [modify] https://crrev.com/0cf3ec2939b1c4ec43b4aa7570ecb80ac1fbb1fb/chrome/browser/ui/cocoa/browser_window_cocoa.h [modify] https://crrev.com/0cf3ec2939b1c4ec43b4aa7570ecb80ac1fbb1fb/chrome/browser/ui/cocoa/browser_window_cocoa.mm [modify] https://crrev.com/0cf3ec2939b1c4ec43b4aa7570ecb80ac1fbb1fb/chrome/browser/ui/views/frame/browser_view.cc [modify] https://crrev.com/0cf3ec2939b1c4ec43b4aa7570ecb80ac1fbb1fb/chrome/browser/ui/views/frame/browser_view.h [add] https://crrev.com/0cf3ec2939b1c4ec43b4aa7570ecb80ac1fbb1fb/chrome/browser/ui/views/new_back_shortcut_bubble.cc [add] https://crrev.com/0cf3ec2939b1c4ec43b4aa7570ecb80ac1fbb1fb/chrome/browser/ui/views/new_back_shortcut_bubble.h [modify] https://crrev.com/0cf3ec2939b1c4ec43b4aa7570ecb80ac1fbb1fb/chrome/browser/ui/views/subtle_notification_view.cc [modify] https://crrev.com/0cf3ec2939b1c4ec43b4aa7570ecb80ac1fbb1fb/chrome/chrome_browser_ui.gypi [modify] https://crrev.com/0cf3ec2939b1c4ec43b4aa7570ecb80ac1fbb1fb/chrome/test/base/test_browser_window.cc [modify] https://crrev.com/0cf3ec2939b1c4ec43b4aa7570ecb80ac1fbb1fb/chrome/test/base/test_browser_window.h
,
May 31 2016
There are quite a lot of CLs here so I'll give a summary of the CLs and the merges.
Note: I have manually tested this feature at refs/branch-heads/2743@{#132} (52.0.2743.20) and everything seems to be working.
(1) 8386be414ec1537ad2c033889996f2419b330c42 / r394968 (mgiuca, strings)
merge: 38619b874e6aaeb1765a4b41943cd7171179ef97 / 2743@{#48}
(2) c7e4899df2f7e590de44237b4a2fdfd978ec70df / r395141 (ojan, finch fallback)
merge: 021778e93de332bc674d30c81edf813f86a0ae26 / 2743@{#131}
(3) 7f3ddc6b70a09c7ae3bcd0d45007e415857fd09c / r396138 (mgiuca, implementation)
merge: 0cf3ec2939b1c4ec43b4aa7570ecb80ac1fbb1fb / 2743@{#132}
(4) d28e06bee3c0807483aed283bafd4c6f72198961 / r396590 (pkasting, fix opacity)
no merge required
(5) d8fdd17d87b906d6a0e21bd6366dface55132440 / r396759 (mgiuca, mac hookup)
still needs merge
Only one more merge is required: the Mac banner. I will wait to test that on Canary first before requesting a merge.
Closing as fixed (on tip-of-tree). Please file new bugs for any follow-up work, to make discussion and merging easier.
,
May 31 2016
There's also my set of fixes to add heuristics about when we show this, which we'll need to merge; we shouldn't ship this without those. I think it makes sense to still track that on this bug, but since that's my job to implement, taking.
,
May 31 2016
I was anticipating separate bugs for those, but if you're happy to continue reusing this one then that's fine with me. (The problem is that ideally each merge CL should have its own bug; as it is I have to keep removing the merge-merged and merge-approved labels and resetting merge-requested, which gets very confusing.)
,
May 31 2016
Mac screenshot LGTM, fwiw.
,
Jun 1 2016
Requesting merge of d8fdd17d87b906d6a0e21bd6366dface55132440 / r396759 for M52 branch (rationale: compatibility with Views where this feature has already been merged). Tested on Mac Canary 53.0.2754.0.
,
Jun 1 2016
Your change meets the bar and is auto-approved for M52 (branch: 2743)
,
Jun 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3afb8b628beb3c3f63719df77d3e4e46ddb86818 commit 3afb8b628beb3c3f63719df77d3e4e46ddb86818 Author: Matt Giuca <mgiuca@chromium.org> Date: Wed Jun 01 08:06:05 2016 Added popup that shows the new Back shortcut on Mac. Implements BrowserWindowCocoa::ShowNewBackShortcutBubble to open the Views implementation of this bubble (similar to how the fullscreen bubble works on Mac). BUG= 610039 Review-Url: https://codereview.chromium.org/2017813004 Cr-Commit-Position: refs/heads/master@{#396759} (cherry picked from commit d8fdd17d87b906d6a0e21bd6366dface55132440) Review URL: https://codereview.chromium.org/2020273003 . Cr-Commit-Position: refs/branch-heads/2743@{#160} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/3afb8b628beb3c3f63719df77d3e4e46ddb86818/chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.h [modify] https://crrev.com/3afb8b628beb3c3f63719df77d3e4e46ddb86818/chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm [modify] https://crrev.com/3afb8b628beb3c3f63719df77d3e4e46ddb86818/chrome/browser/ui/cocoa/browser_window_cocoa.mm
,
Jun 2 2016
,
Jun 2 2016
verified above issue on windows 8, Mac(10.11.4) and Linux (Ubuntu 14.04 LTS) using beta build # 52.0.2743.24. it seems to be fixed and working as intended.
,
Jun 7 2016
,
Jun 7 2016
Moving this nonessential bug to the next milestone. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 9 2016
#83, sigh, you're right. It's not clear whether we should say "Press [Alt]+[<] to go back" or "Press [←] to go back" (actually promoting the back key over the alt shortcut).
,
Jun 9 2016
Does alt + the back key still work? If so, I don't care about making a change. If it doesn't, then option 1 from comment 84, but I still don't care enough to spend time on this, or say anyone else should.
,
Jun 10 2016
#85 Actually Alt+Back doesn't work. A couple more issues pointed out by tapted@: - Users might mistake '<' for the actual less-than sign, as opposed to the left arrow. (In hindsight, it's a little weird that our Chromebooks have two keys with the same '<' symbol on them.) - Some Chromebook keyboards have triangles instead of arrows. Some are filled, some are hollow. https://www.google.com.au/search?q=chromebook+keyboard&tbm=isch So we can't really make this right. I'm just going to leave this alone and hope users can figure out what we mean.
,
Jun 10 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/36aa72944195f579454c1d3abea5045219fd685a commit 36aa72944195f579454c1d3abea5045219fd685a Author: pkasting <pkasting@chromium.org> Date: Fri Jun 10 05:49:08 2016 Add heuristics to limit showing of new backspace UI bubble. This restricts the bubble to showing 5 times, and only showing when users trigger backspace twice within three seconds (which likely indicates "hey, why isn't the browser going back"). The bubble is also immediately hidden (by fading out) if the user successfully navigates back or forward (by any means) while it's showing. BUG= 610039 , 615760 TEST=New "press alt-left to go back" UI should not appear when pressing backspace (to attempt to go back) unless backspace is pressed twice within three seconds. After the UI is shown 5 times, it should not appear again. TBR=phajdan.jr Review-Url: https://codereview.chromium.org/2041293002 Cr-Commit-Position: refs/heads/master@{#399114} [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/browser_command_controller.cc [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/browser_view_prefs.cc [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/browser_window.h [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.h [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/cocoa/browser_window_cocoa.h [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/cocoa/browser_window_cocoa.mm [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/views/frame/browser_view.cc [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/views/frame/browser_view.h [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/views/new_back_shortcut_bubble.cc [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/views/new_back_shortcut_bubble.h [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/common/pref_names.cc [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/common/pref_names.h [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/test/base/test_browser_window.cc [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/test/base/test_browser_window.h
,
Jun 10 2016
Requesting merge to M52 for the commit in comment 87. This should finish off the backspace-goes-back changes for 52 unless we decide to try for the AltGr fix.
,
Jun 11 2016
Your change meets the bar and is auto-approved for M52 (branch: 2743)
,
Jun 11 2016
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 13 2016
The following two CLs weren't merged back for Mac, which makes merging my change problematic. I think we need to merge these. https://codereview.chromium.org/2001423002 https://codereview.chromium.org/2028963002
,
Jun 14 2016
#91 > https://codereview.chromium.org/2001423002 I definitely want to avoid merging this. This was a cleanup CL I did which has nothing to do with the go-back bubble. I believe it depends on r396355 which is also not merged. Your CL shouldn't be strongly depending on this; if it's causing you a merge conflict I think you should just resolve the conflict in-branch. > https://codereview.chromium.org/2028963002 That one is small and related to go-back bubble and I'm happy to merge it if necessary.
,
Jun 14 2016
@92: My concern with not merging the first change is that it leaves us in a state where if IsSimplifiedFullscreenUIEnabled() is false, the backspace-goes-back bubble potentially interacts poorly with it. My reasoning went, if this code is dead in 52, then there's no problem merging the change to remove it. If it's not dead, then there's a problem with my change, and indeed with the backspace-goes-back bubble in general, in how they interact with it. If you are sure there can't possibly be any conflict (I would look at my trunk CL), then we could maybe not merge that, but it makes me nervous.
,
Jun 14 2016
Well, see comment #3: in M52, on Mac, the Views fullscreen bubble is controlled by a finch flag and a user-settable flag. It has been at 100% and on by default for some time, but we are still controlling it with a flag. The flag was removed early in the M53 cycle (r396355), and then I started a massive cleanup operation (see Issue 610900 ). As with all flags, it doesn't have to work great if users have the flag flipped. Technically it doesn't even have to not crash. I'd be happy with the backspace bubble not working at all if you have that flag disabled. But I don't want to merge the removal of the flag and all my cleanup CLs into M52. That's far too big a change. (We are already pretty much abusing the merge process to get this feature and all the follow-up fixes merged; merging a mostly-unrelated large-scale cleanup goes too far for me.) I think you should just merge your changes and fix merge conflicts in favour of not caring what happens in the flag-disabled case. I already did a similar thing with my merge earlier (see https://codereview.chromium.org/2028963002 vs https://codereview.chromium.org/2063103002). Note: IsSimplifiedFullscreenEnabled() is still expected to work on Windows, Linux, Chrome OS in M52 and M53, but unlike Mac it just has a minor effect on the dialog which should not affect this feature at all.
,
Jun 14 2016
I would have elected to go a different route, since to me merging reams of code to remove a flag is far better than just breaking the flag. But you're more of the owner of that flag than me, so it's not my call to make. Given that this is the course you want to follow, why don't you merge https://codereview.chromium.org/2028963002 and I'll then merge my CL. (Or at least I will once a drover fix lands that gets it working on Windows, unless you want to just merge both our CLs.)
,
Jun 14 2016
#95 I already merged that CL (https://codereview.chromium.org/2063103002). Go ahead and do yours. If you want me to look over the merged CL before it is submitted, add me as a reviewer.
,
Jun 14 2016
Tested the above issue on Windows 7, Mac 10.11.5 & Ubuntu 14.04 with chrome version '53.0.2767.0' and alert is displayed when backspace is pressed twice. Hence marking the same as TE-Verified-53.0.2767.0. Attach is the screen-print for the same. Thank you!
,
Jun 14 2016
Please merge your CL as soon as you can by 4 PM today so that it gets picked for the beta promotion scheduled on 6/15.
,
Jun 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6895d9eea0c14482c02bb6b27e756f3fd2abdff2 commit 6895d9eea0c14482c02bb6b27e756f3fd2abdff2 Author: Peter Kasting <pkasting@chromium.org> Date: Tue Jun 14 22:30:38 2016 Add heuristics to limit showing of new backspace UI bubble. This restricts the bubble to showing 5 times, and only showing when users trigger backspace twice within three seconds (which likely indicates "hey, why isn't the browser going back"). The bubble is also immediately hidden (by fading out) if the user successfully navigates back or forward (by any means) while it's showing. BUG= 610039 , 615760 TEST=New "press alt-left to go back" UI should not appear when pressing backspace (to attempt to go back) unless backspace is pressed twice within three seconds. After the UI is shown 5 times, it should not appear again. TBR=phajdan.jr Review-Url: https://codereview.chromium.org/2041293002 Cr-Commit-Position: refs/heads/master@{#399114} (cherry picked from commit 36aa72944195f579454c1d3abea5045219fd685a) Review URL: https://codereview.chromium.org/2065283002 . Cr-Commit-Position: refs/branch-heads/2743@{#358} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/browser_command_controller.cc [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/browser_view_prefs.cc [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/browser_window.h [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.h [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/cocoa/browser_window_cocoa.h [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/cocoa/browser_window_cocoa.mm [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/views/frame/browser_view.cc [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/views/frame/browser_view.h [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/views/new_back_shortcut_bubble.cc [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/views/new_back_shortcut_bubble.h [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/common/pref_names.cc [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/common/pref_names.h [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/test/base/test_browser_window.cc [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/test/base/test_browser_window.h
,
Jun 14 2016
I'm calling this done. The only remaining nice-to-have is AltGr support.
,
Jun 14 2016
> The only remaining nice-to-have is AltGr support. which is tracked in bug 613201
,
Jun 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/36aa72944195f579454c1d3abea5045219fd685a commit 36aa72944195f579454c1d3abea5045219fd685a Author: pkasting <pkasting@chromium.org> Date: Fri Jun 10 05:49:08 2016 Add heuristics to limit showing of new backspace UI bubble. This restricts the bubble to showing 5 times, and only showing when users trigger backspace twice within three seconds (which likely indicates "hey, why isn't the browser going back"). The bubble is also immediately hidden (by fading out) if the user successfully navigates back or forward (by any means) while it's showing. BUG= 610039 , 615760 TEST=New "press alt-left to go back" UI should not appear when pressing backspace (to attempt to go back) unless backspace is pressed twice within three seconds. After the UI is shown 5 times, it should not appear again. TBR=phajdan.jr Review-Url: https://codereview.chromium.org/2041293002 Cr-Commit-Position: refs/heads/master@{#399114} [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/browser_command_controller.cc [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/browser_view_prefs.cc [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/browser_window.h [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.h [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/cocoa/browser_window_cocoa.h [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/cocoa/browser_window_cocoa.mm [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/views/frame/browser_view.cc [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/views/frame/browser_view.h [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/views/new_back_shortcut_bubble.cc [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/browser/ui/views/new_back_shortcut_bubble.h [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/common/pref_names.cc [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/common/pref_names.h [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/test/base/test_browser_window.cc [modify] https://crrev.com/36aa72944195f579454c1d3abea5045219fd685a/chrome/test/base/test_browser_window.h
,
Jun 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6895d9eea0c14482c02bb6b27e756f3fd2abdff2 commit 6895d9eea0c14482c02bb6b27e756f3fd2abdff2 Author: Peter Kasting <pkasting@chromium.org> Date: Tue Jun 14 22:30:38 2016 Add heuristics to limit showing of new backspace UI bubble. This restricts the bubble to showing 5 times, and only showing when users trigger backspace twice within three seconds (which likely indicates "hey, why isn't the browser going back"). The bubble is also immediately hidden (by fading out) if the user successfully navigates back or forward (by any means) while it's showing. BUG= 610039 , 615760 TEST=New "press alt-left to go back" UI should not appear when pressing backspace (to attempt to go back) unless backspace is pressed twice within three seconds. After the UI is shown 5 times, it should not appear again. TBR=phajdan.jr Review-Url: https://codereview.chromium.org/2041293002 Cr-Commit-Position: refs/heads/master@{#399114} (cherry picked from commit 36aa72944195f579454c1d3abea5045219fd685a) Review URL: https://codereview.chromium.org/2065283002 . Cr-Commit-Position: refs/branch-heads/2743@{#358} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/browser_command_controller.cc [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/browser_view_prefs.cc [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/browser_window.h [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.h [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/cocoa/browser/exclusive_access_controller_views.mm [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/cocoa/browser_window_cocoa.h [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/cocoa/browser_window_cocoa.mm [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/views/frame/browser_view.cc [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/views/frame/browser_view.h [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/views/new_back_shortcut_bubble.cc [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/browser/ui/views/new_back_shortcut_bubble.h [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/common/pref_names.cc [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/common/pref_names.h [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/test/base/test_browser_window.cc [modify] https://crrev.com/6895d9eea0c14482c02bb6b27e756f3fd2abdff2/chrome/test/base/test_browser_window.h
,
Jun 15 2016
Tested the above issue on Windows 7, Mac 10.11.5 & Ubuntu 14.04 with chrome version '52.0.2743.41' and alert is displayed when backspace is pressed twice. Hence marking the same as TE-Verified-52.0.2743.41. Thank you!
,
Feb 17 2017
happened to see this today: // TODO(mgiuca): Remove these in M54 ( https://crbug.com/610039 ). #define IDC_BACKSPACE_BACK 33010 #define IDC_BACKSPACE_FORWARD 33011 I guess that can happen now?
,
Feb 17 2017
I'm kind of intentionally putting it off -- I don't think it hurts much to have this in there and it's helpful for people coming from other browsers.
,
Feb 19 2017
> I'm kind of intentionally putting it off Me too. I don't think it's a problem to leave it in indefinitely (since we only show it 5 times per user). I would be happy to just delete the TODOs.
,
Feb 20 2017
For UIs like this that we'll rarely see ourselves, one worry (in the long term) is maintenance. +hwi@ as FYI
,
Feb 21 2017
Engineering or UX maintenance? The eng side of this is so minor that it's basically zero, but I'm happy to volunteer to do fixes on it if needed. It's really trivial code. The UX side of this is hopefully minimal as it just reuses the code and styling of the fullscreen bubble. If we change that, it will pick it up for free. If that's not appropriate, then at the point it happens it'd probably be OK to just rip this out instead of worrying about it.
,
Mar 1 2017
Sounds like the decision is to delete the TODO?
,
Mar 1 2017
Maybe? It's kind of a "we probably could delete this code someday, just who knows when". It would be especially easy to delete it if we were in a world where no other browser supported this shortcut either. I dunno how likely that world is.
,
Mar 1 2017
OK, I'll remove the TODO. I also noticed the comment references the BackspaceGoesBack field trial, which I believe was put in there in case we needed to roll this back: https://cs.chromium.org/chromium/src/chrome/common/chrome_features.cc?q=BackspaceGoesBack&l=60 I filed Issue 697655 to resolve this.
,
Mar 1 2017
,
Mar 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d5f275985e8e58c1ef94444d28b7f9cbc66982dc commit d5f275985e8e58c1ef94444d28b7f9cbc66982dc Author: mgiuca <mgiuca@chromium.org> Date: Mon Mar 06 01:43:55 2017 Remove TODOs to remove the "new back shortcut bubble". This feature was put in temporarily to teach users the new shortcut for back/forward. We decided on the linked bug to keep it indefinitely since it's a helpful teaching aid, users will only see it a few times, and the maintenance cost is low. BUG= 610039 Review-Url: https://codereview.chromium.org/2723193003 Cr-Commit-Position: refs/heads/master@{#454807} [modify] https://crrev.com/d5f275985e8e58c1ef94444d28b7f9cbc66982dc/chrome/app/chrome_command_ids.h [modify] https://crrev.com/d5f275985e8e58c1ef94444d28b7f9cbc66982dc/chrome/browser/ui/views/new_back_shortcut_bubble.h
Showing comments 15 - 114
of 114
Older ›
|
||||||||||||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||||||||||||
