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

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

Closed
krutidugade opened this issue Mar 16, 2024 · 7 comments · May be fixed by Automattic/jetpack#37025
Assignees
Labels
Customer Report Issues or PRs that were reported via Happiness. Previously known as "Happiness Request". Groundskeeping Worked on by Dotcom Groundskeeping [Pri] High [Status] Priority Review Triggered KitKat has been notified of this issue in #dotcom-triage-alerts Triaged [Type] Bug Something isn't working

Comments

@krutidugade
Copy link

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 krutidugade added [Type] Bug Something isn't working Needs triage labels Mar 16, 2024
Copy link
Contributor

Support References

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

  • -zen
  • 7894295-zen

@github-actions github-actions bot added the Customer Report Issues or PRs that were reported via Happiness. Previously known as "Happiness Request". label Mar 16, 2024
@github-actions github-actions bot added the [Status] Priority Review Triggered KitKat has been notified of this issue in #dotcom-triage-alerts label Mar 22, 2024
@mpkelly
Copy link

mpkelly commented Apr 22, 2024

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

@mpkelly
Copy link

mpkelly commented Apr 23, 2024

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, Automattic/jetpack#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

@lsl
Copy link

lsl commented Jul 26, 2024

I've drafted a PR, Automattic/jetpack#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?

@mpkelly
Copy link

mpkelly commented Aug 1, 2024

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

@vykes-mac
Copy link

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

@miksansegundo miksansegundo self-assigned this Aug 29, 2024
@miksansegundo
Copy link
Contributor

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

BEFORE AFTER
Screen.Recording.2567-08-29.at.19.12.24.mov
Screen.Recording.2567-08-29.at.19.11.39.mov

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 miksansegundo added the Groundskeeping Worked on by Dotcom Groundskeeping label Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Customer Report Issues or PRs that were reported via Happiness. Previously known as "Happiness Request". Groundskeeping Worked on by Dotcom Groundskeeping [Pri] High [Status] Priority Review Triggered KitKat has been notified of this issue in #dotcom-triage-alerts Triaged [Type] Bug Something isn't working
Development

Successfully merging a pull request may close this issue.

6 participants