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

Editorial: say which <source> attributes are allowed where #6419

Merged
merged 2 commits into from Mar 2, 2021

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Feb 26, 2021

@annevk
Copy link
Member

annevk commented Feb 26, 2021

Interesting, pulling in the attribute description only works for single-page. Does that still work if we do this? (I tried building locally, but I keep running into some error.)

@zcorpan
Copy link
Member Author

zcorpan commented Feb 26, 2021

Oh, this should go in the attribute index? I forgot how this was set up. But does the attribute index then need separate rows for srcset for img and srcset for source?

@annevk
Copy link
Member

annevk commented Feb 26, 2021

I think if we put it in the attribute index it won't show up in the multi-page version due to the aforementioned bug. So I think this is fine, I'm just wondering what the output looks like for single-page since I cannot do that myself easily at the moment.

@domenic
Copy link
Member

domenic commented Feb 26, 2021

I'm pretty sure it works in the multi-page version, just not in PR Preview.

@annevk
Copy link
Member

annevk commented Feb 26, 2021

Oh, because PR Preview invokes Wattsi rather than the build script? We should fix that some day... (I guess there's a similar problem with the other specs as there it prolly invokes Bikeshed directly.)

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.

I think we should be consistent with link, which would mean putting this in the attribute index. Compare to line 120160.

@zcorpan
Copy link
Member Author

zcorpan commented Feb 26, 2021

@domenic ok, but by adding the text to the attribute index, it will be included for the img element as well for attributes that are shared between img and source (e.g. srcset), which would be wrong. How do we fix that?

@domenic
Copy link
Member

domenic commented Mar 1, 2021

Oh, I see, that is unfortunate.

I guess we have a few options:

  1. Split these entries in the attribute index out into separate entries.

  2. Do what you've done here, leading to a different style between <link> and <source>.

  3. Change the <link> attributes to the style you have here, meaning the attribute index won't list the restriction.

  4. Make changes to the tooling which assembles these to handle this more holistically somehow.

All of these have downsides. I think I am most comfortable with (2), i.e. your current version. Two suggestions:

  • In the element intro, put the shared type attribute before the surrounding-element-specific ones. It'd look tidier.

  • Update the attribute index (for all source attributes) to include similar information, e.g. for width, it could say "canvas; embed; iframe; img; input; object; source (in picture); video".

@annevk
Copy link
Member

annevk commented Mar 2, 2021

I agree with that. I think we should reserve splitting in the attribute index for when the attributes have different functions (e.g., type).

@zcorpan
Copy link
Member Author

zcorpan commented Mar 2, 2021

@domenic OK, sounds good. Done. Your suggested change to the attribute index requires a change to html-build though, which I've implemented here: whatwg/html-build#262

sideshowbarker pushed a commit to whatwg/html-build that referenced this pull request Mar 2, 2021
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.

Really nice improvement

@domenic domenic merged commit fade69e into main Mar 2, 2021
@domenic domenic deleted the bocoup/clarify-source-attrs branch March 2, 2021 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Make it clearer source element attributes are bifurcated
3 participants