-
Notifications
You must be signed in to change notification settings - Fork 125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Using suffix Element/s for IDL attributes referring to elements #1732
Comments
It looks like #920 replaced the string reflection (e.g. From our (Salesforce Lightning Web Components) perspective, it seems sensible to support both the element reflection and the string reflection. The element reflection is necessary to support relationships that cross shadow root boundaries. Whereas the string reflection resolves some inconsistencies in how property reflection works across Both the element and the string reflection can be supported simultaneously, thanks to the fact that the element reflection always has an |
JFTR, there have been previous discussions around the possibility of having both string and element reflection in whatwg/html#3515 (comment) |
Thanks for the context. My proposal seems to be "option # 1" from Alice's comment. |
https://bugs.webkit.org/show_bug.cgi?id=239852 Reviewed by Chris Dumez. LayoutTests/imported/w3c: Small fix on the test and update test results with the new PASS. Add new test case to cover elements moved to a different document. * web-platform-tests/dom/nodes/aria-element-reflection.tentative-expected.txt: * web-platform-tests/dom/nodes/aria-element-reflection.tentative.html: Source/WebCore: Implement ARIA reflection for attributes that refer to a single Element: aria-activedescendant & aria-errormessage. For the properties names this patch uses "Element" suffix: ariaActiveDescendantElement & ariaErrorMessageElement; this matches Chromium implementation and AOM explainer, but not AOM spec: w3c/aria#1732 This implementation is done following the proposed spec text defined at: whatwg/html#3917 * accessibility/AriaAttributes.idl: Add ariaActiveDescendantElement & ariaErrorMessageElement. * bindings/scripts/CodeGenerator.pm: (GetterExpression): Add function for Element attributes. (SetterExpression): Ditto. * bindings/scripts/test/JS/JSTestObj.cpp: Add tests for element attribute reflection. (WebCore::JSTestObjDOMConstructor::construct): (WebCore::jsTestObj_reflectedElementAttrGetter): (WebCore::JSC_DEFINE_CUSTOM_GETTER): (WebCore::setJSTestObj_reflectedElementAttrSetter): (WebCore::JSC_DEFINE_CUSTOM_SETTER): * bindings/scripts/test/TestObj.idl: Add reflectedElementAttr attribute. * dom/Element.cpp: (WebCore::isElementReflectionAttribute): Utility function to identify Element reflection attributes. (WebCore::Element::attributeChanged): If an element reflection attribute has changed we need to synchronize the map entry by removing it as it'll be properly updated in setElementAttribute() when needed. (WebCore::Element::explicitlySetAttrElementsMap): Kind of getter for the ExplicitlySetAttrElementsMap but that creates the map if it doesn't exist yet. (WebCore::Element::explicitlySetAttrElementsMapIfExists const): Getter for the map. (WebCore::Element::getElementAttribute const): Implement getter for element attribute. (WebCore::Element::setElementAttribute): Implement setter for element attribute. * dom/Element.h: Add new method headers and defines the ExplicitlySetAttrElementsMap, which so far only stores one Element in the Vector, but uses a Vector in preparation for supporting FrozenArray<Element> reflection in the future. * dom/ElementRareData.cpp: Update size of SameSizeAsElementRareData to include the new pointer. * dom/ElementRareData.h: Add m_explicitlySetAttrElementsMap attribute. (WebCore::ElementRareData::explicitlySetAttrElementsMap): Getter. (WebCore::ElementRareData::useTypes const):Include ExplicitlySetAttrElementsMap. * dom/Node.cpp: (WebCore::stringForRareDataUseType): Add ExplicitlySetAttrElementsMap. * dom/NodeRareData.h: Ditto. Source/WTF: Add new runtime flag AriaReflectionForElementReferencesEnabled. * Scripts/Preferences/WebPreferencesExperimental.yaml: LayoutTests: Update test to include the new reflected attributes. * accessibility/ARIA-reflection-expected.txt: * accessibility/ARIA-reflection.html: Canonical link: https://commits.webkit.org/250325@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@293860 268f45cc-cd09-0410-ab3c-d52691b4dbfc
https://bugs.webkit.org/show_bug.cgi?id=239853 <rdar://problem/92797836> Reviewed by Chris Dumez. LayoutTests/imported/w3c: Update expectations with the new PASS lines. * web-platform-tests/dom/nodes/aria-element-reflection.tentative-expected.txt: Source/WebCore: Implement ARIA reflection for attributes that refer to a list of Elements: aria-controls, aria-describedby, aria-details, aria-flowto, aria-labelledby and aria-owns. For the properties names this patch uses "Elements" suffix: ariaControlsElements, ariaDescribedByElements, ariaDescribedByElements, ariaFlowToElements, ariaLabelledByElements, ariaOwnsElements this matches Chromium implementation and AOM explainer, but not AOM spec: w3c/aria#1732 * accessibility/AriaAttributes.idl: Add the new properties under AriaReflectionForElementReferencesEnabled runtime flag. * bindings/scripts/CodeGenerator.pm: (GetterExpression): Add function for FrozenArray<Element> properties. (SetterExpression): Ditto. * bindings/scripts/test/JS/JSTestObj.cpp: Add tests. (WebCore::JSTestObjDOMConstructor::construct): (WebCore::jsTestObj_reflectedElementsArrayAttrGetter): (WebCore::JSC_DEFINE_CUSTOM_GETTER): (WebCore::setJSTestObj_reflectedElementsArrayAttrSetter): (WebCore::JSC_DEFINE_CUSTOM_SETTER): * bindings/scripts/test/TestObj.idl: Add example attribute. * dom/Element.cpp: (WebCore::isElementsArrayReflectionAttribute): New utility method to identify the attributes that refer to a list of Elements. (WebCore::Element::attributeChanged): Include check for elements array. (WebCore::Element::getElementsArrayAttribute const): Implement getter. (WebCore::Element::setElementsArrayAttribute): Implement setter. * dom/Element.h: Remove FIXME in ExplicitlySetAttrElementsMap as now it stores more than one element. Add headers for getter and setter. LayoutTests: Update test so it identifies the FrozenArray<Element> attributes. * accessibility/ARIA-reflection-expected.txt: * accessibility/ARIA-reflection.html: Canonical link: https://commits.webkit.org/250511@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@294141 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Just to summarize my preference (since whatwg/html#3515 has a lot of discussion, much of from before element reflection took its current shape), I would prefer for these two to be equivalent in every way: element.setAttribute('aria-labelledby', 'foo')
element.ariaLabelledBy = 'foo' And these two as well: console.log(element.getAttribute('aria-labelledby'))
console.log(element.ariaLabelledBy) In other words, I'm proposing for properties like This gives us consistency across the board at the expense of maybe allowing the properties to provide some better ergonomics than simply setting/getting the attribute. |
Sending back to @jnurthen b/c he already has a PR linked. |
Would it be more clear if it were What are the use cases for having both, rather than just the element reference? Want to make sure the complexity is necessary |
A common use case for this is a custom element framework that massages properties into attributes (or vice versa). For instance, here is a template in Preact: export default function App() {
return (
<input type="text" ariaLabel="foo" ariaDescribedBy="bar" />
);
} This renders (incorrectly) as: <input type="text" aria-label="foo" ariadescribedby="bar"> (Note that the And here is a similar example with Vue: <template>
<input type="text" ariaLabel="foo" ariaDescribedBy="bar">
</template> ...which also renders incorrectly: <input type="text" aria-label="foo" ariadescribedby="bar"> The reason this is happening is that both Preact and Vue try to intelligently convert Of course, for Another alternative is for frameworks like Vue and Preact to contain a hard-coded list of ARIA attributes that have special property formats, and to use For this reason, I prefer Also, note that in no case would it really make sense for the framework (or the component author) to use the // Doesn't work
element.ariaDescribedBy = 'foo'
// Works, but awkward
element.ariaDescribedByElements = [element.getRootNode().getElementById('foo')] |
I agree with @nolanlawson that having ID or IDS suffix can be confusing. I guess the idea is that if you have an HTML attribute like In addition you will have the element reference reflection properties like I guess one concern might be that the HTML attribute and the JavaScript element reference reflection property might get out of sync. For example if you do: div.ariaActiveDescendantElement = foo;
console.log(div.ariaActiveDescendant); // foo
console.log(div.ariaActiveDescendantElement.id); // foo
// Modify element id.
foo.id = "bar";
console.log(div.ariaActiveDescendant); // foo
console.log(div.ariaActiveDescendantElement.id); // bar But that's already happening if you use div.ariaActiveDescendant = "baz"; // This is equivalent to: div.setAttribute("aria-activedescendant", "baz");
console.log(div.ariaActiveDescendant); // baz
console.log(div.ariaActiveDescendantElement.id); // baz If |
ARIA IDL still doesn't include attributes that refer to elements, but AOM IDL doesn't use any suffix for those attributes (e.g.
ariaActiveDescendant
orariaDescribedBy
).However Chromium implementation and the AOM explainer use suffix
Element
orElements
for them (e.g.ariaActiveDescendantElement
orariaDescribedByElements
).What's the expected naming for these attributes? Thanks!
The text was updated successfully, but these errors were encountered: