Add ARIA panel to accessibility sidebar pane

Add ARIA panel to accessibility sidebar pane.

Incomplete, but usable:
- Can edit attribute values only.
- Can not add extra attributes yet.
- No autocomplete yet.
- No validation yet.
- No moving between attributes via keyboard yet.

Minimal usable version to get started, will add missing features in follow-ups.

BUG=616950
Committed: https://crrev.com/c9d64446c281cd18bb374ac0370bf9fb4cf5cb32
Cr-Commit-Position: refs/heads/master@{#407891}

[aboxhall, 2016-07-15 23:32:05]

Description was changed from

==========
WIP ARIA panel

BUG=
==========

to

==========
Add ARIA panel to accessibility sidebar pane

BUG=616950
==========

[aboxhall, 2016-07-18 18:23:07]

Description was changed from

==========
Add ARIA panel to accessibility sidebar pane

BUG=616950
==========

to

==========
Add ARIA panel to accessibility sidebar pane.

Incomplete, but usable:
- Can edit attribute values only.
- Can not add extra attributes yet.
- No autocomplete yet.
- No validation yet.
- No moving between attributes via keyboard yet.

Minimal usable version to get started, will add missing features in follow-ups.

BUG=616950
==========

[aboxhall, 2016-07-18 21:42:58]

aboxhall@chromium.org changed reviewers:
+ dgozman@chromium.org

[dgozman, 2016-07-18 22:38:20]

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js
(right):

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:9:
WebInspector.AXARIAAttributesSubPane = function()
ARIAAttributesPane

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:30:
var foundARIAAttr = false;
Avoid abbreviations: attr -> attribute. I'd call this just |foundAttributes| or
|found|.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:40:
if (foundARIAAttr) {
Can just check |this._treeOutline.rootElement().childCount()|.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:45:
this._treeOutline.element.classList.add("hidden");
Use toggle instead:
this._treeOutline.element.classList.toggle("hidden", !foundARIAAttr);

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:56:
* @param {!Object} attribute
Let's make it a typedef. I've found that DOMNode.attributes() has incorrect
annotation (it returns an array). Could you please add a typedef for
DOMNode.Attribute (with name and value) and use it here?

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:77:
_populateListItem: function() {
{ on next line

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:90:
this.nameElement = createElement("span");
_nameElement

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:99:
* @return {?Element}
- !Element
- Is it used at all? Let's not return anything.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:103:
this.valueElement =
WebInspector.ARIAAttributesTreeElement.createARIAValueElement(value);
_valueElement

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:108:
_mouseClick: function(event)
Annotate please.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:118:
startEditing: function()
All these editing methods should be private.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:138:
text = this.value || text;
What is |this.value| ?

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:158:

nit: extra blank line

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:172:
this._parentPane.node().setAttribute(this._attribute.name, newText);
Let's use setAttributeValue to not craft html here.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:181:
editedElement.parentElement.classList.remove("child-editing");
Is "child-editing" used anywhere?

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:212:
this.editingCancelled();
Just call this instead of |result = "commit"|.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:215:
this.editingCommitted(event.target.textContent, result);
Same for cancel.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:219:
event.consume();
And consume in both cases. Note: consume(true) will also preventDefault
automatically.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:265:
var prefix = wordRange.toString().toLowerCase();
Let's remove this code for now, until we have an implementation. Just call the
callback with an empty array.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:279:
WebInspector.AXARIAAttributesSubPane.ARIAAttributes = [
WebInspector.ARIAAttributesPane._attributes

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilityNodeView.js
(right):

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilityNodeView.js:19:
this.element.classList.add("computed");
We use more specific classes to make search easier:
accessibility-computed

[aboxhall, 2016-07-19 17:20:54]

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js
(right):

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:9:
WebInspector.AXARIAAttributesSubPane = function()
On 2016/07/18 22:38:19, dgozman wrote:
> ARIAAttributesPane

Done.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:30:
var foundARIAAttr = false;
On 2016/07/18 22:38:19, dgozman wrote:
> Avoid abbreviations: attr -> attribute. I'd call this just |foundAttributes|
or
> |found|.

Done.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:40:
if (foundARIAAttr) {
On 2016/07/18 22:38:20, dgozman wrote:
> Can just check |this._treeOutline.rootElement().childCount()|.

Done.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:45:
this._treeOutline.element.classList.add("hidden");
On 2016/07/18 22:38:19, dgozman wrote:
> Use toggle instead:
> this._treeOutline.element.classList.toggle("hidden", !foundARIAAttr);

Nice, TIL. Done.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:56:
* @param {!Object} attribute
On 2016/07/18 22:38:19, dgozman wrote:
> Let's make it a typedef. I've found that DOMNode.attributes() has incorrect
> annotation (it returns an array). Could you please add a typedef for
> DOMNode.Attribute (with name and value) and use it here?

Done.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:77:
_populateListItem: function() {
On 2016/07/18 22:38:20, dgozman wrote:
> { on next line

Done.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:90:
this.nameElement = createElement("span");
On 2016/07/18 22:38:20, dgozman wrote:
> _nameElement

Done.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:99:
* @return {?Element}
On 2016/07/18 22:38:19, dgozman wrote:
> - !Element
> - Is it used at all? Let's not return anything.

Done.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:103:
this.valueElement =
WebInspector.ARIAAttributesTreeElement.createARIAValueElement(value);
On 2016/07/18 22:38:19, dgozman wrote:
> _valueElement

Done.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:108:
_mouseClick: function(event)
On 2016/07/18 22:38:20, dgozman wrote:
> Annotate please.

Done.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:118:
startEditing: function()
On 2016/07/18 22:38:19, dgozman wrote:
> All these editing methods should be private.

Done.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:138:
text = this.value || text;
On 2016/07/18 22:38:20, dgozman wrote:
> What is |this.value| ?

Ugh, should have been this._attribute.value, but I actually don't think it does
anything; removed this line.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:158:

On 2016/07/18 22:38:19, dgozman wrote:
> nit: extra blank line

Done.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:172:
this._parentPane.node().setAttribute(this._attribute.name, newText);
On 2016/07/18 22:38:19, dgozman wrote:
> Let's use setAttributeValue to not craft html here.

Done.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:181:
editedElement.parentElement.classList.remove("child-editing");
On 2016/07/18 22:38:20, dgozman wrote:
> Is "child-editing" used anywhere?

Hm, doesn't look like it.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:212:
this.editingCancelled();
On 2016/07/18 22:38:19, dgozman wrote:
> Just call this instead of |result = "commit"|.

Done.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:215:
this.editingCommitted(event.target.textContent, result);
On 2016/07/18 22:38:19, dgozman wrote:
> Same for cancel.

Done.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:219:
event.consume();
On 2016/07/18 22:38:20, dgozman wrote:
> And consume in both cases. Note: consume(true) will also preventDefault
> automatically.

Done.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:265:
var prefix = wordRange.toString().toLowerCase();
On 2016/07/18 22:38:20, dgozman wrote:
> Let's remove this code for now, until we have an implementation. Just call the
> callback with an empty array.

Done.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:279:
WebInspector.AXARIAAttributesSubPane.ARIAAttributes = [
On 2016/07/18 22:38:20, dgozman wrote:
> WebInspector.ARIAAttributesPane._attributes

Done.

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilityNodeView.js
(right):

https://codereview.chromium.org/2058323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilityNodeView.js:19:
this.element.classList.add("computed");
On 2016/07/18 22:38:20, dgozman wrote:
> We use more specific classes to make search easier:
> accessibility-computed

Done.

[dgozman, 2016-07-19 18:13:12]

This looks pretty good, but let's add a test which will dump the new treeoutline
on some test pages with aria attributes.

https://codereview.chromium.org/2058323002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js
(right):

https://codereview.chromium.org/2058323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:168:
_editingEnded: function()
Looks like a useless method now.

https://codereview.chromium.org/2058323002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js
(right):

https://codereview.chromium.org/2058323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:13:
this._ariaSubPane = null
style: missing semicolon

https://codereview.chromium.org/2058323002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/DOMModel.js (right):

https://codereview.chromium.org/2058323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMModel.js:61:
this._attributes = [];
Let's annotate this one here to ensure compiler checks the usages for us.

https://codereview.chromium.org/2058323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMModel.js:433: * @return
{!Array<!WebInspector.DOMNode.Attribute>}
Thanks!

[aboxhall, 2016-07-20 22:11:34]

On 2016/07/19 18:13:12, dgozman wrote:
> This looks pretty good, but let's add a test which will dump the new
treeoutline
> on some test pages with aria attributes.

> This looks pretty good, but let's add a test which will dump the new
treeoutline
> on some test pages with aria attributes.

I've been working on this, but it seems like the accessibility-related code
isn't loaded by default because it's not a 'normal' sidebar pane. Any ideas?


Exception while running: function nodeSelected(node)
    {
        targetNode = node;
        var target = node.target();
        var accessibilityModel =
WebInspector.AccessibilityModel.fromTarget(target);
        accessibilityModel.getAXNode(node.id)
                          .then(callback);
    }
TypeError: Cannot read property 'fromTarget' of undefined
    at nodeSelected (evaluateInWebInspector0.js:2036:65)
    at result (evaluateInWebInspector0.js:605:25)

[dgozman, 2016-07-20 22:14:32]

On 2016/07/20 22:11:34, aboxhall wrote:
> On 2016/07/19 18:13:12, dgozman wrote:
> > This looks pretty good, but let's add a test which will dump the new
> treeoutline
> > on some test pages with aria attributes.
> 
> > This looks pretty good, but let's add a test which will dump the new
> treeoutline
> > on some test pages with aria attributes.
> 
> I've been working on this, but it seems like the accessibility-related code
> isn't loaded by default because it's not a 'normal' sidebar pane. Any ideas?
> 
> 
> Exception while running: function nodeSelected(node)
>     {
>         targetNode = node;
>         var target = node.target();
>         var accessibilityModel =
> WebInspector.AccessibilityModel.fromTarget(target);
>         accessibilityModel.getAXNode(node.id)
>                           .then(callback);
>     }
> TypeError: Cannot read property 'fromTarget' of undefined
>     at nodeSelected (evaluateInWebInspector0.js:2036:65)
>     at result (evaluateInWebInspector0.js:605:25)

Right! You'll have to put your tests in inspector/elements/accessibility and
enable experiment by default here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...

[aboxhall, 2016-07-21 21:38:48]

On 2016/07/20 22:14:32, dgozman wrote:
> On 2016/07/20 22:11:34, aboxhall wrote:
> > On 2016/07/19 18:13:12, dgozman wrote:
> > > This looks pretty good, but let's add a test which will dump the new
> > treeoutline
> > > on some test pages with aria attributes.
> > 
> > > This looks pretty good, but let's add a test which will dump the new
> > treeoutline
> > > on some test pages with aria attributes.
> > 
> > I've been working on this, but it seems like the accessibility-related code
> > isn't loaded by default because it's not a 'normal' sidebar pane. Any ideas?
> > 
> > 
> > Exception while running: function nodeSelected(node)
> >     {
> >         targetNode = node;
> >         var target = node.target();
> >         var accessibilityModel =
> > WebInspector.AccessibilityModel.fromTarget(target);
> >         accessibilityModel.getAXNode(node.id)
> >                           .then(callback);
> >     }
> > TypeError: Cannot read property 'fromTarget' of undefined
> >     at nodeSelected (evaluateInWebInspector0.js:2036:65)
> >     at result (evaluateInWebInspector0.js:605:25)
> 
> Right! You'll have to put your tests in inspector/elements/accessibility and
> enable experiment by default here:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front...

Done, but still having issues: the test runs fine in the debugger, but when
running as a LayoutTest it can't find the accessibility pane at all.

I've uploaded what I have; PTAL?

[aboxhall, 2016-07-21 21:39:27]

https://codereview.chromium.org/2058323002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js
(right):

https://codereview.chromium.org/2058323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:168:
_editingEnded: function()
On 2016/07/19 18:13:12, dgozman wrote:
> Looks like a useless method now.

Haha, yes. Removed.

https://codereview.chromium.org/2058323002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js
(right):

https://codereview.chromium.org/2058323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:13:
this._ariaSubPane = null
On 2016/07/19 18:13:12, dgozman wrote:
> style: missing semicolon

Done.

https://codereview.chromium.org/2058323002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sdk/DOMModel.js (right):

https://codereview.chromium.org/2058323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMModel.js:61:
this._attributes = [];
On 2016/07/19 18:13:12, dgozman wrote:
> Let's annotate this one here to ensure compiler checks the usages for us.

Done.

https://codereview.chromium.org/2058323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sdk/DOMModel.js:433: * @return
{!Array<!WebInspector.DOMNode.Attribute>}
On 2016/07/19 18:13:12, dgozman wrote:
> Thanks!

Sure! :D

[dgozman, 2016-07-22 16:44:30]

https://codereview.chromium.org/2058323002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js
(right):

https://codereview.chromium.org/2058323002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js:7:
var sidebarPaneWrappers =
WebInspector.panels.elements._elementsSidebarViewWrappers;
These panes are loaded asynchronously. We should wait until all the panes are
loaded in ElementsPanel._loadSidebarViews. 

I suggest to do there:
Promise.all([<all-promises-go-here>]).then(this._sidebarViewsLoadedForTest.bind(this))

And then sniff for _sidebarViewsLoadedForTest method right here. That means
showAccessibilityWidget should take a callback.

https://codereview.chromium.org/2058323002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector/elements/accessibility/edit-aria-attributes.html
(right):

https://codereview.chromium.org/2058323002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector/elements/accessibility/edit-aria-attributes.html:49:
Tests that writing an ARIA attribute causes the accessibility node to be
updated.
Expectations file is missing in the patch.

[aboxhall, 2016-07-22 23:58:52]

On 2016/07/22 16:44:30, dgozman wrote:
>
https://codereview.chromium.org/2058323002/diff/80001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js
> (right):
> 
>
https://codereview.chromium.org/2058323002/diff/80001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js:7:
> var sidebarPaneWrappers =
> WebInspector.panels.elements._elementsSidebarViewWrappers;
> These panes are loaded asynchronously. We should wait until all the panes are
> loaded in ElementsPanel._loadSidebarViews. 
> 
> I suggest to do there:
>
Promise.all([<all-promises-go-here>]).then(this._sidebarViewsLoadedForTest.bind(this))
> 
> And then sniff for _sidebarViewsLoadedForTest method right here. That means
> showAccessibilityWidget should take a callback.

Gave that a go, but still having serious issues. Will come sit by you on Monday
and we can work through it. Latest attempt (with loads of logspam) uploaded.

> 
>
https://codereview.chromium.org/2058323002/diff/80001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/inspector/elements/accessibility/edit-aria-attributes.html
> (right):
> 
>
https://codereview.chromium.org/2058323002/diff/80001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/inspector/elements/accessibility/edit-aria-attributes.html:49:
> Tests that writing an ARIA attribute causes the accessibility node to be
> updated.
> Expectations file is missing in the patch.

Cause I can't get the test to work correctly! :)

[aboxhall, 2016-07-25 16:53:16]

On 2016/07/22 23:58:52, aboxhall wrote:
> On 2016/07/22 16:44:30, dgozman wrote:
> >
>
https://codereview.chromium.org/2058323002/diff/80001/third_party/WebKit/Layo...
> > File
> >
>
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/2058323002/diff/80001/third_party/WebKit/Layo...
> >
>
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js:7:
> > var sidebarPaneWrappers =
> > WebInspector.panels.elements._elementsSidebarViewWrappers;
> > These panes are loaded asynchronously. We should wait until all the panes
are
> > loaded in ElementsPanel._loadSidebarViews. 
> > 
> > I suggest to do there:
> >
>
Promise.all([<all-promises-go-here>]).then(this._sidebarViewsLoadedForTest.bind(this))
> > 
> > And then sniff for _sidebarViewsLoadedForTest method right here. That means
> > showAccessibilityWidget should take a callback.
> 
> Gave that a go, but still having serious issues. Will come sit by you on
Monday
> and we can work through it. Latest attempt (with loads of logspam) uploaded.

Ok, all working now and test expectation added. Thanks for your help, PTAL :)

[dgozman, 2016-07-25 17:39:38]

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js
(right):

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js:14:
.then(() => {
I think we can just pass result of Promise.all() into .then(...). No need for
extra wrapper function.

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js:25:
_waitForViewsLoaded = function() {
InspectorTest._waitForViewsLoaded

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js:30:
() => { resolve(); });
Can just pass |resolve| as a sniffer.

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js:35:
_getAccessibilitySidebarPane = function()
InspectorTest._getAccessibilitySidebarPane

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js:82:
if ('properties' in accessibilityNode) {
style: double quotes

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js:84:
if ('value' in property)
ditto

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js:100:
InspectorTest.completeTest();
Add error message similar to above.

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js
(right):

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:44:
_gotNodeForTest: function() {
style: { on next line.

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:114:
event.consume(true);
Let's only consume after next if.

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js
(right):

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:71:
this._ariaSubPane.show(this.element);
We don't have to show pane here, as we add it to the stack below.

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:72:
this._ariaSubPane.requestReveal();
requestReveal should be called after adding the to the pane stack.

[aboxhall, 2016-07-25 19:45:12]

Patchset #8 (id:140001) has been deleted

[aboxhall, 2016-07-25 19:49:20]

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js
(right):

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js:14:
.then(() => {
On 2016/07/25 17:39:38, dgozman wrote:
> I think we can just pass result of Promise.all() into .then(...). No need for
> extra wrapper function.

Hm, I think I need to extra wrapper so that Promise.all() is executed after
waitForViewsLoaded() resolves, not when .then() is interpreted.

(I did try passing Promise.all() directly and the test times out, so that's my
assumption as to what's going on.)

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js:25:
_waitForViewsLoaded = function() {
On 2016/07/25 17:39:38, dgozman wrote:
> InspectorTest._waitForViewsLoaded

I figured it doesn't need to be accessible outside this scope?

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js:30:
() => { resolve(); });
On 2016/07/25 17:39:38, dgozman wrote:
> Can just pass |resolve| as a sniffer.

Done.

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js:35:
_getAccessibilitySidebarPane = function()
On 2016/07/25 17:39:38, dgozman wrote:
> InspectorTest._getAccessibilitySidebarPane

Similar to above.

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js:82:
if ('properties' in accessibilityNode) {
On 2016/07/25 17:39:38, dgozman wrote:
> style: double quotes

Oh, weird presubmit didn't catch that. Fixed.

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js:84:
if ('value' in property)
On 2016/07/25 17:39:38, dgozman wrote:
> ditto

Done.

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js:100:
InspectorTest.completeTest();
On 2016/07/25 17:39:38, dgozman wrote:
> Add error message similar to above.

Done.

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js
(right):

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:44:
_gotNodeForTest: function() {
On 2016/07/25 17:39:38, dgozman wrote:
> style: { on next line.

Done.

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/ARIAAttributesView.js:114:
event.consume(true);
On 2016/07/25 17:39:38, dgozman wrote:
> Let's only consume after next if.

Done.

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js
(right):

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:71:
this._ariaSubPane.show(this.element);
On 2016/07/25 17:39:38, dgozman wrote:
> We don't have to show pane here, as we add it to the stack below.

Done.

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/accessibility/AccessibilitySidebarView.js:72:
this._ariaSubPane.requestReveal();
On 2016/07/25 17:39:38, dgozman wrote:
> requestReveal should be called after adding the to the pane stack.

Done.

[dgozman, 2016-07-25 22:41:53]

lgtm

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js
(right):

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js:14:
.then(() => {
On 2016/07/25 19:49:19, aboxhall wrote:
> On 2016/07/25 17:39:38, dgozman wrote:
> > I think we can just pass result of Promise.all() into .then(...). No need
for
> > extra wrapper function.
> 
> Hm, I think I need to extra wrapper so that Promise.all() is executed after
> waitForViewsLoaded() resolves, not when .then() is interpreted.
> 
> (I did try passing Promise.all() directly and the test times out, so that's my
> assumption as to what's going on.)

You are right! I missed that.

https://codereview.chromium.org/2058323002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/accessibility/accessibility-pane-test.js:25:
_waitForViewsLoaded = function() {
On 2016/07/25 19:49:19, aboxhall wrote:
> On 2016/07/25 17:39:38, dgozman wrote:
> > InspectorTest._waitForViewsLoaded
> 
> I figured it doesn't need to be accessible outside this scope?

It doesn't, but that's how we usually name them. At least, change it according
to style guide (name after function, { on next line):

function _waitForViewsLoaded()
{
...
}

[aboxhall, 2016-07-26 15:33:30]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 15:33:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 15:35:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-26 15:35:32]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[aboxhall, 2016-07-26 18:04:01]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 18:04:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 19:20:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-26 19:20:41]

Dry run: This issue passed the CQ dry run.

[aboxhall, 2016-07-26 19:59:56]

The CQ bit was checked by aboxhall@chromium.org

[aboxhall, 2016-07-26 19:59:56]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2058323002/#ps200001
(title: "Rebase and fix test")

[commit-bot: I haz the power, 2016-07-26 20:00:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 20:05:26]

Description was changed from

==========
Add ARIA panel to accessibility sidebar pane.

Incomplete, but usable:
- Can edit attribute values only.
- Can not add extra attributes yet.
- No autocomplete yet.
- No validation yet.
- No moving between attributes via keyboard yet.

Minimal usable version to get started, will add missing features in follow-ups.

BUG=616950
==========

to

==========
Add ARIA panel to accessibility sidebar pane.

Incomplete, but usable:
- Can edit attribute values only.
- Can not add extra attributes yet.
- No autocomplete yet.
- No validation yet.
- No moving between attributes via keyboard yet.

Minimal usable version to get started, will add missing features in follow-ups.

BUG=616950
==========

[commit-bot: I haz the power, 2016-07-26 20:05:28]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-07-26 20:10:22]

Description was changed from

==========
Add ARIA panel to accessibility sidebar pane.

Incomplete, but usable:
- Can edit attribute values only.
- Can not add extra attributes yet.
- No autocomplete yet.
- No validation yet.
- No moving between attributes via keyboard yet.

Minimal usable version to get started, will add missing features in follow-ups.

BUG=616950
==========

to

==========
Add ARIA panel to accessibility sidebar pane.

Incomplete, but usable:
- Can edit attribute values only.
- Can not add extra attributes yet.
- No autocomplete yet.
- No validation yet.
- No moving between attributes via keyboard yet.

Minimal usable version to get started, will add missing features in follow-ups.

BUG=616950

Committed: https://crrev.com/c9d64446c281cd18bb374ac0370bf9fb4cf5cb32
Cr-Commit-Position: refs/heads/master@{#407891}
==========

[commit-bot: I haz the power, 2016-07-26 20:10:24]

Patchset 10 (id:??) landed as
https://crrev.com/c9d64446c281cd18bb374ac0370bf9fb4cf5cb32
Cr-Commit-Position: refs/heads/master@{#407891}