Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(20)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 544293002: [New Tab Page] Change which elements get focused during tab ordering (Closed)

Created:
6 years, 3 months ago by Mathieu
Modified:
6 years, 3 months ago
Reviewers:
dmazzoni, huangs
CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, dominich, jfweitz+watch_chromium.org, David Black, arv+watch_chromium.org, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

[New Tab Page] Change which elements get focused during tab ordering Accessibility improvement. BUG=326793 Committed: https://crrev.com/cbdb6d62787e1562ad23bb5517d31b698c6babad Cr-Commit-Position: refs/heads/master@{#294588}

Patch Set 1 #

Patch Set 2 : tabindex = 0, background #

Patch Set 3 : Fixed backspace/delete #

Patch Set 4 : comment #

Total comments: 8

Patch Set 5 : navigateContentWindow on keyDown #

Total comments: 2

Patch Set 6 : addressed comments #

Total comments: 10

Patch Set 7 : nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+78 lines, -25 lines) Patch
M chrome/browser/resources/local_ntp/local_ntp.css View 1 2 3 4 5 2 chunks +12 lines, -9 lines 0 comments Download
M chrome/browser/resources/local_ntp/local_ntp.js View 1 2 3 4 5 6 9 chunks +44 lines, -12 lines 0 comments Download
M chrome/browser/resources/local_ntp/most_visited_util.js View 1 2 3 4 5 2 chunks +22 lines, -4 lines 0 comments Download

Messages

Total messages: 18 (3 generated)
Mathieu
Sam, Despite my best efforts, I'm pretty sure I've missed some things. In particular, I ...
6 years, 3 months ago (2014-09-05 19:07:16 UTC) #2
huangs
tabindex="0" is special in that here "0" behaves like "+infinity". See: ******** <!doctype HTML> <html> ...
6 years, 3 months ago (2014-09-08 16:01:49 UTC) #3
Mathieu
+Dominic Please review. Tab index is 0 per Dominic's comments on email.
6 years, 3 months ago (2014-09-11 19:49:11 UTC) #5
dmazzoni
On 2014/09/05 19:07:16, Mathieu Perreault wrote: > Sam, > > Despite my best efforts, I'm ...
6 years, 3 months ago (2014-09-11 20:28:58 UTC) #6
Mathieu
On 2014/09/11 20:28:58, dmazzoni wrote: > On 2014/09/05 19:07:16, Mathieu Perreault wrote: > > Sam, ...
6 years, 3 months ago (2014-09-11 20:31:24 UTC) #7
huangs
First set of comments. https://codereview.chromium.org/544293002/diff/60001/chrome/browser/resources/local_ntp/local_ntp.css File chrome/browser/resources/local_ntp/local_ntp.css (right): https://codereview.chromium.org/544293002/diff/60001/chrome/browser/resources/local_ntp/local_ntp.css#newcode307 chrome/browser/resources/local_ntp/local_ntp.css:307: .default-theme.classical .mv-focused ~ .mv-mask { ...
6 years, 3 months ago (2014-09-11 20:31:41 UTC) #8
dmazzoni
On 2014/09/08 16:01:49, huangs1 wrote: > tabindex="0" is special in that here "0" behaves like ...
6 years, 3 months ago (2014-09-11 20:31:51 UTC) #9
dmazzoni
https://codereview.chromium.org/544293002/diff/80001/chrome/browser/resources/local_ntp/most_visited_util.js File chrome/browser/resources/local_ntp/most_visited_util.js (right): https://codereview.chromium.org/544293002/diff/80001/chrome/browser/resources/local_ntp/most_visited_util.js#newcode172 chrome/browser/resources/local_ntp/most_visited_util.js:172: } else if (event.keyCode == 13 /* ENTER */) ...
6 years, 3 months ago (2014-09-11 20:33:15 UTC) #10
Mathieu
Thanks, have a look! https://codereview.chromium.org/544293002/diff/60001/chrome/browser/resources/local_ntp/local_ntp.css File chrome/browser/resources/local_ntp/local_ntp.css (right): https://codereview.chromium.org/544293002/diff/60001/chrome/browser/resources/local_ntp/local_ntp.css#newcode307 chrome/browser/resources/local_ntp/local_ntp.css:307: .default-theme.classical .mv-focused ~ .mv-mask { ...
6 years, 3 months ago (2014-09-11 20:54:10 UTC) #11
dmazzoni
lgtm
6 years, 3 months ago (2014-09-11 21:05:29 UTC) #12
huangs
Nice work! LGTM with nitty nits. https://codereview.chromium.org/544293002/diff/100001/chrome/browser/resources/local_ntp/local_ntp.js File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/544293002/diff/100001/chrome/browser/resources/local_ntp/local_ntp.js#newcode416 chrome/browser/resources/local_ntp/local_ntp.js:416: '.mv-page-ready:hover .mv-mask, .mv-focused ...
6 years, 3 months ago (2014-09-11 21:20:04 UTC) #13
Mathieu
Thanks! Submitting ... https://codereview.chromium.org/544293002/diff/100001/chrome/browser/resources/local_ntp/local_ntp.js File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/544293002/diff/100001/chrome/browser/resources/local_ntp/local_ntp.js#newcode416 chrome/browser/resources/local_ntp/local_ntp.js:416: '.mv-page-ready:hover .mv-mask, .mv-focused ~ .mv-mask {' ...
6 years, 3 months ago (2014-09-12 13:28:22 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/544293002/120001
6 years, 3 months ago (2014-09-12 13:38:05 UTC) #16
commit-bot: I haz the power
Committed patchset #7 (id:120001) as 4a35fddc1e3589c913155ea6d3be3ad74d1f2781
6 years, 3 months ago (2014-09-12 14:38:25 UTC) #17
commit-bot: I haz the power
6 years, 3 months ago (2014-09-12 14:46:37 UTC) #18
Message was sent while issue was closed. 
Patchset 7 (id:??) landed as
https://crrev.com/cbdb6d62787e1562ad23bb5517d31b698c6babad
Cr-Commit-Position: refs/heads/master@{#294588}

Powered by Google App Engine
This is Rietveld 408576698