-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
check dimension attribute values on tag #1454
Conversation
Do browsers misbehave if they're given invalid dimension attributes? |
Well, I'm pretty sure browsers just ignore them, so we should too, to prevent the default dimension options from being overwritten by sloppy authoring. (We map these values to CSS, which isn't quite what the browser does, so it's not quite apples-to-apples.) The values I'm mostly concerned about disallowing are: "auto", empty string (possibly generated by a template?), and percentage units. Easier to handle this in the console than in bug reports. :) |
Check out #1449. It's a later PR where we discovered the browsers just change NaN and null to zero. We can't really block 'auto' until #983 is implemented. Auto was a way to allow you to set the player dimensions with CSS, since internally were using inline styles and a default 300x150. It's not great but this PR would kill that option. |
I'm not as worried about NaN here, although that's generally probably good to check for too. NaN and null aren't what I'm checking for here. The spec's integer requirement on the attribute dimensions is stricter than regular CSS, which allows floats, and will actually render them in some cases with sub-CSS-pixel precision sometimes on retina devices. See my comments on #983 for why I think hardcoding in a class might be the wrong approach. Let's not wait for that to deal with this?
Yeah, that's the option I want to kill :) Hijacking the tag's dimension attribute with invalid values to fill a hole in the API feels yucky, and complicates writing tests. Let's just fill the hole! First of all, authors can use CSS to override inline styles on any of these elements. You just have to use As I understand it, the scenario you're trying to address (let's call it "PURECSS") by allowing But there is no way to deliver all of the responsive use cases, and the height/width API, and the browser 2:1 default aspect ratio without any inline css. It's just impossible. There's too much conditional logic. You can capture a lot of it, but not all. The only way to address the PURECSS use case that I see is to expose a boolean option, call it |
Any thoughts on this since my comment on #983? Currently this feels like a small piece of a bigger dimension handling change that breaks how some users are currently using dimensions. I think I'd prefer to close this and wrap it into the bigger overhaul of dimensions handing. |
Yeah, I agree this isn't urgent. I am just trying to parcel the big dimension issues out, and it's tricky because they're so intertwined. This seemed like a simple check to add. FWIW with this PR, and the existing code, authors could still nuke inline css with:
They just can't do it with:
which is technically invalid. If you like we can put this aside, but I think the point basically stands. |
Ok yeah, I think we should close this for now, but we'll be sure to kill the |
see inline comments.