Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1431103003:
Chrome Apps Page: Fixed issue with app focusing and context. (Closed)
|
Created:
5 years, 1 month ago by karandeepb Modified:
5 years, 1 month ago Reviewers:
Evan Stade CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, pedrosimonetti+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChrome Apps Page: Fixed issue with app focusing and context.
On the chrome://apps page, if we right click on an app, and then
navigate to the app again, the app is not highlighted/focused. This
patch fixes this issue. Also, currently, if we click outside the
tiles, the active tile still retains the context. This patch fixes
this and ensures that the context is transferred to the page
correctly.
BUG=487194, 507681
Committed: https://crrev.com/4813d9128008df1b65dbeb24ed924b8913676fdd
Cr-Commit-Position: refs/heads/master@{#359042}
Patch Set 1 #
Total comments: 3
Messages
Total messages: 18 (9 generated)
Description was changed from ========== Removed space to conform with style guidelines Fixed bug 507681 and 487194 BUG= ========== to ========== Chrome Apps Page: Fixed issue with app focusing and context. On the chrome://apps page, if we right click on an app, and then navigate to the app again, the app is not highlighted/focused. This patch fixes this issue. Also, currently, if we click outside the tiles, the active tile still retains the context. This patch fixes this and ensures that the context is transferred to the page correctly. BUG=487194, 507681 TEST=Goto chrome://apps/. Right click on an app. The app context menu opens. Left click on the page once(outside the tiles). Press menu key. Notice the app still has context. TEST=Goto chrome://apps/. Right click on an app. The app context menu opens. Left click on the page twice(outside the tiles). Press menu key. Notice the page context menu opens this time. TEST=Goto chrome://apps. Right click on an app. Press escape to close the app context menu. Press right arrow key and then left arrow key. Notice the app is currently focused/highlighted. ==========
Description was changed from ========== Chrome Apps Page: Fixed issue with app focusing and context. On the chrome://apps page, if we right click on an app, and then navigate to the app again, the app is not highlighted/focused. This patch fixes this issue. Also, currently, if we click outside the tiles, the active tile still retains the context. This patch fixes this and ensures that the context is transferred to the page correctly. BUG=487194, 507681 TEST=Goto chrome://apps/. Right click on an app. The app context menu opens. Left click on the page once(outside the tiles). Press menu key. Notice the app still has context. TEST=Goto chrome://apps/. Right click on an app. The app context menu opens. Left click on the page twice(outside the tiles). Press menu key. Notice the page context menu opens this time. TEST=Goto chrome://apps. Right click on an app. Press escape to close the app context menu. Press right arrow key and then left arrow key. Notice the app is currently focused/highlighted. ========== to ========== Chrome Apps Page: Fixed issue with app focusing and context. On the chrome://apps page, if we right click on an app, and then navigate to the app again, the app is not highlighted/focused. This patch fixes this issue. Also, currently, if we click outside the tiles, the active tile still retains the context. This patch fixes this and ensures that the context is transferred to the page correctly. BUG=487194, 507681 TEST=Goto chrome://apps/. Right click on an app. The app context menu opens. Left click on the page once(outside the tiles). Press menu key. Notice the app still has context. TEST=Goto chrome://apps/. Right click on an app. The app context menu opens. Left click on the page twice(outside the tiles). Press menu key. Notice the page context menu opens this time. TEST=Goto chrome://apps. Right click on an app. Press escape to close the app context menu. Press right arrow key and then left arrow key. Notice the app is currently focused/highlighted. ==========
Description was changed from ========== Chrome Apps Page: Fixed issue with app focusing and context. On the chrome://apps page, if we right click on an app, and then navigate to the app again, the app is not highlighted/focused. This patch fixes this issue. Also, currently, if we click outside the tiles, the active tile still retains the context. This patch fixes this and ensures that the context is transferred to the page correctly. BUG=487194, 507681 TEST=Goto chrome://apps/. Right click on an app. The app context menu opens. Left click on the page once(outside the tiles). Press menu key. Notice the app still has context. TEST=Goto chrome://apps/. Right click on an app. The app context menu opens. Left click on the page twice(outside the tiles). Press menu key. Notice the page context menu opens this time. TEST=Goto chrome://apps. Right click on an app. Press escape to close the app context menu. Press right arrow key and then left arrow key. Notice the app is currently focused/highlighted. ========== to ========== Chrome Apps Page: Fixed issue with app focusing and context. On the chrome://apps page, if we right click on an app, and then navigate to the app again, the app is not highlighted/focused. This patch fixes this issue. Also, currently, if we click outside the tiles, the active tile still retains the context. This patch fixes this and ensures that the context is transferred to the page correctly. BUG=487194, 507681 TEST=Goto chrome://apps/. Right click on an app. The app context menu opens. Left click on the page once(outside the tiles). Press menu key. Notice the app still has context. TEST=Goto chrome://apps/. Right click on an app. The app context menu opens. Left click on the page twice(outside the tiles). Press menu key. Notice the page context menu opens this time. TEST=Goto chrome://apps. Right click on an app. Press escape to close the app context menu. Press right arrow key and then left arrow key. Notice the app is currently focused/highlighted. ==========
karandeepb@chromium.org changed reviewers: + dbeam@chromium.org
PTAL. Thanks.
dbeam@chromium.org changed reviewers: + estade@chromium.org - dbeam@chromium.org
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
-dbeam, +estade (wrote this code)
https://codereview.chromium.org/1431103003/diff/1/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/tile_page.js (left): https://codereview.chromium.org/1431103003/diff/1/chrome/browser/resources/nt... chrome/browser/resources/ntp4/tile_page.js:708: // inside the grid but outside of any tile. does this still work? where does focus go if you click between the tiles now
https://codereview.chromium.org/1431103003/diff/1/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/tile_page.js (left): https://codereview.chromium.org/1431103003/diff/1/chrome/browser/resources/nt... chrome/browser/resources/ntp4/tile_page.js:708: // inside the grid but outside of any tile. On 2015/11/09 23:29:32, Evan Stade wrote: > does this still work? where does focus go if you click between the tiles now On clicking between the tiles now, the focus reaches the tile page. This is done to address the bug https://code.google.com/p/chromium/issues/detail?id=487194
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
nit: I think the TEST= steps in your CL description add unnecessary clutter. The repro steps detailed in the bug reports should be enough. lgtm https://codereview.chromium.org/1431103003/diff/1/chrome/browser/resources/nt... File chrome/browser/resources/ntp4/tile_page.js (left): https://codereview.chromium.org/1431103003/diff/1/chrome/browser/resources/nt... chrome/browser/resources/ntp4/tile_page.js:708: // inside the grid but outside of any tile. On 2015/11/10 14:23:20, karandeepb wrote: > On 2015/11/09 23:29:32, Evan Stade wrote: > > does this still work? where does focus go if you click between the tiles now > > On clicking between the tiles now, the focus reaches the tile page. This is done > to address the bug https://code.google.com/p/chromium/issues/detail?id=487194 honestly, that bug is not worth fixing, but I can't figure out what the purpose of this block of code is anyway, so it might as well be deleted.
Description was changed from ========== Chrome Apps Page: Fixed issue with app focusing and context. On the chrome://apps page, if we right click on an app, and then navigate to the app again, the app is not highlighted/focused. This patch fixes this issue. Also, currently, if we click outside the tiles, the active tile still retains the context. This patch fixes this and ensures that the context is transferred to the page correctly. BUG=487194, 507681 TEST=Goto chrome://apps/. Right click on an app. The app context menu opens. Left click on the page once(outside the tiles). Press menu key. Notice the app still has context. TEST=Goto chrome://apps/. Right click on an app. The app context menu opens. Left click on the page twice(outside the tiles). Press menu key. Notice the page context menu opens this time. TEST=Goto chrome://apps. Right click on an app. Press escape to close the app context menu. Press right arrow key and then left arrow key. Notice the app is currently focused/highlighted. ========== to ========== Chrome Apps Page: Fixed issue with app focusing and context. On the chrome://apps page, if we right click on an app, and then navigate to the app again, the app is not highlighted/focused. This patch fixes this issue. Also, currently, if we click outside the tiles, the active tile still retains the context. This patch fixes this and ensures that the context is transferred to the page correctly. BUG=487194, 507681 ==========
On 2015/11/11 01:14:57, Evan Stade wrote: > nit: I think the TEST= steps in your CL description add unnecessary clutter. The > repro steps detailed in the bug reports should be enough. > > lgtm > > https://codereview.chromium.org/1431103003/diff/1/chrome/browser/resources/nt... > File chrome/browser/resources/ntp4/tile_page.js (left): > > https://codereview.chromium.org/1431103003/diff/1/chrome/browser/resources/nt... > chrome/browser/resources/ntp4/tile_page.js:708: // inside the grid but outside > of any tile. > On 2015/11/10 14:23:20, karandeepb wrote: > > On 2015/11/09 23:29:32, Evan Stade wrote: > > > does this still work? where does focus go if you click between the tiles now > > > > On clicking between the tiles now, the focus reaches the tile page. This is > done > > to address the bug https://code.google.com/p/chromium/issues/detail?id=487194 > > honestly, that bug is not worth fixing, but I can't figure out what the purpose > of this block of code is anyway, so it might as well be deleted. I have removed the TEST= steps from the CL description. Thanks for the review.
The CQ bit was checked by karandeepb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1431103003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1431103003/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/4813d9128008df1b65dbeb24ed924b8913676fdd Cr-Commit-Position: refs/heads/master@{#359042} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
