Skip to content

fix(core/dropdown-button): accessibility consistency in dropdown-button-icon #2654

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.

Sign up for GitHub

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

Open
GayatriK2002 wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix(core/dropdown-button): accessibility consistency in dropdown-button-icon #2654

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6bef1f8
added patch for vue wrapper
GayatriK2002 Jul 15, 2026
3735a7c
updated package.json according to review comments
GayatriK2002 Jul 15, 2026
40a0a1a
Merge branch 'main' into fix-4336/dropdown-accessibility
GayatriK2002 Jul 15, 2026
1e06d47
Merge branch 'main' into fix-4336/dropdown-accessibility
GayatriK2002 Jul 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@
"visual-regression": "dotenv -- turbo run visual-regression",
"test": "dotenv -- turbo run test --concurrency=1",
"test.setup": "dotenv -- turbo run test.setup --concurrency=1",
"prepare": "pnpm disable-telemetry",
"prepare": "pnpm disable-telemetry && patch-package --patch-dir patches --patch-content-hash",
"postinstall": "patch-package --patch-dir patches --patch-content-hash",

@gemini-code-assist gemini-code-assist Bot Jul 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using patch-package in a pnpm workspace is highly discouraged and can lead to issues. Since pnpm uses a global content-addressable store and hard links files into node_modules, running patch-package can directly modify files in the global store, potentially corrupting it for other projects on the same machine. Additionally, it may not apply reliably due to pnpm's symlink structure.

Instead, use pnpm's native patching mechanism:

  1. Run pnpm patch @stencil/vue-output-target to extract the package to a temporary directory.
  2. Make the required changes in that directory.
  3. Run pnpm patch-commit <temp-dir> to generate the patch file and automatically configure it under patchedDependencies in your configuration file (e.g., pnpm-workspace.yaml or package.json depending on your pnpm version).
  4. Remove patch-package and postinstall-postinstall from your devDependencies.
Suggested change
"prepare": "pnpm disable-telemetry && patch-package --patch-dir patches --patch-content-hash",
"postinstall": "patch-package --patch-dir patches --patch-content-hash",
"prepare": "pnpm disable-telemetry",

@GayatriK2002 GayatriK2002 Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've migrated away from patch-package to pnpm's native patching mechanism to stay consistent with the existing @stencil/core patch and avoid the content-addressable store risks you mentioned.

"disable-telemetry": "turbo telemetry disable",
"ci:version": "pnpm changeset version && pnpm i --lockfile-only",
"ci:publish": "pnpm changeset publish",
Expand All @@ -50,6 +51,8 @@
"eslint": "catalog:",
"eslint-config-prettier": "^8.10.0",
"execa": "^5.1.1",
"patch-package": "^8.0.1",
"postinstall-postinstall": "^2.1.0",
"prettier": "^2.8.8",
"prettier-plugin-organize-imports": "^3.2.4",
"tsx": "^4.16.2",
Expand Down
1 change: 1 addition & 0 deletions packages/angular-standalone-test-app/src/preview-examples/dropdown-button.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
.dropdown-button {
display: flex;
flex-direction: column;
align-items: flex-start;
justify-content: center;
position: relative;
max-width: 20rem;
Expand Down
1 change: 1 addition & 0 deletions packages/angular-test-app/src/preview-examples/dropdown-button.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
.dropdown-button {
display: flex;
flex-direction: column;
align-items: flex-start;
justify-content: center;
position: relative;
max-width: 20rem;
Expand Down
2 changes: 1 addition & 1 deletion packages/angular/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
"declarationDir": "dist",
"experimentalDecorators": true,
"forceConsistentCasingInFileNames": true,
"lib": ["dom", "es2017"],
"lib": ["dom", "es2020"],
"module": "es2015",
"moduleResolution": "bundler",
"noImplicitAny": true,
Expand Down
1 change: 1 addition & 0 deletions packages/html-test-app/src/preview-examples/dropdown-button.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
.dropdown-button {
display: flex;
flex-direction: column;
align-items: flex-start;
justify-content: center;
position: relative;
max-width: 20rem;
Expand Down
2 changes: 1 addition & 1 deletion packages/nextjs-test-app/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"compilerOptions": {
"target": "ES2017",
"target": "ES2020",
"lib": [
"dom",
"dom.iterable",
Expand Down
1 change: 1 addition & 0 deletions packages/react-test-app/src/preview-examples/dropdown-button.scoped.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
.dropdown-button {
display: flex;
flex-direction: column;
align-items: flex-start;
justify-content: center;
position: relative;
max-width: 20rem;
Expand Down
2 changes: 1 addition & 1 deletion packages/vue-test-app/src/preview-examples/dropdown-button-icon.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

<script setup lang="ts">
import { iconCheckboxes } from '@siemens/ix-icons/icons';
import { IxDropdownButton } from '@siemens/ix-vue';
import { IxDropdownButton, IxDropdownItem} from '@siemens/ix-vue';
</script>

<style scoped src="./dropdown-button-icon.css"></style>
Expand Down
1 change: 1 addition & 0 deletions packages/vue-test-app/src/preview-examples/dropdown-button.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
.dropdown-button {
display: flex;
flex-direction: column;
align-items: flex-start;
justify-content: center;
position: relative;
max-width: 20rem;
Expand Down
2 changes: 1 addition & 1 deletion packages/vue-test-app/src/preview-examples/dropdown-button.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

<script setup lang="ts">
import { iconCheckboxes } from '@siemens/ix-icons/icons';
import { IxDropdownButton } from '@siemens/ix-vue';
import { IxDropdownButton, IxDropdownItem } from '@siemens/ix-vue';
</script>

<style scoped src="./dropdown-button.css"></style>
Expand Down
4 changes: 2 additions & 2 deletions packages/vue/src/vue-component-lib/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@
* and then check if it is not undefined for Vue >= 3.1.0.
* See https://github.com/vuejs/vue-next/issues/3889
*/
const EMPTY_PROP = Symbol();
const EMPTY_PROP = {};

@gemini-code-assist gemini-code-assist Bot Jul 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since EMPTY_PROP is now defined as a plain object {} instead of a unique Symbol(), it is mutable. To prevent accidental mutation of this sentinel value elsewhere in the codebase, it is highly recommended to freeze the object using Object.freeze().

Suggested change
const EMPTY_PROP = {};
const EMPTY_PROP = Object.freeze({});

@GayatriK2002 GayatriK2002 Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[EMPTY_PROP] is only used as a sentinel compared by reference equality [value !== EMPTY_PROP] and is never mutated anywhere, so freezing isn't functionally required. I've left it as a plain {} for now to keep the patch minimal and aligned with the upstream source.

const DEFAULT_EMPTY_PROP = { default: EMPTY_PROP };

interface NavManager<T = any> {

Check warning on line 36 in packages/vue/src/vue-component-lib/utils.ts

View workflow job for this annotation

GitHub Actions / build 

Unexpected any. Specify a different type
navigate: (options: T) => void;
}

Expand Down Expand Up @@ -65,7 +65,7 @@
*/
export const defineContainer = <Props, VModelType = string | number | boolean>(
name: string,
defineCustomElement: any,

Check warning on line 68 in packages/vue/src/vue-component-lib/utils.ts

View workflow job for this annotation

GitHub Actions / build 

Unexpected any. Specify a different type
componentProps: string[] = [],
modelProp?: string,
modelUpdateEvent?: string
Expand Down Expand Up @@ -112,7 +112,7 @@
* when ionChange bubbles up from Component B.
*/
if (e.target.tagName === el.tagName) {
modelPropValue = (e?.target as any)[modelProp];

Check warning on line 115 in packages/vue/src/vue-component-lib/utils.ts

View workflow job for this annotation

GitHub Actions / build 

Unexpected any. Specify a different type
emit(UPDATE_VALUE_EVENT, modelPropValue);
}
});
Expand All @@ -130,7 +130,7 @@
if (routerLink === EMPTY_PROP) return;

if (navManager !== undefined) {
const navigationPayload: any = { event: ev };

Check warning on line 133 in packages/vue/src/vue-component-lib/utils.ts

View workflow job for this annotation

GitHub Actions / build 

Unexpected any. Specify a different type
for (const key in props) {
const value = props[key];
if (
Expand Down Expand Up @@ -168,7 +168,7 @@
}
};

let propsToAdd: any = {

Check warning on line 171 in packages/vue/src/vue-component-lib/utils.ts

View workflow job for this annotation

GitHub Actions / build 

Unexpected any. Specify a different type
ref: containerRef,
class: getElementClasses(containerRef, classes),
onClick: handleClick,
Expand All @@ -185,7 +185,7 @@
if (
// eslint-disable-next-line no-prototype-builtins
(props.hasOwnProperty(key) && value !== EMPTY_PROP) ||
key.startsWith(ARIA_PROP_PREFIX)
(key.startsWith(ARIA_PROP_PREFIX) && value !== EMPTY_PROP)
) {
propsToAdd[key] = value;
}
Comment on lines 185 to 191

@coderabbitai coderabbitai Bot Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Add regression coverage for sentinel-valued and explicit ARIA props.

Test that omitted ARIA props never reach the custom element as a sentinel string, while explicit values such as aria-label="Menu" are still forwarded unchanged. Exercise the actual Vue runtime path as well as this utility if both implementations are shipped.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/vue/src/vue-component-lib/utils.ts` around lines 185 - 191, Add
regression tests around the prop-forwarding logic containing ARIA_PROP_PREFIX
and EMPTY_PROP, covering omitted ARIA props being excluded rather than passed as
the sentinel string and explicit values such as aria-label="Menu" being
forwarded unchanged. Exercise the actual Vue runtime path and the utility
implementation when both are shipped.

Expand Down
2 changes: 1 addition & 1 deletion packages/vue/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"lib": ["dom", "es2020"],
"module": "es2015",
"moduleResolution": "bundler",
"target": "es2017",
"target": "es2020",
"skipLibCheck": true
}
}
28 changes: 28 additions & 0 deletions patches/@stencil_vue-output-target.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
diff --git a/dist/runtime.cjs.js b/dist/runtime.cjs.js
index 0000000..0000000 100644
--- a/dist/runtime.cjs.js
+++ b/dist/runtime.cjs.js
@@ -114,7 +114,7 @@ exports.createSsrApp = createSsrApp;
* you to check if the key exists for Vue <3.1.0
* and then check if it is not undefined for Vue >= 3.1.0.
* See https://github.com/vuejs/vue-next/issues/3889
*/
-const EMPTY_PROP = Symbol();
+const EMPTY_PROP = {};
const DEFAULT_EMPTY_PROP = { default: EMPTY_PROP };
const getComponentClasses = (classes) => {
return classes?.split(' ') || [];
diff --git a/dist/runtime.js b/dist/runtime.js
index 0000000..0000000 100644
--- a/dist/runtime.js
+++ b/dist/runtime.js
@@ -114,7 +114,7 @@ export { createSsrApp };
* you to check if the key exists for Vue <3.1.0
* and then check if it is not undefined for Vue >= 3.1.0.
* See https://github.com/vuejs/vue-next/issues/3889
*/
-const EMPTY_PROP = Symbol();
+const EMPTY_PROP = {};
const DEFAULT_EMPTY_PROP = { default: EMPTY_PROP };
const getComponentClasses = (classes) => {
return classes?.split(' ') || [];
Loading
Loading