Skip to content
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

Fix unable to set progress value to 0 #2757

Merged
merged 1 commit into from
Sep 20, 2020
Merged

Conversation

marvinhagemeister
Copy link
Member

Due to a weird oddity in the DOM interface the value 0 is only treated as 0 when the value attribute is present. When the value is not present the progress bar is treated as being indeterminate. But the problem is that the DOM interface will always have all properties and the initial value for value is 0. This mismatch caused us to never set the progress bar value to 0

Fixes #2756 .

@github-actions
Copy link

github-actions bot commented Sep 19, 2020

Size Change: +64 B (0%)

Total Size: 40.8 kB

Filename Size Change
dist/preact.js 4.08 kB +15 B (0%)
dist/preact.min.js 4.11 kB +16 B (0%)
dist/preact.module.js 4.1 kB +16 B (0%)
dist/preact.umd.js 4.14 kB +17 B (0%)
ℹ️ View Unchanged
Filename Size Change
compat/dist/compat.js 3.17 kB 0 B
compat/dist/compat.module.js 3.18 kB 0 B
compat/dist/compat.umd.js 3.23 kB 0 B
debug/dist/debug.js 3.09 kB 0 B
debug/dist/debug.module.js 3.09 kB 0 B
debug/dist/debug.umd.js 3.18 kB 0 B
devtools/dist/devtools.js 185 B 0 B
devtools/dist/devtools.module.js 195 B 0 B
devtools/dist/devtools.umd.js 261 B 0 B
hooks/dist/hooks.js 1.1 kB 0 B
hooks/dist/hooks.module.js 1.12 kB 0 B
hooks/dist/hooks.umd.js 1.18 kB 0 B
test-utils/dist/testUtils.js 437 B 0 B
test-utils/dist/testUtils.module.js 439 B 0 B
test-utils/dist/testUtils.umd.js 515 B 0 B

compressed-size-action

@coveralls
Copy link

coveralls commented Sep 19, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 54c006d on progress-value into b17a932 on master.

// despite the attribute not being present. When the attribute
// is missing the progress bar is treated as indeterminate.
// To fix that we'll always update it when it is 0 for progress elements
(i !== dom.value || (newVNode.type === 'progress' && !i))
Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out my previous fix of doing !dom.hasAttribute('value') is not a good one as it leads to all input values always being updated. The new fix only updates it, despite dom.value === props.value when the value is 0 for progress elements.

@marvinhagemeister marvinhagemeister merged commit 134f65c into master Sep 20, 2020
@marvinhagemeister marvinhagemeister deleted the progress-value branch September 20, 2020 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting the value of a progress element to 0 removes the attribute
3 participants