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

Use correct properties for progress element #8471

Merged
merged 1 commit into from
Nov 10, 2022

Conversation

dizhang168
Copy link
Contributor

@dizhang168 dizhang168 commented Nov 3, 2022

In Progress Element's description of how to render native appearance [1], we should use block-size: 1em instead of height: 1em and inline-size: 10em instead of width: 10em. This change will not affect existing behavior for horizontal writing mode (the specced behavior).

(See WHATWG Working Mode: Changes for more details.)

The choice to support writing-mode is left to the UA [2]. Blink would like to enable writing-mode vertical for control forms, including the Progress element. The changes proposed in this PR will be correct for both writing mode vertical and horizontal. For more implementation details, see chromium CL [3].

[1] https://html.spec.whatwg.org/multipage/rendering.html#the-progress-element-2
[2] https://drafts.csswg.org/css-ui-4/#appearance-switching:~:text=User%20agents%20may%20disregard%20some%20CSS%20properties%20on%20widgets%20rendered
[3] https://chromium-review.googlesource.com/c/chromium/src/+/3979781


/rendering.html ( diff )

@domenic
Copy link
Member

domenic commented Nov 4, 2022

Can you restore the pull request template, including implementer interest, tests, and browser bugs?

@dizhang168
Copy link
Contributor Author

Thanks, I wrongly assumed this was just an editorial change. Please let me know if it is missing anything.

@domenic
Copy link
Member

domenic commented Nov 4, 2022

I think non-manual tests should be possible here. I can think of a few ideas:

I agree that because of the clause you linked about native appearance, these tests would be .optional.html tests. But adding them would still be good.

@dizhang168
Copy link
Contributor Author

dizhang168 commented Nov 4, 2022

Thanks for your recommendations, I just changed the WPT tests in web-platform-tests/wpt#36659 to be .optional.html.
Do you think I should also add getComputedStyle() tests too?

@domenic
Copy link
Member

domenic commented Nov 7, 2022

Do you think I should also add getComputedStyle() tests too?

If possible, yes!

@dizhang168
Copy link
Contributor Author

dizhang168 commented Nov 8, 2022

Added the new test css/css-writing-modes/progress-appearance-native-computed-style.optional.html
where the assertions are:

  const style = getComputedStyle(progress);
  assert_equals(style.blockSize, "16px");
  assert_equals(style.inlineSize, "160px");
  assert_equals(style.blockSize, style.height);
  assert_equals(style.inlineSize, style.width);

https://github.com/web-platform-tests/wpt/blob/a3fa0d0057e28acd94903a898e8d0eda2bffd253/css/css-writing-modes/progress-appearance-native-computed-style.optional.html

Edit: I merged the CL in chromium since it is experimental and the WPT tests are all optional. If more feedback, I am happy to add more patches.

@domenic domenic force-pushed the progress-appearance-properties branch from ff68b71 to 506d9d6 Compare November 9, 2022 06:33
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM! I will follow up internally with how to get the participation check to pass, and then we can merge.

@domenic domenic merged commit 989886e into whatwg:main Nov 10, 2022
@domenic
Copy link
Member

domenic commented Nov 10, 2022

All merged! Please remember to file a bug on bugs.webkit.org pointing to this PR and to the new tests, and edit the original post to include a link to it.

@dizhang168 dizhang168 deleted the progress-appearance-properties branch November 21, 2022 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants