Skip to content

Commit

Permalink
AX Web Inspector Panel: Switch form controls report "Checked: true/fa…
Browse files Browse the repository at this point in the history 
…lse" rather than something like "State: on/off"

https://bugs.webkit.org/show_bug.cgi?id=274846
rdar://128952449

Reviewed by Devin Rousso.

Switches are like checkboxes without a mixed state, and are exposed in the web platform in this way — HTML switches
are created via `input type="checkbox"` plus the `switch` attribute, and are "enabled" via the `checked` attribute.
And ARIA switches (`role="switch"`) use `aria-checked`.

A side effect of this is that when we added support for switch controls, we forgot to update Web Inspector to handle
the nuanced difference between these types of controls, and thus switch controls report a "Checked: true/false" state
rather than "State: on/off", which is more appropriate terminology. This patch addresses this issue.

* LayoutTests/inspector/dom/getAccessibilityPropertiesForNode-expected.txt:
* LayoutTests/inspector/dom/getAccessibilityPropertiesForNode.html:
* Source/JavaScriptCore/inspector/protocol/DOM.json:
* Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:
(WebCore::InspectorDOMAgent::buildObjectForAccessibilityProperties):
* Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:
* Source/WebInspectorUI/UserInterface/Models/DOMNode.js:
(WI.DOMNode.prototype.accessibilityProperties.accessibilityPropertiesCallback):
(WI.DOMNode.prototype.accessibilityProperties):
* Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js:
(WI.DOMNodeDetailsSidebarPanel.prototype.initialLayout):
(WI.DOMNodeDetailsSidebarPanel.prototype._refreshAccessibility.accessibilityPropertiesCallback):
(WI.DOMNodeDetailsSidebarPanel.prototype._refreshAccessibility):

Canonical link: https://commits.webkit.org/279772@main
  • Loading branch information
@twilco
twilco committed Jun 6, 2024
1 parent 826e8c5 commit 132e20a
Show file tree
Hide file tree
Showing 7 changed files with 84 additions and 16 deletions.
36 changes: 31 additions & 5 deletions LayoutTests/inspector/dom/getAccessibilityPropertiesForNode-expected.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ Checking Web Inspector protocol for the Accessibility Node Inspector.
exists: true
label:
role:
childNodeIds.length: 93
childNodeIds.length: 96


Total elements to be tested: 123.
Total elements to be tested: 126.

<div onclick="void(0);">click</div>
exists: true
Expand Down Expand Up @@ -777,11 +777,37 @@ Total elements to be tested: 123.
readonly: true
required: false

<div role="switch" aria-required="true">Required switch.</div>
<input type="checkbox" switch="" checked="">
exists: true
label: Required switch.
label:
role: switch
checked: false
switchState: on
focused: false
parentNodeId: exists
required: false

<input type="checkbox" switch="">
exists: true
label:
role: switch
switchState: off
focused: false
parentNodeId: exists
required: false

<div role="switch" aria-checked="true">On-state ARIA switch.</div>
exists: true
label: On-state ARIA switch.
role: switch
switchState: on
parentNodeId: exists
required: false

<div role="switch" aria-required="true">Required, off-state ARIA switch.</div>
exists: true
label: Required, off-state ARIA switch.
role: switch
switchState: off
parentNodeId: exists
required: true

Expand Down
5 changes: 4 additions & 1 deletion LayoutTests/inspector/dom/getAccessibilityPropertiesForNode.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,10 @@
<input class="ex" aria-invalid="foo" value="fake value will eval to true">
<input class="ex" readonly value="readonly">

<div class="ex" role="switch" aria-required="true">Required switch.</div>
<div class="ex" role="switch" aria-required="true">Required, off-state ARIA switch.</div>
<div class="ex" role="switch" aria-checked="true">On-state ARIA switch.</div>
<input class="ex" type="checkbox" switch>
<input class="ex" type="checkbox" switch checked>

<div class="ex" role="textbox" tabindex="0" aria-readonly="true">readonly</div>
<input class="ex" disabled value="disabled">
Expand Down
3 changes: 2 additions & 1 deletion Source/JavaScriptCore/inspector/protocol/DOM.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,8 @@
{ "name": "required", "type": "boolean", "optional": true, "description": "Required state of form controls." },
{ "name": "role", "type": "string", "description": "Computed value for first recognized role token, default role per element, or overridden role." },
{ "name": "selected", "type": "boolean", "optional": true, "description": "Selected state of certain form controls." },
{ "name": "selectedChildNodeIds", "type": "array", "items": { "$ref": "NodeId" }, "optional": true, "description": "Array of <code>DOMNode</code> ids of any children marked as selected." }
{ "name": "selectedChildNodeIds", "type": "array", "items": { "$ref": "NodeId" }, "optional": true, "description": "Array of <code>DOMNode</code> ids of any children marked as selected." },
{ "name": "switchState", "type": "string", "optional": true, "enum": ["off", "on"], "description": "On / off state of switch form controls." }
]
},
{
Expand Down
31 changes: 24 additions & 7 deletions Source/WebCore/inspector/agents/InspectorDOMAgent.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2186,6 +2186,7 @@ Ref<Inspector::Protocol::DOM::AccessibilityProperties> InspectorDOMAgent::buildO
Node* activeDescendantNode = nullptr;
bool busy = false;
auto checked = Inspector::Protocol::DOM::AccessibilityProperties::Checked::False;
auto switchState = Inspector::Protocol::DOM::AccessibilityProperties::SwitchState::Off;
RefPtr<JSON::ArrayOf<Inspector::Protocol::DOM::NodeId>> childNodeIds;
RefPtr<JSON::ArrayOf<Inspector::Protocol::DOM::NodeId>> controlledNodeIds;
auto currentState = Inspector::Protocol::DOM::AccessibilityProperties::Current::False;
Expand All @@ -2211,6 +2212,7 @@ Ref<Inspector::Protocol::DOM::AccessibilityProperties> InspectorDOMAgent::buildO
String role;
bool selected = false;
RefPtr<JSON::ArrayOf<Inspector::Protocol::DOM::NodeId>> selectedChildNodeIds;
bool isSwitch = false;
bool supportsChecked = false;
bool supportsExpanded = false;
bool supportsLiveRegion = false;
Expand All @@ -2235,17 +2237,26 @@ Ref<Inspector::Protocol::DOM::AccessibilityProperties> InspectorDOMAgent::buildO
current = current->parentObject();
}

isSwitch = axObject->isSwitch();
supportsChecked = axObject->supportsChecked();
if (supportsChecked) {
AccessibilityButtonState checkValue = axObject->checkboxOrRadioValue(); // Element using aria-checked.
if (checkValue == AccessibilityButtonState::On)
checked = Inspector::Protocol::DOM::AccessibilityProperties::Checked::True;
else if (checkValue == AccessibilityButtonState::Mixed)
if (checkValue == AccessibilityButtonState::On) {
if (isSwitch)
switchState = Inspector::Protocol::DOM::AccessibilityProperties::SwitchState::On;
else
checked = Inspector::Protocol::DOM::AccessibilityProperties::Checked::True;
} else if (checkValue == AccessibilityButtonState::Mixed && !isSwitch)
checked = Inspector::Protocol::DOM::AccessibilityProperties::Checked::Mixed;
else if (axObject->isChecked()) // Native checkbox.
checked = Inspector::Protocol::DOM::AccessibilityProperties::Checked::True;
else if (axObject->isChecked()) {
// Native checkbox or switch.
if (isSwitch)
switchState = Inspector::Protocol::DOM::AccessibilityProperties::SwitchState::On;
else
checked = Inspector::Protocol::DOM::AccessibilityProperties::Checked::True;
}
}

if (!axObject->children().isEmpty()) {
childNodeIds = JSON::ArrayOf<Inspector::Protocol::DOM::NodeId>::create();
processAccessibilityChildren(*axObject, *childNodeIds);
Expand Down Expand Up @@ -2423,8 +2434,14 @@ Ref<Inspector::Protocol::DOM::AccessibilityProperties> InspectorDOMAgent::buildO
}
if (busy)
value->setBusy(busy);
if (supportsChecked)

// Switches `supportsChecked` (the underlying implementation is mostly shared with checkboxes),
// but should report a switch state and not a checked state.
if (isSwitch)
value->setSwitchState(switchState);
else if (supportsChecked)
value->setChecked(checked);

if (childNodeIds)
value->setChildNodeIds(childNodeIds.releaseNonNull());
if (controlledNodeIds)
Expand Down
4 changes: 4 additions & 0 deletions Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1190,6 +1190,8 @@ localizedStrings["Observer Callback"] = "Observer Callback";
localizedStrings["Observer Handlers:"] = "Observer Handlers:";
localizedStrings["Observers:"] = "Observers:";
localizedStrings["Off"] = "Off";
/* Label indicating that an input of type switch is off. */
localizedStrings["Off @ Switch State"] = "Off";
/* Label for a preference that is turned off. */
localizedStrings["Off @ User Preferences Overrides"] = "Off";
/* Input label for the x-axis of the offset of a CSS box shadow */
Expand All @@ -1198,6 +1200,8 @@ localizedStrings["Offset X @ Box Shadow Editor"] = "Offset X";
localizedStrings["Offset Y @ Box Shadow Editor"] = "Offset Y";
/* Property value for `font-variant-numeric: oldstyle-nums`. */
localizedStrings["Old-Style Numerals @ Font Details Sidebar Property Value"] = "Old-Style Numerals";
/* Label indicating that an input of type switch is on. */
localizedStrings["On @ Switch State"] = "On";
/* Label for a preference that is turned on. */
localizedStrings["On @ User Preferences Overrides"] = "On";
localizedStrings["Once"] = "Once";
Expand Down
3 changes: 2 additions & 1 deletion Source/WebInspectorUI/UserInterface/Models/DOMNode.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -935,7 +935,8 @@ WI.DOMNode = class DOMNode extends WI.Object
required: accessibilityProperties.required,
role: accessibilityProperties.role,
selected: accessibilityProperties.selected,
selectedChildNodeIds: accessibilityProperties.selectedChildNodeIds
selectedChildNodeIds: accessibilityProperties.selectedChildNodeIds,
switchState: accessibilityProperties.switchState,
});
}
}
Expand Down
18 changes: 17 additions & 1 deletion Source/WebInspectorUI/UserInterface/Views/DOMNodeDetailsSidebarPanel.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ WI.DOMNodeDetailsSidebarPanel = class DOMNodeDetailsSidebarPanel extends WI.DOMD
this._accessibilityNodeActiveDescendantRow = new WI.DetailsSectionSimpleRow(WI.UIString("Shared Focus"));
this._accessibilityNodeBusyRow = new WI.DetailsSectionSimpleRow(WI.UIString("Busy"));
this._accessibilityNodeCheckedRow = new WI.DetailsSectionSimpleRow(WI.UIString("Checked"));
this._accessibilityNodeSwitchStateRow = new WI.DetailsSectionSimpleRow(WI.UIString("State"));
this._accessibilityNodeChildrenRow = new WI.DetailsSectionSimpleRow(WI.UIString("Children"));
this._accessibilityNodeControlsRow = new WI.DetailsSectionSimpleRow(WI.UIString("Controls"));
this._accessibilityNodeCurrentRow = new WI.DetailsSectionSimpleRow(WI.UIString("Current"));
Expand Down Expand Up @@ -546,6 +547,19 @@ WI.DOMNodeDetailsSidebarPanel = class DOMNodeDetailsSidebarPanel extends WI.DOMD
checked = WI.UIString("No");
}

let switchState = "";
// COMPATIBILITY (macOS X.0, iOS X.0): DOM.AccessibilityProperties.switchState did not exist yet.
if (InspectorBackend.Enum.DOM.AccessibilityPropertiesSwitchState) {
switch (accessibilityProperties.switchState) {
case InspectorBackend.Enum.DOM.AccessibilityPropertiesSwitchState.On:
switchState = WI.UIString("On", "On @ Switch State", "Label indicating that an input of type switch is on.");
break;
case InspectorBackend.Enum.DOM.AccessibilityPropertiesSwitchState.Off:
switchState = WI.UIString("Off", "Off @ Switch State", "Label indicating that an input of type switch is off.");
break;
}
}

// Accessibility tree children are not a 1:1 mapping with DOM tree children.
var childNodeLinkList = linkListForNodeIds(accessibilityProperties.childNodeIds);
var controlledNodeLinkList = linkListForNodeIds(accessibilityProperties.controlledNodeIds);
Expand Down Expand Up @@ -728,6 +742,7 @@ WI.DOMNodeDetailsSidebarPanel = class DOMNodeDetailsSidebarPanel extends WI.DOMD
this._accessibilityNodeInvalidRow.value = invalid;
this._accessibilityNodeLabelRow.value = label;
this._accessibilityNodeLiveRegionStatusRow.value = liveRegionStatusNode || liveRegionStatus;
this._accessibilityNodeSwitchStateRow.value = switchState;

// Row label changes based on whether the value is a delegate node link.
this._accessibilityNodeMouseEventRow.label = mouseEventNodeLink ? WI.UIString("Click Listener") : WI.UIString("Clickable");
Expand Down Expand Up @@ -777,7 +792,8 @@ WI.DOMNodeDetailsSidebarPanel = class DOMNodeDetailsSidebarPanel extends WI.DOMD
this._accessibilityNodeExpandedRow,
this._accessibilityNodePressedRow,
this._accessibilityNodeReadonlyRow,
this._accessibilityNodeSelectedRow
this._accessibilityNodeSelectedRow,
this._accessibilityNodeSwitchStateRow,
];

this._accessibilityEmptyRow.hideEmptyMessage();
Expand Down

0 comments on commit 132e20a

Please sign in to comment.