[webkit-reviews] review denied: [Bug 128504] Web Inspector: AX: more properties: exists, required, and invalid (exists was previously combined with ignored) : [Attachment 224335] patch

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has denied James Craig <jcraig at apple.com>'s
request for review:
Bug 128504: Web Inspector: AX: more properties: exists, required, and invalid
(exists was previously combined with ignored)
https://bugs.webkit.org/show_bug.cgi?id=128504

Attachment 224335: patch
https://bugs.webkit.org/attachment.cgi?id=224335&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
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.