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

Once <progress> value attribute has been set, it will never be removed. #4487

Closed
1 task done
jacobalbano opened this issue Sep 5, 2024 · 8 comments · Fixed by #4492
Closed
1 task done

Once <progress> value attribute has been set, it will never be removed. #4487

jacobalbano opened this issue Sep 5, 2024 · 8 comments · Fixed by #4492

Comments

@jacobalbano
Copy link

jacobalbano commented Sep 5, 2024

  • Check if updating to the latest Preact version resolves the issue

Describe the bug
Once <progress> value attribute has been set, it will never be removed.

On this element, the absence of a value attribute has significance:

If there is no value attribute, the progress bar is indeterminate; this indicates that an activity is ongoing with no indication of how long it is expected to take.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/progress

Currently I'm unable to remove the attribute without resorting to direct DOM manipulation.

To Reproduce
Run the following code at https://preactjs.com/repl

import { render, Component } from 'preact';

class ProgressDemo extends Component {
	constructor() {
		super();
		this.state = { loading: true };
	}

	render() {
		const { loading } = this.state;
		const value = loading ? undefined : 1;
		return (
			<div>
				<div>Progress should {loading || 'not'} be animated.</div>

				<div>
					<progress value={value} />
					<button onClick={() => this.setState({ loading: !loading })}>
						Loading ({String(loading)})
					</>
				</>

				<div><code>state.loading</code> is {String(loading)}</div>
				<div><code>progress.loading</code> is being set to {String(value)}</div>
			</>
		);
	}
}

render(<ProgressDemo />, document.getElementById('app'));

Expected behavior

  • Progress bar is initially rendered as <progress>, with no value attribute, so it animates in an indeterminate state
  • Clicking the button sets loading to false, which results in the html <progress value="1">. Animation stops.
  • Clicking the button again sets loading to true, which results in the same initial html, <progress> with no value attribute. Animation resumes.

Actual behavior

  • Once value has been set once, attempting to set it to a falsey value will will have no effect; the attribute will retain its previous value and will not be removed.
@jacobalbano
Copy link
Author

jacobalbano commented Sep 5, 2024

Here's an identical demo using htm instead of jsx:

import { render, Component } from 'preact';
import { html } from 'htm/preact';

export default class ProgressDemo extends Component {
	constructor() {
		super();
		this.state = { loading: true };
	}

	render() {
		const { loading } = this.state;
		const value = loading ? undefined : 1;
		return html`
			<div>
				<div>Progress should ${loading || 'not'} be animated.</div>

				<div>
					<progress value=${value} ></progress>
					<button onClick=${() => this.setState({ loading: !loading })}>
						Loading (${String(loading)})
					</div>
				</div>

				<div><code>state.loading</code> is ${String(loading)}</div>
				<div><code>progress.loading</code> is being set to ${String(value)}</div>
			</div>
		`;
	}
}

render(html`<${ProgressDemo} />`, document.getElementById('app'));

@rschristian

This comment has been minimized.

@jacobalbano
Copy link
Author

jacobalbano commented Sep 5, 2024

@rschristian With all due respect, the ternary is not the issue. Here's a (less minimal) example where I do exactly as you suggest; initially undefined, 1 to indicate completion, and null to unset. This does change the behavior, but the value attribute is not removed, it is simply set to 0 and the bar does not animate.

import { render, Component } from 'preact';

class ProgressDemo extends Component {
	constructor() {
		super();
		this.state = { loading: 'initial' };
	}

	get animating() {
		const { loading } = this.state;
		return loading == 'initial' || loading == 'loading';
	}

	get value() {
		switch (this.state.loading) {
			case 'initial': return undefined;
			case 'loading': return null;
			case 'done': return 1;
		}
	}

	render() {
		const { animating } = this;
		const { loading } = this.state;
		return (
			<div>
				<div>Progress should {animating || 'not'} be animated.</div>

				<div>
					<progress value={this.value} />
					<button onClick={() => {
						let nextState = 'initial';
						switch (loading) {
							case 'initial':
							case 'loading':
								nextState = 'done';
								break;
							case 'done':
								nextState = 'loading';
								break;
						}
						
						this.setState({ loading: nextState  })				
					}}>
						Loading ({String(loading)})
					</>
				</>

				<div><code>state.loading</code> is {String(loading)}</div>
				<div><code>progress.loading</code> is being set to {String(this.value)}</div>
			</>
		);
	}
}

render(<ProgressDemo />, document.getElementById('app'));

@jacobalbano
Copy link
Author

jacobalbano commented Sep 5, 2024

Here's a new (extremely) minimal example where you can see that passing null to this value initially creates an attribute with the value of 0.

import { render } from 'preact';
render(<progress value={null} />, document.getElementById('app'));

@jacobalbano
Copy link
Author

My initial example works perfectly in React for what it's worth (ternary and all).

Run here: https://playcode.io/react

import React, { Component } from 'react';

export class App extends Component {
	constructor() {
		super();
		this.state = { loading: true };
	}

	render() {
		const { loading } = this.state;
		const value = loading ? undefined : 1;
		return (
			<div>
				<div>
					<progress value={value} />
					<button onClick={() => this.setState({ loading: !loading })}>
						Loading ({String(loading)})
					</button>
				</div>
			</div>
		);
	}
}

@JoviDeCroock
Copy link
Member

That's a really good find, I reckon the origin of the bug would be here

@rschristian
Copy link
Member

Sorry, I had misunderstood what you were after in that case. Generally, you don't go from determinate back to indeterminate (often controls don't let you) so I assumed you were after setting progress back to 0, with the value attr just being an x/y problem.

Apologies.

@jacobalbano
Copy link
Author

jacobalbano commented Sep 5, 2024

@rschristian I see, yeah my use case is just to give an indication that a search is running. At no point do I have concrete progress information in this particular instance.

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 a pull request may close this issue.

3 participants