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

Correct <source> dimension property handling #9752

Closed
wants to merge 2 commits into from
Closed

Conversation

annevk
Copy link
Member

@annevk annevk commented Sep 18, 2023

Fixes #8178.

  • At least two implementers are interested (and none opposed):
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chromium: …
    • Gecko: …
    • WebKit: …
    • Deno (only for timers, structured clone, base64 utils, channel messaging, module resolution, web workers, and web storage): …
    • Node.js (only for timers, structured clone, base64 utils, channel messaging, and module resolution): …
  • MDN issue is filed: …

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


💥 Error: Wattsi server error 💥

PR Preview failed to build. (Last tried on Sep 20, 2023, 8:41 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.

🔗 Related URL

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

@annevk
Copy link
Member Author

annevk commented Sep 18, 2023

@BorisChiou @emilio could you confirm this fixes the issue? Furthermore we probably don't need additional test coverage for this as https://github.com/web-platform-tests/wpt/blame/master/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/picture-aspect-ratio.html by @cbiesinger already covers this? Maybe he could review this too.

@annevk annevk requested a review from zcorpan September 18, 2023 15:03
Copy link

@cbiesinger cbiesinger left a comment

Choose a reason for hiding this comment

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

The commit message would probably benefit from mentioning the specific example this is meant to fix (#8178 (comment))

I'm undecided on whether I like this change in general, but since there's no way to specify height="auto" in HTML I guess this is an improvement.

<p>When the text below says that an attribute <var>attribute</var> on an element
<var>element</var> <dfn>maps to the dimension property (with fallback)</dfn> (or properties)
<var>properties</var>, then <var>attribute</var> on <var>element</var> <span>maps to the dimension
property</span> <var>properties</var>. And if <var>element</var> does not have an attribute

Choose a reason for hiding this comment

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

This should probably treat a parse error the same as a missing attribute?

(ref: line 128634)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, pushed a fix for that.

I'll make sure to include the example in the commit message when landing this.

@annevk annevk added the do not merge yet Pull request must not be merged per rationale in comment label Sep 20, 2023
@annevk
Copy link
Member Author

annevk commented Sep 20, 2023

As per #8178 (comment) this whole PR might be unneeded. If that's true (and it seems like it is) I suggest we close this PR and create a new one for the note, to avoid confusion.

@zcorpan zcorpan closed this Sep 25, 2023
@annevk annevk deleted the annevk/source-dim branch September 25, 2023 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge yet Pull request must not be merged per rationale in comment
Development

Successfully merging this pull request may close these issues.

The missing property values of width/height attributes on source elements
3 participants