Title says it all, percentage based dimensions are valid markup and should therefore be allowed.

Patch incoming.

CommentFileSizeAuthor
#9 respect_percentage-2058397-9.patch475 bytessubhojit777
#8 respect_percentage-2058397-8.patch475 bytessubhojit777
#2 youtube-responsive-dimensions-2058397-2.patch2.07 KBguschilds
#1 percentage_dimensions-2058397-1.patch590 bytesdeciphered

Comments

deciphered’s picture

deciphered
Primary language English
Location Ballarat, Victoria, Australia
commented
Status: Active » Needs review
StatusFileSize
new percentage_dimensions-2058397-1.patch590 bytes

Patch

guschilds’s picture

guschilds commented
Category: task » feature
StatusFileSize
new youtube-responsive-dimensions-2058397-2.patch2.07 KB

Thanks for the patch!

I tested it, but I'm not sure how practical it is. I'm assuming the intention is to create a responsive effect on the player. The problem is that 'height' is still accepted. When I picked 'custom' size with a width of '100%', which worked, but the pixel-based and percentage-based heights I tested didn't work in a way that made sense.

After seeing this patch, I think the best idea would be to offer 'responsive' as a size choice on the field display settings. I've dug into what it takes to make these responsive before in #22 and #27 of #1571420: Add additional YouTube player options, so I've added a bit of CSS that uses that technique to bring a new 'responsive' option to life in the attached patch.

To test the attached patch:

  1. Install Drupal with the YouTube Field module enabled
  2. Create a YouTube-type field on a content type
  3. Edit the field display settings for that field, and choose 'responsive (full-width)' as the size choice
  4. Create and view a piece of content with a value in that field

The video's player should responsively take up 100% of the width of it's container. If you want the player to take up less than 100% of it's parent, you can use additional CSS to apply a smaller width to any container element around the player.

I'm guessing this is an increasingly common use case these days, so I'd love to get this patch committed if it fulfills the need.

Let me know what you think!

guschilds’s picture

guschilds commented
Title: Respect percentage based dimenstion. » Respect percentage based dimension (responsive behavior).
Version: 7.x-1.x-dev » 7.x-1.0
deciphered’s picture

deciphered
Primary language English
Location Ballarat, Victoria, Australia
commented
Title: Respect percentage based dimension (responsive behavior). » Respect percentage based dimenstion.
Version: 7.x-1.0 » 7.x-1.x-dev

Please don't derail this issue. If you want a responsive mode, that's fine, but that doesn't discount the ability to use percentages in the height/width attributes, which is valid markup regardless of whether you personally don't see how practical it is yourself.

I would recommend that if you need more than just enabling percentage based height/width attributes that you open a secondary issue.

guschilds’s picture

guschilds commented

I've moved the responsive stuff to #2077333: Add a responsive setting to the video display size choices.

The W3C spec for iframes specifies "...in CSS pixels" for the width and height attributes. That is why I question the practicality of accepting % values before committing the patch.

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
Assigned: Unassigned » subhojit777
Issue summary: View changes
Issue tags: +Needs reroll

I have this requirement too. Currently it does not respects percentage (like 100%) or pixel values (like 500px).

My use case:
I need width of player to be responsive and fixed height, in this case I require relative width of youtube player and absolute pixelated height.

guschilds’s picture

guschilds commented
Status: Needs review » Needs work
subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
Assigned: subhojit777 » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new respect_percentage-2058397-8.patch475 bytes

Uploading patch. Thanks to @Deciphered for the pointer. I have made minor modifications, it will not check the type of setting, the setting will be used as it is.

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
StatusFileSize
new respect_percentage-2058397-9.patch475 bytes

Incorrect patch uploaded in #8. Uploading correct patch.

interdruper’s picture

interdruper commented
Status: Needs review » Reviewed & tested by the community

#9 works. Now a custom size mixing % and pixels is allowed, like width="100%" and height="480px".

Note that in some browsers the combination width="100%" and height="100%" or height="auto" does not work properly, so a fixed height is required.

  • guschilds committed 4f810ef on 7.x-1.x authored by Deciphered 
    Issue #2058397 by Deciphered, subhojit777: Respect percentage-based...
guschilds’s picture

guschilds commented
Status: Reviewed & tested by the community » Fixed

The patch in #9 was not committed because:

  • it allowed for JavaScript to be injected onto the page. For example, enter auto"></iframe><script>alert('hey!');</script><iframe as a dimension and load a page with a video player.
  • The HTML spec for iframes specifies that valid values are pixel-based and percentage-based in HTML 4.01 and only pixel values in HTML5. "px" is never needed and "auto" isn't valid.

My statement in #5 about pixels only was only correct for HTML5. I've committed the original patch to the 7.x-1.x branch and percentage-based values are now properly applied when using custom player dimensions.

Thanks for the patches, testing, and patience everyone.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.