themes icon indicating copy to clipboard operation
themes copied to clipboard

[Bug]: Content overflows outside content area on mobile devices for theme Feelin' Good

Open krutidugade opened this issue 1 year ago • 3 comments

Quick summary

Feelin' Good theme is not mobile responsive. The content added overflows outside content area for mobile devices. Some of the content can't be seen. User reported issue with videos on their site in -zen. I could replicate this on my site - https://testbizplanplayground.wpcomstaging.com/home-2/

Screenshot 2024-03-16 at 12 56 06 AM

Steps to reproduce

  1. Activate Feelin' Good theme - https://wordpress.com/theme/feelingood
  2. Create a page and add image block with image
  3. Add videopress block with a video
  4. Add few column blocks with content
  5. Add paragraph block with some text
  6. Publish the page
  7. Check the page on mobile device

What you expected to happen

The content should appear within the content area

What actually happened

The content (text, image, video) stretches outside the content area making a portion of it not visible to viewers

Browser

Google Chrome/Chromium

Context

Customer report in 7894295-zen regarding video not fitting in the frame.

Platform (Simple, Atomic, or both?)

Simple, Atomic

Other notes

No response

Reproducibility

Consistent

Severity

None

Available workarounds?

None

Workaround details

On inspecting I noticed, this is caused because of following CSS

body .is-layout-flex { display: flex; }

Changing to display:block fixes the issue but I feel this workaround is incorrect. Let me know if this would work as temporary fix:

@media only screen and (max-width: 600px) { body .is-layout-flex { display: block; } }

krutidugade avatar Mar 16 '24 01:03 krutidugade

Support References

This comment is automatically generated. Please do not edit it.

  • [ ] -zen
  • [ ] 7894295-zen

github-actions[bot] avatar Mar 16 '24 01:03 github-actions[bot]

I have reproduced this and will look at a fix now.

mpkelly avatar Apr 22 '24 13:04 mpkelly

This is turning out to be a tricky issue. The fix is simple in theory but can be made in different places: core, Jetpack, and individual theme files. I've found that most themes don't have this issue, but comparing those with Feelin' Good, I can't see what's different. Ideally, I would make Feelin' Good consistent with them rather than add some special CSS to its theme.

Another option is to update Jetpack in a way that iframe use width:100%; max-width: nnnpx instead of width:nnnpx as the first one will not overflow like the containers. I've drafted a PR, https://github.com/Automattic/jetpack/pull/37025, but I'm still not sure it's the best solution and testing turned out to be problematic anyway.

I also found that the Freddie theme has a similar issue to the one reported here. There are probably others, too, which makes me think that we should fix it in the block. I will try and deploy something tomorrow.

Screenshot 2567-04-23 at 15 50 26

mpkelly avatar Apr 23 '24 14:04 mpkelly

I've drafted a PR, https://github.com/Automattic/jetpack/pull/37025, but I'm still not sure it's the best solution and testing turned out to be problematic anyway.

This looks workable to me with the aspect-ratio change. Let's just make sure we're not breaking other themes in the process:

Other themes like twentytwentythree / four are doing dynamic resizing of the iframe to avoid this overflow. Is that part of videopress or the theme itself?

lsl avatar Jul 26 '24 02:07 lsl

Other themes like twentytwentythree / four are doing dynamic resizing of the iframe to avoid this overflow. Is that part of videopress or the theme itself?

This is the resize function that sets the iframe's width and height: Screenshot 2567-08-01 at 16 17 55

This code is in the script https://v0.wordpress.com/js/next/videopress-iframe.js?m=1674852142 and runs on load. It executes for all themes I checked. The value it uses for the width comes from the parent element, so it depends on the CSS rules used in the parent DOM hierarchy, and in Feelin' Good, those rules are different from those in other themes, where it doesn't overflow.

Let's just make sure we're not breaking other themes in the process:

This is always the tricky part with so many themes out there. But based on how T23 and T24 work today, setting width to be 100% shouldn't have any adverse effects. I will test a fix tomorrow with a mix of themes.

I'll also update the aspect-ratio.

@lsl

mpkelly avatar Aug 01 '24 09:08 mpkelly

@mpkelly You think this should be flagged as project or apply simple fix and provide more elegant solution later on?

vykes-mac avatar Aug 21 '24 17:08 vykes-mac

I'm taking this over. I have found a solution.

BEFORE AFTER

The issue is because the script gets the iframe parent to get the width without removing the iframe from the DOM tree so it cannot return the correct value.

The fix would be something like this:

+  // Hide the iframe to calculate the real parent width
+  t.firstElementChild.style.display = 'none';
    var r = getComputedStyle(t), n = parseFloat(r.width);
+  // Show the iframe after reading the parent's width
+  t.firstElementChild.style.display = null; 

Where can I find the source file for videopress-iframe.js to make that change? I haven't found it in PCYsg-28C-p2

miksansegundo avatar Aug 29 '24 12:08 miksansegundo