fix(compat): fix compat handler of draggable #12445

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

Jump to bottom

Open

yangxiuxiu1115 wants to merge 8 commits into vuejs:main

from yangxiuxiu1115:fix/compat-attr

Open

fix(compat): fix compat handler of draggable #12445

yangxiuxiu1115 wants to merge 8 commits into vuejs:main from yangxiuxiu1115:fix/compat-attr

+26 −5

Conversation

@yangxiuxiu1115

Copy link

Contributor

@yangxiuxiu1115 yangxiuxiu1115 commented Nov 20, 2024 •

edited by coderabbitai bot

Loading

fix #12444
image

Summary by CodeRabbit

Bug Fixes
- Improved handling of enumerated attributes so null, false, and the string "false" are coerced and rendered consistently.
Tests
- Added a test verifying attribute preservation and deprecation warnings when enumerated attribute coercion occurs under compatibility mode.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

@github-actions

Copy link

github-actions bot commented Nov 20, 2024 •

edited

Loading

Size Report

Bundles

File	Size	Gzip	Brotli
runtime-dom.global.prod.js	103 kB	38.9 kB	35.1 kB
vue.global.prod.js	161 kB	58.8 kB	52.4 kB

Usages

Name	Size	Gzip	Brotli
createApp (CAPI only)	46.9 kB	18.3 kB	16.8 kB
createApp	55 kB	21.4 kB	19.6 kB
createSSRApp	59.3 kB	23.1 kB	21.1 kB
defineCustomElement	60.6 kB	23.1 kB	21.1 kB
overall	69.3 kB	26.6 kB	24.3 kB

@pkg-pr-new

Copy link

pkg-pr-new bot commented Nov 20, 2024 •

edited

Loading

Open in StackBlitz

@vue/compiler-core

npm i https://pkg.pr.new/@vue/compiler-core@12445

@vue/compiler-dom

npm i https://pkg.pr.new/@vue/compiler-dom@12445

@vue/compiler-sfc

npm i https://pkg.pr.new/@vue/compiler-sfc@12445

@vue/compiler-ssr

npm i https://pkg.pr.new/@vue/compiler-ssr@12445

@vue/reactivity

npm i https://pkg.pr.new/@vue/reactivity@12445

@vue/runtime-core

npm i https://pkg.pr.new/@vue/runtime-core@12445

@vue/runtime-dom

npm i https://pkg.pr.new/@vue/runtime-dom@12445

@vue/server-renderer

npm i https://pkg.pr.new/@vue/server-renderer@12445

@vue/shared

npm i https://pkg.pr.new/@vue/shared@12445

vue

npm i https://pkg.pr.new/vue@12445

@vue/compat

npm i https://pkg.pr.new/@vue/compat@12445

commit: 102f560

@yangxiuxiu1115 yangxiuxiu1115 marked this pull request as draft

November 20, 2024 10:27

@yangxiuxiu1115 yangxiuxiu1115 marked this pull request as ready for review

November 20, 2024 11:42

@edison1105 edison1105 added 🔨 p3-minor-bug scope: v2 compat labels

Nov 21, 2024

@edison1105

Copy link

Member

edison1105 commented Nov 21, 2024

/ecosystem-ci run

@vue-bot

Copy link

Contributor

vue-bot commented Nov 21, 2024

📝 Ran ecosystem CI: Open

suite	result	latest scheduled
language-tools	✅ success	✅ success
nuxt	✅ success	✅ success
pinia	✅ success	✅ success
primevue	✅ success	✅ success
quasar	✅ success	✅ success
radix-vue	✅ success	✅ success
router	✅ success	✅ success
test-utils	✅ success	✅ success
vant	✅ success	✅ success
vite-plugin-vue	✅ success	✅ success
vitepress	✅ success	✅ success
vue-i18n	✅ success	✅ success
vue-macros	✅ success	✅ success
vuetify	✅ success	✅ success
vueuse	✅ success	✅ success
vue-simple-compiler	✅ success	✅ success

@edison1105 edison1105 added the ready to merge label

Nov 21, 2024

skirtles-code

skirtles-code reviewed

May 20, 2025

View reviewed changes

packages/vue-compat/__tests__/global.spec.ts Outdated

expect(app2.config.globalProperties.test).toBe(undefined)

})

test('ATTR_ENUMERATED_COERCION: true', () => {

Copy link

Contributor

@skirtles-code skirtles-code May 20, 2025

Choose a reason for hiding this comment

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

I'm wondering whether the tests for this should be in misc.spec.ts instead, as there are already tests for ATTR_ENUMERATED_COERCION in that file?

packages/runtime-dom/src/modules/attrs.ts Outdated

Comment on lines 62 to 64

value === null||value===false||value==='false'

? 'false'

: typeofvalue!=='boolean'&&value !== undefined

: value !== undefined

Copy link

Contributor

@skirtles-code skirtles-code May 20, 2025

Choose a reason for hiding this comment

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

I think the logic is correct, but it might be easier to understand if the undefined check were handled first?

yangchangtao added 4 commits

May 27, 2025 17:36


 fix(compat): fix compat handler of draggable

5b59155


 chore: update

d996c6a


 chore: testcase

06fd879


 chore: update

2a58947

@yangxiuxiu1115 yangxiuxiu1115 force-pushed the fix/compat-attr branch from f006bad to 2a58947 Compare

May 27, 2025 09:36

@coderabbitai

Copy link

coderabbitai bot commented May 27, 2025 •

edited

Loading

Walkthrough

Updated compat enumerated-attribute coercion so string "false", false, and null coerce to "false", undefined returns null, otherwise "true". Added a test verifying draggable="false" renders as "false" and emits a deprecation warning.

Changes

Cohort / File(s)	Change Summary
Runtime DOM attr logic `packages/runtime-dom/src/modules/attrs.ts`	Revised `compatCoerceAttr` internals: return `null` for `undefined`; treat `null`, `false`, and the string `'false'` as `'false'`; otherwise return `'true'`. No exported signatures changed.
Compat tests `packages/vue-compat/__tests__/misc.spec.ts`	Added `ATTR_ENUMERATED_COERCION` test mounting `draggable="false"` and `:spellcheck="false"`, asserting rendered attributes and deprecation warnings.

Sequence Diagram(s)

sequenceDiagram
 participant TestRunner
 participant VueCompat
 participant compatCoerceAttr
 participant DOM
 TestRunner->>VueCompat: mount template with ATTR_ENUMERATED_COERCION enabled
 VueCompat->>compatCoerceAttr: coerce attribute "draggable" with value "false"
 compatCoerceAttr-->>VueCompat: returns "false"
 VueCompat->>DOM: render element with draggable="false"
 VueCompat-->>TestRunner: emit deprecation warning(s)
 TestRunner->>TestRunner: assert DOM and warnings

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pay attention to compatCoerceAttr conditional logic and edge cases (string vs boolean vs null/undefined).
Verify tests cover both literal attributes and bound props (e.g., "false" vs :spellcheck="false").

Possibly related PRs

fix(compat): ensure false value on input value retains attribute #13216 — Modifies the same compatCoerceAttr implementation in packages/runtime-dom/src/modules/attrs.ts, addressing how enumerated attributes are coerced.

Suggested reviewers

KazariEX

Poem

A rabbit hopped in code at night,
Turned wrong-to-right with gentle bite.
"false" stays false, no sneaky trick—
Tests applaud with pace so quick.
🥕

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: fixing the compat handler for the draggable attribute.
Linked Issues check	✅ Passed	The code changes directly address issue #12444 by fixing the enumerated attribute coercion logic so draggable="false" is preserved instead of coerced to true.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing the enumerated attribute coercion logic and adding a test case; no out-of-scope modifications detected.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5e0e684 and 102f560.

📒 Files selected for processing (1)

packages/vue-compat/__tests__/misc.spec.ts (1 hunks)

✅ Files skipped from review due to trivial changes (1)

packages/vue-compat/tests/misc.spec.ts

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: continuous-release
GitHub Check: autofix

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

yangchangtao added 2 commits

May 27, 2025 17:41


 chore: update

c67368a


 chore: update

5e0e684

coderabbitai[bot]

coderabbitai bot reviewed

May 27, 2025

View reviewed changes

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)

packages/vue-compat/__tests__/misc.spec.ts (1)

287-289: Note: Pre-existing type annotation pattern (not blocking).

The static analysis tool flags the use of Function as a type, suggesting to explicitly define the function shape. However, this pattern is used consistently throughout the file (lines 262, 268, 274, etc.), so this is a pre-existing codebase pattern rather than an issue introduced by this change.

🧰 Tools

🪛 Biome (1.9.4)

[error] 288-288: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2a58947 and 5e0e684.

📒 Files selected for processing (2)

packages/runtime-dom/src/modules/attrs.ts (1 hunks)
packages/vue-compat/__tests__/misc.spec.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

packages/runtime-dom/src/modules/attrs.ts

🧰 Additional context used

🪛 Biome (1.9.4)

packages/vue-compat/__tests__/misc.spec.ts

[error] 288-288: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)

⏰ Context from checks skipped due to timeout of 90000ms (2)

GitHub Check: test / e2e-test
GitHub Check: test / unit-test-windows

🔇 Additional comments (1)

packages/vue-compat/__tests__/misc.spec.ts (1)

279-291: LGTM! Good test coverage for the compatibility flag.

The new test case effectively verifies that when ATTR_ENUMERATED_COERCION is explicitly enabled, the draggable="false" attribute is preserved correctly and the appropriate deprecation warning is emitted. This complements the existing test and follows the established testing patterns in the file.

🧰 Tools

🪛 Biome (1.9.4)

[error] 288-288: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)

skirtles-code

skirtles-code reviewed

May 27, 2025

View reviewed changes

Copy link

Contributor

@skirtles-code skirtles-code left a comment

Choose a reason for hiding this comment

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

A few more thoughts on the test...

packages/vue-compat/__tests__/misc.spec.ts Outdated Show resolved Hide resolved

edison1105 added 2 commits

November 24, 2025 10:27

@edison1105


 Merge branch 'main' into fix/compat-attr

a56b4ab

@edison1105


 test(compat): update ATTR_ENUMERATED_COERCION test to include spellch...

102f560

...eck attribute

@edison1105 edison1105 requested a review from skirtles-code

November 24, 2025 07:27

Labels

🔨 p3-minor-bug ready to merge scope: v2 compat

4 participants

@yangxiuxiu1115 @edison1105 @vue-bot @skirtles-code

Uh oh!

fix(compat): fix compat handler of draggable #12445

Are you sure you want to change the base?

fix(compat): fix compat handler of draggable #12445

Conversation

@yangxiuxiu1115 yangxiuxiu1115 commented Nov 20, 2024 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

github-actions bot commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Report

Bundles

Usages

Uh oh!

pkg-pr-new bot commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edison1105 commented Nov 21, 2024

Uh oh!

vue-bot commented Nov 21, 2024

Uh oh!

@skirtles-code skirtles-code May 20, 2025

Choose a reason for hiding this comment

Uh oh!

@skirtles-code skirtles-code May 20, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

@skirtles-code skirtles-code left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

@yangxiuxiu1115 yangxiuxiu1115 commented Nov 20, 2024 •

edited by coderabbitai bot

Loading

github-actions bot commented Nov 20, 2024 •

edited

Loading

pkg-pr-new bot commented Nov 20, 2024 •

edited

Loading

coderabbitai bot commented May 27, 2025 •

edited

Loading