Skip to content

Commit f5b49c7

Browse files
pskelinpskelin
authored
fix(framework): stop reflecting object/array properties to attributes (#13626)
1 parent d3eb719 commit f5b49c7

4 files changed

Lines changed: 108 additions & 20 deletions

File tree

‎packages/base/cypress/specs/UI5ElementPropsAndAttrs.cy.tsx‎

Lines changed: 63 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ describe("Properties and attributes convert to each other", () => {
7878
.should("not.have.attr", "object-prop");
7979
});
8080

81-
it("Tests that array properties have attributes", () => {
81+
it("Tests that array properties have no attributes when set programmatically", () => {
8282
cy.mount(<Generic></Generic>);
8383

8484
cy.get("[ui5-test-generic]")
@@ -88,7 +88,68 @@ describe("Properties and attributes convert to each other", () => {
8888
.invoke("prop", "multiProp", ["a", "b"]);
8989

9090
cy.get("@testGeneric")
91-
.should("have.attr", "multi-prop", '["a","b"]');
91+
.should("not.have.attr", "multi-prop");
92+
93+
cy.get("@testGeneric")
94+
.invoke("prop", "multiProp")
95+
.should("deep.equal", ["a", "b"]);
96+
});
97+
98+
it("Tests that a declarative array attribute is parsed into the property and not removed by the framework", () => {
99+
// Mount with the attribute pre-set (declarative input)
100+
cy.mount(<Generic multi-prop='["x","y","z"]'></Generic>);
101+
102+
cy.get("[ui5-test-generic]")
103+
.as("testGeneric");
104+
105+
// The attribute author-set in markup must survive first render
106+
cy.get("@testGeneric")
107+
.should("have.attr", "multi-prop", '["x","y","z"]');
108+
109+
// And it must be parsed into the property via fromAttribute
110+
cy.get("@testGeneric")
111+
.invoke("prop", "multiProp")
112+
.should("deep.equal", ["x", "y", "z"]);
113+
});
114+
115+
it("Tests that a later setAttribute for an array property still flows into the property", () => {
116+
cy.mount(<Generic></Generic>);
117+
118+
cy.get("[ui5-test-generic]")
119+
.as("testGeneric");
120+
121+
cy.get("@testGeneric")
122+
.invoke("attr", "multi-prop", '["one","two"]');
123+
124+
cy.get("@testGeneric")
125+
.invoke("prop", "multiProp")
126+
.should("deep.equal", ["one", "two"]);
127+
128+
// Programmatic write to the property must not touch the DOM attribute -
129+
// it stays at the author-set value even though it is now out of sync with the property.
130+
cy.get("@testGeneric")
131+
.invoke("prop", "multiProp", ["three"]);
132+
133+
cy.get("@testGeneric")
134+
.should("have.attr", "multi-prop", '["one","two"]');
135+
136+
cy.get("@testGeneric")
137+
.invoke("prop", "multiProp")
138+
.should("deep.equal", ["three"]);
139+
});
140+
141+
it("Tests that a declarative object attribute is parsed into the property and not removed by the framework", () => {
142+
cy.mount(<Generic object-prop='{"a":1,"b":"two"}'></Generic>);
143+
144+
cy.get("[ui5-test-generic]")
145+
.as("testGeneric");
146+
147+
cy.get("@testGeneric")
148+
.should("have.attr", "object-prop", '{"a":1,"b":"two"}');
149+
150+
cy.get("@testGeneric")
151+
.invoke("prop", "objectProp")
152+
.should("deep.equal", { a: 1, b: "two" });
92153
});
93154

94155
it("Tests that noAttribute properties have no attributes", () => {

‎packages/base/src/UI5Element.ts‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -98,12 +98,14 @@ const defaultConverter = {
9898
return value as boolean ? "" : null;
9999
}
100100

101-
// don't set attributes for arrays and objects
101+
// Don't reflect arrays and objects to the DOM. Attributes exist for CSS selectors
102+
// (which don't apply to objects/arrays) and for debugging via the Elements panel
103+
// (devs will use the console with property access for these). Declarative
104+
// attribute -> property is still supported via fromAttribute (JSON.parse).
102105
if (type === Object || type === Array) {
103-
return JSON.stringify(value);
106+
return null;
104107
}
105108

106-
// object, array, other
107109
if (value === null || value === undefined) {
108110
return null;
109111
}
@@ -728,6 +730,14 @@ abstract class UI5Element extends HTMLElement {
728730

729731
const properties = ctor.getMetadata().getProperties();
730732
const propData = properties[name];
733+
734+
// Object and Array properties are not reflected to attributes. The attribute is only
735+
// consumed as a declarative input (parsed via fromAttribute on attributeChangedCallback),
736+
// so the framework must neither write nor remove it - leave any author-set attribute alone.
737+
if (propData.type === Object || propData.type === Array) {
738+
return;
739+
}
740+
731741
const attrName = camelToKebabCase(name);
732742
const converter = propData.converter || defaultConverter;
733743

‎packages/base/src/UI5ElementMetadata.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ class UI5ElementMetadata {
133133
*/
134134
hasAttribute(propName: string): boolean {
135135
const propData = this.getProperties()[propName];
136-
return propData.type !== Object && !propData.noAttribute;
136+
return !propData.noAttribute;
137137
}
138138

139139
/**

‎packages/main/cypress/specs/MultiComboBox.cy.tsx‎

Lines changed: 31 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -888,7 +888,8 @@ describe("General", () => {
888888
);
889889

890890
cy.get("ui5-multi-combobox")
891-
.should("have.attr", "selected-values",'["al","en"]');
891+
.invoke("prop", "selectedValues")
892+
.should("deep.equal", ["al", "en"]);
892893

893894
cy.get("[ui5-mcb-item]")
894895
.eq(0)
@@ -944,7 +945,8 @@ describe("General", () => {
944945
.should("have.length", "1");
945946

946947
cy.get("[ui5-multi-combobox]")
947-
.should("have.attr", "selected-values", '["dk"]');
948+
.invoke("prop", "selectedValues")
949+
.should("deep.equal", ["dk"]);
948950
});
949951

950952
it("updates selectedValues when selecting items via checkbox", () => {
@@ -958,8 +960,11 @@ describe("General", () => {
958960
);
959961

960962
cy.get("[ui5-multi-combobox]")
961-
.as("mcb")
962-
.should("have.attr", "selected-values", '[]');
963+
.as("mcb");
964+
965+
cy.get("@mcb")
966+
.invoke("prop", "selectedValues")
967+
.should("deep.equal", []);
963968

964969
// Open the dropdown
965970
cy.get("@mcb")
@@ -975,7 +980,8 @@ describe("General", () => {
975980
.realClick();
976981

977982
cy.get("@mcb")
978-
.should("have.attr", "selected-values", '["DE"]');
983+
.invoke("prop", "selectedValues")
984+
.should("deep.equal", ["DE"]);
979985

980986
// Select second item via checkbox
981987
cy.get("[ui5-mcb-item]")
@@ -985,7 +991,8 @@ describe("General", () => {
985991
.realClick();
986992

987993
cy.get("@mcb")
988-
.should("have.attr", "selected-values", '["DE","FR"]');
994+
.invoke("prop", "selectedValues")
995+
.should("deep.equal", ["DE", "FR"]);
989996

990997
// Select third and fourth items
991998
cy.get("[ui5-mcb-item]")
@@ -1001,7 +1008,8 @@ describe("General", () => {
10011008
.realClick();
10021009

10031010
cy.get("@mcb")
1004-
.should("have.attr", "selected-values", '["DE","FR","IT","US"]');
1011+
.invoke("prop", "selectedValues")
1012+
.should("deep.equal", ["DE", "FR", "IT", "US"]);
10051013
});
10061014

10071015
it("selects correct items when selectedValues is set before items are added", () => {
@@ -1011,8 +1019,11 @@ describe("General", () => {
10111019
);
10121020

10131021
cy.get("[ui5-multi-combobox]")
1014-
.as("mcb")
1015-
.should("have.attr", "selected-values", '["FR","US"]');
1022+
.as("mcb");
1023+
1024+
cy.get("@mcb")
1025+
.invoke("prop", "selectedValues")
1026+
.should("deep.equal", ["FR", "US"]);
10161027

10171028
// No tokens yet since no items
10181029
cy.get("@mcb")
@@ -1077,8 +1088,11 @@ describe("General", () => {
10771088
);
10781089

10791090
cy.get("[ui5-multi-combobox]")
1080-
.as("mcb")
1081-
.should("have.attr", "selected-values", "[]");
1091+
.as("mcb");
1092+
1093+
cy.get("@mcb")
1094+
.invoke("prop", "selectedValues")
1095+
.should("deep.equal", []);
10821096

10831097
// Type "Ca" to trigger typeahead for Canada
10841098
cy.get("@mcb")
@@ -1092,7 +1106,8 @@ describe("General", () => {
10921106

10931107
// Verify selectedValues is updated
10941108
cy.get("@mcb")
1095-
.should("have.attr", "selected-values", '["CA"]');
1109+
.invoke("prop", "selectedValues")
1110+
.should("deep.equal", ["CA"]);
10961111

10971112
// Verify token is created
10981113
cy.get("@mcb")
@@ -1111,7 +1126,8 @@ describe("General", () => {
11111126

11121127
// Verify selectedValues now has both values
11131128
cy.get("@mcb")
1114-
.should("have.attr", "selected-values", '["CA","JP"]');
1129+
.invoke("prop", "selectedValues")
1130+
.should("deep.equal", ["CA", "JP"]);
11151131
});
11161132
});
11171133

@@ -2768,7 +2784,8 @@ describe("Event firing", () => {
27682784
return event.detail.item === undefined;
27692785
}));
27702786
cy.get("[ui5-multi-combobox]")
2771-
.should("have.attr", "selected-values", '[]');
2787+
.invoke("prop", "selectedValues")
2788+
.should("deep.equal", []);
27722789
});
27732790
});
27742791

0 commit comments

Comments
 (0)