Bug 128504 – Web Inspector: AXI: more properties: exists, required, and invalid (exists was previously combined with ignored)

• Component: Web Inspector
• Status: RESOLVED FIXED

[James Craig]

bug 127447 combined "isAccessibilityObject" and "accessibilityIsIgnored", but in hindsight, it'd be good to do these separately. For example, ""<img src="foo" alt=""> still has an accessibility object (and therefore a role) even though the element is ignored in the platform API. Likewise, elements with the presentation role are ignored for the API, but have an object and would be able to indicate more contextual information than just the "No accessibility information" that is conveyed now.

[Radar WebKit Bug Importer]

<rdar://problem/16021878>

[James Craig]

accessibilityIsIgnored is now just "ignored" in the inspector-protocol for getAccessibilityPropertiesForNode

[James Craig]

Calling this "exists" ( was previously combined with ignored), and also adding required and invalid on relevant fields.

[James Craig]

Created attachment 224169 [details]
patch

[James Craig]

Created attachment 224172 [details]
patch

[James Craig]

Created attachment 224230 [details]
patch

fixing apply conflict with TOT

[James Craig]

Not sure why this patch is not applying.

[Timothy Hatcher]

Seems like your DOMNodeDetailsSidebarPanel.js is out of date.

[Timothy Hatcher]

Or this patch depends on another patch that is also up for review and not in TOT yet.

[James Craig]

Created attachment 224329 [details]
patch

[James Craig]

Created attachment 224330 [details]
patch

[James Craig]

I'm up-to-date on all files (the diffs appear clean to me), and this patch doesn't depend on anything that's missing from TOT.

[James Craig]

This is frustrating. I’m going to check out a clean copy and reapply the diffs manually.

[James Craig]

Created attachment 224331 [details]
patch minus the file in question

Not for submission. Just testing to see if this applies.

[James Craig]

Created attachment 224332 [details]
patch

[James Craig]

Maybe a bug in ./Tools/Scripts/svn-apply ??

[James Craig]

Created attachment 224335 [details]
patch

[James Craig]

That's better. The diffs were identical, but it seems svn-apply wasn't able to handle a regular diff without the extra symbols added by svn-create-patch.

[Timothy Hatcher]

Comment on attachment 224335 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=224335&action=review

> Source/WebCore/inspector/protocol/DOM.json:68
> +                { "name": "required", "type": "boolean", "description": "Returns whether the element is required or not required." },
> +                { "name": "role", "type": "string", "description": "Computed value for first recognized role token, default role per element, or overridden role." },
> +                { "name": "supportsRequired", "type": "boolean", "description": "Returns whether the required state is relevant for this element (form fields, etc)." },

"required" can be marked optional. And if it is supported, it can be included. The front-end can check for the existence of required using: "required" in axobj or axobj.required !== undefined.

> Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:245
> +        function accessibilityPropertiesCallback(props)

accessibilityProperties is better. We don't abbreviate in WebKit.

> Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:253
> +                var ignored = (props.ignored ? "true" : "");
> +                var invalid = (props.invalid && props.invalid !== "false" ? props.invalid : "");

WebKit style drops the parens in these cases.

> Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:260
> +                var required = (props.supportsRequired ? (props.required ? "true" : "false") : "");

No need for the outer parens.

> Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:276
> +                this._accessibilityNodeIgnoredRow.value = ignored;
> +                this._accessibilityNodeInvalidRow.value = invalid;
> +                this._accessibilityNodeLabelRow.value = label;
> +                this._accessibilityNodeRequiredRow.value = required;
> +                this._accessibilityNodeRoleRow.value = role;

ignored and required look like they are "true" and "false" strings. Those user visible strings should be WebInspector.UIStrings. Something like "Yes" and "No" would be nicer and can be localized.

[chris fleizach]

Comment on attachment 224335 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=224335&action=review

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1434
> +                required =  axObject->isRequired();

wrong spacing after "="

[James Craig]

(In reply to comment #19)
> > Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:245
> > +        function accessibilityPropertiesCallback(props)
> 
> accessibilityProperties is better. We don't abbreviate in WebKit.

I was trying to reduce redundancy as suggested in the previous bug. If "accessibilityProperties" is the only argument of the "accessibilityPropertiesCallback" callback, why should it be so verbose?

[James Craig]

Created attachment 224450 [details]
patch with review feedback

[James Craig]

Comment on attachment 224450 [details]
patch with review feedback

Actually, I'm going to wrap the block of optional attrs in an "exists" conditional. Marking previous as obsolete.

[James Craig]

Created attachment 224453 [details]
patch with review feedback

[Timothy Hatcher]

Comment on attachment 224453 [details]
patch with review feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=224453&action=review

> Source/WebCore/inspector/InspectorDOMAgent.cpp:1443
>          .setNodeId(pushNodePathToFrontend(node));
> -
> +    if (exists) {

An empty line here helps readability.

> Source/WebInspectorUI/UserInterface/DOMNodeDetailsSidebarPanel.js:291
> +                ];
>                  this._accessibilityEmptyRow.hideEmptyMessage();

An empty line here would be nice.

[James Craig]

Created attachment 224562 [details]
patch

[WebKit Commit Bot]

Comment on attachment 224562 [details]
patch

Clearing flags on attachment: 224562

Committed r164332: <http://trac.webkit.org/changeset/164332>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.