Issue 29323156: Issue 2410 - Improved accessibility of dialogs in options page (Closed)
|
Created:
July 31, 2015, 3:02 p.m. by Thomas Greiner Modified:
Oct. 2, 2015, 2:59 p.m. Reviewers:
saroyanm CC:
Felix Dahlke Visibility:
Public. |
DescriptionIn addition to the changes described in the ticket, I also added a keyboard shortcut so that you can press ESC to close an open dialog.
Patch Set 1 #Patch Set 2 : Rebased to f8e3e591fcbb #
Total comments: 21
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
MessagesTotal messages: 7
https://codereview.adblockplus.org/29323156/diff/29328570/options.html File options.html (right): https://codereview.adblockplus.org/29323156/diff/29328570/options.html#newcod... options.html:326: <input type="search" id="find-language" class="default-action" /> As far as I can see default-action is used for default or initial focus on the element I think make sense to rename it to something more descriptive. https://codereview.adblockplus.org/29323156/diff/29328570/options.js File options.js (right): https://codereview.adblockplus.org/29323156/diff/29328570/options.js#newcode500 options.js:500: document.querySelector("#dialog .focus-last").focus(); you already has the dialog element, I think you can refer to the ".focus-last" and ".focus-first" classes using "this" keyword: this.querySelector(".focus-last"); and this.querySelector(".focus-first"); https://codereview.adblockplus.org/29323156/diff/29328570/options.js#newcode521 options.js:521: var defaultAction = document.querySelector("#dialog-content-" + name what about naming this defaultFocus or initialFocus ? https://codereview.adblockplus.org/29323156/diff/29328570/options.js#newcode524 options.js:524: defaultAction = document.querySelector("#dialog .focus-first"); I think here you also can use dialog element "dialog.querySelector(".focus-first");" https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css#ne... skin/options.css:483: #dialog-body button::before Nit: should not exceed the 80 char limit if you put inline with the line before. https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css#ne... skin/options.css:514: #dialog-body button::before Nit: should not exceed the 80 char limit if you put inline with the line before. https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css#ne... skin/options.css:522: -moz-margin-end: 12px; Just as remark: I think from some point we should also use standardized version as well: margin-inline-end (same for other similar elements). https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css#ne... skin/options.css:1007: #dialog label Nit: should not exceed the 80 char limit if you put inline with the line before. https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css#ne... skin/options.css:1063: text-align: left; Please also consider the case for RTL.
https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css#ne... skin/options.css:483: #dialog-body button::before On 2015/10/01 16:31:40, saroyanm wrote: > Nit: should not exceed the 80 char limit if you put inline with the line before. > Just noticed that the consistency in either case is missing for selectors, feel free to ignore my comments regarding that.
https://codereview.adblockplus.org/29323156/diff/29328570/options.html File options.html (right): https://codereview.adblockplus.org/29323156/diff/29328570/options.html#newcod... options.html:326: <input type="search" id="find-language" class="default-action" /> On 2015/10/01 16:31:40, saroyanm wrote: > As far as I can see default-action is used for default or initial focus on the > element I think make sense to rename it to something more descriptive. Done. https://codereview.adblockplus.org/29323156/diff/29328570/options.js File options.js (right): https://codereview.adblockplus.org/29323156/diff/29328570/options.js#newcode500 options.js:500: document.querySelector("#dialog .focus-last").focus(); On 2015/10/01 16:31:40, saroyanm wrote: > you already has the dialog element, I think you can refer to the ".focus-last" > and ".focus-first" classes using "this" keyword: > this.querySelector(".focus-last"); and this.querySelector(".focus-first"); Done. https://codereview.adblockplus.org/29323156/diff/29328570/options.js#newcode521 options.js:521: var defaultAction = document.querySelector("#dialog-content-" + name On 2015/10/01 16:31:40, saroyanm wrote: > what about naming this defaultFocus or initialFocus ? Done. https://codereview.adblockplus.org/29323156/diff/29328570/options.js#newcode524 options.js:524: defaultAction = document.querySelector("#dialog .focus-first"); On 2015/10/01 16:31:40, saroyanm wrote: > I think here you also can use dialog element > "dialog.querySelector(".focus-first");" Done. https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css#ne... skin/options.css:514: #dialog-body button::before On 2015/10/01 16:31:41, saroyanm wrote: > Nit: should not exceed the 80 char limit if you put inline with the line before. Sure but it's more readable if each selector is on its own line, don't you think so? https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css#ne... skin/options.css:522: -moz-margin-end: 12px; On 2015/10/01 16:31:40, saroyanm wrote: > Just as remark: I think from some point we should also use standardized version > as well: > margin-inline-end (same for other similar elements). Unfortunately, there's no standardized version yet. All there is right now is a draft so at this point it's not stable enough to rely on a non-prefixed version. https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css#ne... skin/options.css:1063: text-align: left; On 2015/10/01 16:31:40, saroyanm wrote: > Please also consider the case for RTL. Done.
https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css#ne... skin/options.css:514: #dialog-body button::before On 2015/10/02 10:27:29, Thomas Greiner wrote: > On 2015/10/01 16:31:41, saroyanm wrote: > > Nit: should not exceed the 80 char limit if you put inline with the line > before. > > Sure but it's more readable if each selector is on its own line, don't you think > so? I agree with you, from readability point of view having every selector on it's own line make more sense, but it makes file also bigger, so I will vote for current approach if we are okey having bigger files. https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css#ne... skin/options.css:522: -moz-margin-end: 12px; On 2015/10/02 10:27:29, Thomas Greiner wrote: > On 2015/10/01 16:31:40, saroyanm wrote: > > Just as remark: I think from some point we should also use standardized > version > > as well: > > margin-inline-end (same for other similar elements). > > Unfortunately, there's no standardized version yet. All there is right now is a > draft so at this point it's not stable enough to rely on a non-prefixed version. I see, make sense also not use border-inline-start-width in first run page, I think we used that property there, that why I thought it standardized. https://codereview.adblockplus.org/29323156/diff/29328807/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29323156/diff/29328807/skin/options.css#ne... skin/options.css:1063: text-align: start; According to MDN "start" value for text-align property is part of experimental API, so I think using "initial" will be more safe here: text-align: initial; reference: https://developer.mozilla.org/en/docs/Web/CSS/text-align
https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css#ne... skin/options.css:514: #dialog-body button::before On 2015/10/02 13:18:50, saroyanm wrote: > On 2015/10/02 10:27:29, Thomas Greiner wrote: > > On 2015/10/01 16:31:41, saroyanm wrote: > > > Nit: should not exceed the 80 char limit if you put inline with the line > > before. > > > > Sure but it's more readable if each selector is on its own line, don't you > think > > so? > > I agree with you, from readability point of view having every selector on it's > own line make more sense, but it makes file also bigger, so I will vote for > current approach if we are okey having bigger files. Bigger files should not be an issue as long as the code inside is well structured and easily understandable. https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css#ne... skin/options.css:522: -moz-margin-end: 12px; On 2015/10/02 13:18:50, saroyanm wrote: > On 2015/10/02 10:27:29, Thomas Greiner wrote: > > On 2015/10/01 16:31:40, saroyanm wrote: > > > Just as remark: I think from some point we should also use standardized > > version > > > as well: > > > margin-inline-end (same for other similar elements). > > > > Unfortunately, there's no standardized version yet. All there is right now is > a > > draft so at this point it's not stable enough to rely on a non-prefixed > version. > > I see, make sense also not use border-inline-start-width in first run page, I > think we used that property there, that why I thought it standardized. I guess we can tackle that whenever we make changes to firstRun.css https://codereview.adblockplus.org/29323156/diff/29328807/skin/options.css File skin/options.css (right): https://codereview.adblockplus.org/29323156/diff/29328807/skin/options.css#ne... skin/options.css:1063: text-align: start; On 2015/10/02 13:18:51, saroyanm wrote: > According to MDN "start" value for text-align property is part of experimental > API, so I think using "initial" will be more safe here: > text-align: initial; > > reference: > https://developer.mozilla.org/en/docs/Web/CSS/text-align Done.
On 2015/10/02 14:17:33, Thomas Greiner wrote: > https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css > File skin/options.css (right): > > https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css#ne... > skin/options.css:514: #dialog-body button::before > On 2015/10/02 13:18:50, saroyanm wrote: > > On 2015/10/02 10:27:29, Thomas Greiner wrote: > > > On 2015/10/01 16:31:41, saroyanm wrote: > > > > Nit: should not exceed the 80 char limit if you put inline with the line > > > before. > > > > > > Sure but it's more readable if each selector is on its own line, don't you > > think > > > so? > > > > I agree with you, from readability point of view having every selector on it's > > own line make more sense, but it makes file also bigger, so I will vote for > > current approach if we are okey having bigger files. > > Bigger files should not be an issue as long as the code inside is well > structured and easily understandable. > > https://codereview.adblockplus.org/29323156/diff/29328570/skin/options.css#ne... > skin/options.css:522: -moz-margin-end: 12px; > On 2015/10/02 13:18:50, saroyanm wrote: > > On 2015/10/02 10:27:29, Thomas Greiner wrote: > > > On 2015/10/01 16:31:40, saroyanm wrote: > > > > Just as remark: I think from some point we should also use standardized > > > version > > > > as well: > > > > margin-inline-end (same for other similar elements). > > > > > > Unfortunately, there's no standardized version yet. All there is right now > is > > a > > > draft so at this point it's not stable enough to rely on a non-prefixed > > version. > > > > I see, make sense also not use border-inline-start-width in first run page, I > > think we used that property there, that why I thought it standardized. > > I guess we can tackle that whenever we make changes to firstRun.css > > https://codereview.adblockplus.org/29323156/diff/29328807/skin/options.css > File skin/options.css (right): > > https://codereview.adblockplus.org/29323156/diff/29328807/skin/options.css#ne... > skin/options.css:1063: text-align: start; > On 2015/10/02 13:18:51, saroyanm wrote: > > According to MDN "start" value for text-align property is part of experimental > > API, so I think using "initial" will be more safe here: > > text-align: initial; > > > > reference: > > https://developer.mozilla.org/en/docs/Web/CSS/text-align > > Done. LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
