Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2962673003:
[DevTools] Bring ARIA config up to date with ARIA 1.1 (Closed)
|
Created:
3 years, 5 months ago by aboxhall Modified:
3 years, 5 months ago CC:
chromium-reviews, caseq+blink_chromium.org, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, pfeldman+blink_chromium.org, nektarios, je_julie, dougt+watch_chromium.org, aleventhal+watch_chromium.org, dtseng+watch_chromium.org, devtools-reviews_chromium.org, blink-reviews, apavlov+blink_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman, dmazzoni, dmazzoni+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionBring ARIA config up to date with ARIA 1.1 (https://www.w3.org/TR/wai-aria-1.1)
BUG=616950
Review-Url: https://codereview.chromium.org/2962673003
Cr-Commit-Position: refs/heads/master@{#485174}
Committed: https://chromium.googlesource.com/chromium/src/+/11353980a216e622907d7077541128c4384c7850
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address review comments #Patch Set 3 : Add aria-setsize and aria-posinset to row role #
Total comments: 30
Patch Set 4 : aleventhal comments #Messages
Total messages: 28 (12 generated)
aboxhall@chromium.org changed reviewers: + dmazzoni@chromium.org, lushnikov@chromium.org
Description was changed from ========== Bring ARIA config up to date with ARIA 1.1 BUG=616950 ========== to ========== Bring ARIA config up to date with ARIA 1.1 BUG=616950 ==========
aboxhall@chromium.org changed reviewers: + aleventhal@chromium.org - dmazzoni@chromium.org
Description was changed from ========== Bring ARIA config up to date with ARIA 1.1 BUG=616950 ========== to ========== Bring ARIA config up to date with ARIA 1.1 (https://www.w3.org/TR/wai-aria-1.1) BUG=616950 ==========
https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js (right): https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:1: Accessibility.ARIAMetadata._config = { shouldn't we have a LICENSE here? Also, how do you get this file? Is it generated or manually crafted?
On 2017/06/28 18:19:05, lushnikov wrote: > https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... > File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js > (right): > > https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:1: > Accessibility.ARIAMetadata._config = { > shouldn't we have a LICENSE here? > > Also, how do you get this file? Is it generated or manually crafted? Manual.
https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js (right): https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:1: Accessibility.ARIAMetadata._config = { On 2017/06/28 18:19:05, lushnikov wrote: > shouldn't we have a LICENSE here? Oops, not sure how that was missed originally. Will fix; does this LGTY otherwise?
On 2017/06/28 21:04:49, aboxhall wrote: > https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... > File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js > (right): > > https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:1: > Accessibility.ARIAMetadata._config = { > On 2017/06/28 18:19:05, lushnikov wrote: > > shouldn't we have a LICENSE here? > > Oops, not sure how that was missed originally. > > Will fix; does this LGTY otherwise? I think we should add @aria-setsize and @aria-posinset to "row". We support it, and IIRC Firefox does as well. Here's the bug filed on the ARIA spec: https://github.com/w3c/aria/issues/558
yes, lgtm, but we should get license header back :)
lgtm I went over the properties line-by-line, but not all of the roles. If you want even more review, I can do it tomorrow (sorry). I think the best way to ensure these are correct are to try to use these rules in our C++ implementation. When it doesn't pass tests, that will help us identify any errors. https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js (left): https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:60: 'combobox': { Note: according to @nektarios, we have implemented this so that the textfield itself can be role="combobox", rather than a combobox containing a textbox. In addition, Google implementations in google.com assume this. I'm going to be looking at this. https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js (right): https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:25: 'aria-hidden': {'default': 'undefined', 'enum': ['true', 'false', 'undefined'], 'type': 'token'}, Is there special logic somewhere so that 'undefined' is not treated as a possible literal value, but rather means the attribute is not present? I'm actually not sure whether the 'undefined' literal string should be treated as undefined or not, and sent an email to public-aria for clarification. https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:61: 'implicitValues': {'aria-live': 'assertive ', 'aria-atomic': 'true'} There is a space in 'assertive '
On 2017/06/28 22:54:14, lushnikov wrote: > yes, lgtm, but we should get license header back :) For trivial issues like this, I think it's generally fine to LGTM and trust people to address the issue before they commit. I'm often reading reviews on my phone or laptop before I get to work in the morning to try and catch people in the US before they leave work, so I can't always address things right away.
https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js (left): https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:60: 'combobox': { On 2017/06/29 20:18:13, aleventhal wrote: > Note: according to @nektarios, we have implemented this so that the textfield > itself can be role="combobox", rather than a combobox containing a textbox. In > addition, Google implementations in http://google.com assume this. I'm going to be > looking at this. Added TODO to follow up. https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js (right): https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:25: 'aria-hidden': {'default': 'undefined', 'enum': ['true', 'false', 'undefined'], 'type': 'token'}, On 2017/06/29 20:18:13, aleventhal wrote: > Is there special logic somewhere so that 'undefined' is not treated as a > possible literal value, but rather means the attribute is not present? > > I'm actually not sure whether the 'undefined' literal string should be treated > as undefined or not, and sent an email to public-aria for clarification. Hm, the spec says: The "undefined" value, when allowed on a state or property, is an explicit indication that the state or property is not set. The value is used on states and properties that support tokens, and the "undefined" value is a string that is one of the allowed tokens. It is also used on some states and properties that accept true/false values, when "undefined" has a different meaning than "false". https://www.w3.org/TR/wai-aria-1.1/#propcharacteristic_value but also States and properties specifically required for the role and subclass roles. Content authors must provide a non-empty value for required states and properties. Content authors must not use the value "undefined" for required states and properties. https://www.w3.org/TR/wai-aria-1.1/#requiredState So I conclude that you can explicitly set "undefined" *unless* it the property is required for the role. https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:61: 'implicitValues': {'aria-live': 'assertive ', 'aria-atomic': 'true'} On 2017/06/29 20:18:13, aleventhal wrote: > There is a space in 'assertive ' Done.
On 2017/06/29 20:18:13, aleventhal wrote: > lgtm > > I went over the properties line-by-line, but not all of the roles. If you want > even more review, I can do it tomorrow (sorry). Yeah, if you could take a look at the roles I would appreciate it! Your feedback is very helpful. > I think the best way to ensure these are correct are to try to use these rules > in our C++ implementation. When it doesn't pass tests, that will help us > identify any errors. Definitely agreed, but that's a ways down the line (working on it). > https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... > File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js > (left): > > https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:60: > 'combobox': { > Note: according to @nektarios, we have implemented this so that the textfield > itself can be role="combobox", rather than a combobox containing a textbox. In > addition, Google implementations in http://google.com assume this. I'm going to be > looking at this. > > https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... > File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js > (right): > > https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:25: > 'aria-hidden': {'default': 'undefined', 'enum': ['true', 'false', 'undefined'], > 'type': 'token'}, > Is there special logic somewhere so that 'undefined' is not treated as a > possible literal value, but rather means the attribute is not present? > > I'm actually not sure whether the 'undefined' literal string should be treated > as undefined or not, and sent an email to public-aria for clarification. > > https://codereview.chromium.org/2962673003/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:61: > 'implicitValues': {'aria-live': 'assertive ', 'aria-atomic': 'true'} > There is a space in 'assertive '
On 2017/06/29 23:58:27, aboxhall wrote: > Hm, the spec says: > > The "undefined" value, when allowed on a state or property, is an explicit > indication that the state or property is not set. The value is used on states > and properties that support tokens, and the "undefined" value is a string that > is one of the allowed tokens. It is also used on some states and properties that > accept true/false values, when "undefined" has a different meaning than "false". > > https://www.w3.org/TR/wai-aria-1.1/#propcharacteristic_value > > but also > > States and properties specifically required for the role and subclass roles. > Content authors must provide a non-empty value for required states and > properties. Content authors must not use the value "undefined" for required > states and properties. > > https://www.w3.org/TR/wai-aria-1.1/#requiredState > > So I conclude that you can explicitly set "undefined" *unless* it the property > is required for the role. Agreed. I don't believe our implementation supports this so I'll be filing an issue and looking at that.
Ok, found stuff after going over each line. It was a Friday afternoon before a 4 day holiday, so frozen yogurt and a beer was involved.. https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js (left): https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:53: 'checkbox': {'nameFrom': ['contents', 'author'], 'requiredAttributes': ['aria-checked'], 'superclasses': ['input']}, FYI only, I just sent the following email to public-aria@w3.org: *Change in ARIA 1.1 breaks checkbox/radio with embedded field* ARIA 1.1 says checkbox and radio have childrenPresentational: true, whereas ARIA 1.0 didn't. Unfortunately, this will break the following use case: [ ] Delete mail after ___ days The ___ can be a textfield, combobox, etc. It's the reason that a value is part of the name computation. Unfortunately, when the parent checkbox/radio is childrenPresentational, we truncate the tree at that point and make it a leaf. So there is no object to fire events for when the embedded control gets focused. <div role="checkbox">Delete mail after <input> days</div> https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js (right): https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:126: 'document': { Document has nameRequired: false, believe or not https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:138: 'mustContain': ['row'], Spec says required owned elements are: row rowgroup → row I guess it means that it can contain either rowgroup or row children. If rowgroups, then those will contain the rows. Is there a way to codify this? https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:159: 'landmark': landmark should be only nameFrom 'author' https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:168: 'mustContain': ['listitem'], Can contain a group group → listitem listitem https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:172: 'implicitValues': {'aria-orientation': 'vertical'} No name required for list. https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:190: 'superclasses': ['region'], superclass is section https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:196: 'math': {'nameFrom': ['author'], 'superclasses': ['section'], 'nameRequired': true}, math is childrenPresentational: true, although I have no idea why. Seems like a problem. https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:206: 'mustContain': ['group', 'menuitem', 'menuitemradio', 'menuitemcheckbox'], This looks correct, but I want to point out that here you include group, which is listed as "group → menuitemradio". In other cases where there is an → , you don't include the item preceding the arrow. https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:285: ['aria-colindex', 'aria-level', 'aria-rowindex', 'aria-selected', 'aria-setsize', 'aria-posinset'] We can't add a comment in this JSON, correct? Otherwise I'd add a comment that we include setsize/posinset for treegrid purposes and have filed the issue for that. https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:318: // TODO(aboxhall): separator properties depend on focusability Maybe for now put aria-value* as supported instead of required? https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:328: 'implicitValues': {'aria-orientation': 'horizontal', 'aria-valuemin': '0', 'aria-valuemax': '100'} Well! You learn something new every day. Creating a todo for myself since I doubt we do that. https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:334: 'supportedAttributes': ['aria-required'], Needs aria-readonly https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:363: 'mustContain': ['row'], rowgroup? https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:402: {'mustContain': ['row'], 'nameFrom': ['author'], 'superclasses': ['grid', 'tree'], 'nameRequired': true}, treegrid can contain rowgroup
The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the thorough review, that was really helpful! I realise you already LGTM'd, but any last comments before I submit? https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js (left): https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:53: 'checkbox': {'nameFrom': ['contents', 'author'], 'requiredAttributes': ['aria-checked'], 'superclasses': ['input']}, On 2017/06/30 20:30:19, aleventhal wrote: > FYI only, I just sent the following email to mailto:public-aria@w3.org: > > *Change in ARIA 1.1 breaks checkbox/radio with embedded field* > > ARIA 1.1 says checkbox and radio have childrenPresentational: true, whereas ARIA > 1.0 didn't. > > Unfortunately, this will break the following use case: > > [ ] Delete mail after ___ days > > The ___ can be a textfield, combobox, etc. It's the reason that a value is part > of the name computation. Unfortunately, when the parent checkbox/radio is > childrenPresentational, we truncate the tree at that point and make it a leaf. > So there is no object to fire events for when the embedded control gets focused. > > <div role="checkbox">Delete mail after <input> days</div> Ah, noted. I'll take it out for now. https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js (right): https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:126: 'document': { On 2017/06/30 20:30:20, aleventhal wrote: > Document has nameRequired: false, believe or not Good catch, thanks. Fixed. https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:138: 'mustContain': ['row'], On 2017/06/30 20:30:19, aleventhal wrote: > Spec says required owned elements are: > row > rowgroup → row > > I guess it means that it can contain either rowgroup or row children. If > rowgroups, then those will contain the rows. Is there a way to codify this? Yeah, I punted on that for this CL, and for all similar cases. I'll add a TODO here to come back to it. https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:159: 'landmark': On 2017/06/30 20:30:19, aleventhal wrote: > landmark should be only nameFrom 'author' Done. https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:168: 'mustContain': ['listitem'], On 2017/06/30 20:30:20, aleventhal wrote: > Can contain a group > group → listitem > listitem TODO added. https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:172: 'implicitValues': {'aria-orientation': 'vertical'} On 2017/06/30 20:30:20, aleventhal wrote: > No name required for list. Done. https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:190: 'superclasses': ['region'], On 2017/06/30 20:30:20, aleventhal wrote: > superclass is section Done. https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:196: 'math': {'nameFrom': ['author'], 'superclasses': ['section'], 'nameRequired': true}, On 2017/06/30 20:30:20, aleventhal wrote: > math is childrenPresentational: true, although I have no idea why. Seems like a > problem. Hm, yeah, it does. Added with a TODO. https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:206: 'mustContain': ['group', 'menuitem', 'menuitemradio', 'menuitemcheckbox'], On 2017/06/30 20:30:20, aleventhal wrote: > This looks correct, but I want to point out that here you include group, which > is listed as "group → menuitemradio". In other cases where there is an → , you > don't include the item preceding the arrow. Removed and added TODO, thanks. https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:285: ['aria-colindex', 'aria-level', 'aria-rowindex', 'aria-selected', 'aria-setsize', 'aria-posinset'] On 2017/06/30 20:30:19, aleventhal wrote: > We can't add a comment in this JSON, correct? > Otherwise I'd add a comment that we include setsize/posinset for treegrid > purposes and have filed the issue for that. Luckily this is JavaScript (for now) :P Comment added. https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:318: // TODO(aboxhall): separator properties depend on focusability On 2017/06/30 20:30:20, aleventhal wrote: > Maybe for now put aria-value* as supported instead of required? Good idea, done. https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:328: 'implicitValues': {'aria-orientation': 'horizontal', 'aria-valuemin': '0', 'aria-valuemax': '100'} On 2017/06/30 20:30:19, aleventhal wrote: > Well! You learn something new every day. Creating a todo for myself since I > doubt we do that. :) https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:334: 'supportedAttributes': ['aria-required'], On 2017/06/30 20:30:20, aleventhal wrote: > Needs aria-readonly Done. https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:363: 'mustContain': ['row'], On 2017/06/30 20:30:20, aleventhal wrote: > rowgroup? TODO added. https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:402: {'mustContain': ['row'], 'nameFrom': ['author'], 'superclasses': ['grid', 'tree'], 'nameRequired': true}, On 2017/06/30 20:30:19, aleventhal wrote: > treegrid can contain rowgroup TODO added.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/07/06 01:58:53, aboxhall wrote: > Thanks for the thorough review, that was really helpful! I realise you already > LGTM'd, but any last comments before I submit? > > https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js > (left): > > https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:53: > 'checkbox': {'nameFrom': ['contents', 'author'], 'requiredAttributes': > ['aria-checked'], 'superclasses': ['input']}, > On 2017/06/30 20:30:19, aleventhal wrote: > > FYI only, I just sent the following email to mailto:public-aria@w3.org: > > > > *Change in ARIA 1.1 breaks checkbox/radio with embedded field* > > > > ARIA 1.1 says checkbox and radio have childrenPresentational: true, whereas > ARIA > > 1.0 didn't. > > > > Unfortunately, this will break the following use case: > > > > [ ] Delete mail after ___ days > > > > The ___ can be a textfield, combobox, etc. It's the reason that a value is > part > > of the name computation. Unfortunately, when the parent checkbox/radio is > > childrenPresentational, we truncate the tree at that point and make it a leaf. > > So there is no object to fire events for when the embedded control gets > focused. > > > > <div role="checkbox">Delete mail after <input> days</div> > > Ah, noted. I'll take it out for now. > > https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js > (right): > > https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:126: > 'document': { > On 2017/06/30 20:30:20, aleventhal wrote: > > Document has nameRequired: false, believe or not > > Good catch, thanks. Fixed. > > https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:138: > 'mustContain': ['row'], > On 2017/06/30 20:30:19, aleventhal wrote: > > Spec says required owned elements are: > > row > > rowgroup → row > > > > I guess it means that it can contain either rowgroup or row children. If > > rowgroups, then those will contain the rows. Is there a way to codify this? > > Yeah, I punted on that for this CL, and for all similar cases. I'll add a TODO > here to come back to it. > > https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:159: > 'landmark': > On 2017/06/30 20:30:19, aleventhal wrote: > > landmark should be only nameFrom 'author' > > Done. > > https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:168: > 'mustContain': ['listitem'], > On 2017/06/30 20:30:20, aleventhal wrote: > > Can contain a group > > group → listitem > > listitem > > TODO added. > > https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:172: > 'implicitValues': {'aria-orientation': 'vertical'} > On 2017/06/30 20:30:20, aleventhal wrote: > > No name required for list. > > Done. > > https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:190: > 'superclasses': ['region'], > On 2017/06/30 20:30:20, aleventhal wrote: > > superclass is section > > Done. > > https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:196: > 'math': {'nameFrom': ['author'], 'superclasses': ['section'], 'nameRequired': > true}, > On 2017/06/30 20:30:20, aleventhal wrote: > > math is childrenPresentational: true, although I have no idea why. Seems like > a > > problem. > > Hm, yeah, it does. Added with a TODO. > > https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:206: > 'mustContain': ['group', 'menuitem', 'menuitemradio', 'menuitemcheckbox'], > On 2017/06/30 20:30:20, aleventhal wrote: > > This looks correct, but I want to point out that here you include group, which > > is listed as "group → menuitemradio". In other cases where there is an → , > you > > don't include the item preceding the arrow. > > Removed and added TODO, thanks. > > https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:285: > ['aria-colindex', 'aria-level', 'aria-rowindex', 'aria-selected', > 'aria-setsize', 'aria-posinset'] > On 2017/06/30 20:30:19, aleventhal wrote: > > We can't add a comment in this JSON, correct? > > Otherwise I'd add a comment that we include setsize/posinset for treegrid > > purposes and have filed the issue for that. > > Luckily this is JavaScript (for now) :P > > Comment added. > > https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:318: // > TODO(aboxhall): separator properties depend on focusability > On 2017/06/30 20:30:20, aleventhal wrote: > > Maybe for now put aria-value* as supported instead of required? > > Good idea, done. > > https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:328: > 'implicitValues': {'aria-orientation': 'horizontal', 'aria-valuemin': '0', > 'aria-valuemax': '100'} > On 2017/06/30 20:30:19, aleventhal wrote: > > Well! You learn something new every day. Creating a todo for myself since I > > doubt we do that. > > :) > > https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:334: > 'supportedAttributes': ['aria-required'], > On 2017/06/30 20:30:20, aleventhal wrote: > > Needs aria-readonly > > Done. > > https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:363: > 'mustContain': ['row'], > On 2017/06/30 20:30:20, aleventhal wrote: > > rowgroup? > > TODO added. > > https://codereview.chromium.org/2962673003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/accessibility/ARIAConfig.js:402: > {'mustContain': ['row'], 'nameFrom': ['author'], 'superclasses': ['grid', > 'tree'], 'nameRequired': true}, > On 2017/06/30 20:30:19, aleventhal wrote: > > treegrid can contain rowgroup > > TODO added. lgtm
The CQ bit was checked by aboxhall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2962673003/#ps60001 (title: "aleventhal comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499645282987280,
"parent_rev": "9bf17495890e9474e25d673b25c0cfdeca767a9d", "commit_rev":
"11353980a216e622907d7077541128c4384c7850"}
Message was sent while issue was closed.
Description was changed from ========== Bring ARIA config up to date with ARIA 1.1 (https://www.w3.org/TR/wai-aria-1.1) BUG=616950 ========== to ========== Bring ARIA config up to date with ARIA 1.1 (https://www.w3.org/TR/wai-aria-1.1) BUG=616950 Review-Url: https://codereview.chromium.org/2962673003 Cr-Commit-Position: refs/heads/master@{#485174} Committed: https://chromium.googlesource.com/chromium/src/+/11353980a216e622907d70775411... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/11353980a216e622907d70775411... |
