Skip to content

fix: responsive foundation breakpoints, lang attribute, skip link, vw units#2906

Merged
sydseter merged 7 commits into
OWASP:masterfrom
Adarshkumar0509:fix/responsive-foundation
May 7, 2026
Merged

fix: responsive foundation breakpoints, lang attribute, skip link, vw units#2906
sydseter merged 7 commits into
OWASP:masterfrom
Adarshkumar0509:fix/responsive-foundation

Conversation

@Adarshkumar0509
Copy link
Copy Markdown
Contributor

@Adarshkumar0509 Adarshkumar0509 commented May 3, 2026

Responsive foundation fix for the issue: #2194

This PR addresses Phase 1. It is not a complete redesign just the foundational issues blocking correct responsive and i18n behavior.

Changes

Fixed hardcoded lang="en" in app.html now updates dynamically per language using %sveltekit.lang%
Replaced max-aspect-ratio: 1/1 with max-width: 767px across 43 source files
Replaced max-aspect-ratio: 1.5/1 with max-width: 1024px in card components
Added skip-to-content link see it fixes WCAG 2.4.1 keyboard navigation failure and Further skip link visual styling and SvelteKit first-Tab behaviour fix will be handled by @10-trix or @immortal71 in a follow-up PR.
Fixed how-to-play width from 59vw to min(59vw, 100%)
Fixed min-height: 22vw to min-height: auto in error container

Note: The ODT template fix (commit 0addcb9) is unrelated to responsive work and was included accidentally.

Reviewer Guidance

  • The 'max-width: 1024px' used for card components was chosen as a standard tablet/medium breakpoint to maintain consistent layout behavior, but I’m open to adjusting it if you think a different value fits better.
  • The CSP error showing in the console is already present on the live site and isn’t related to this PR.
  • I initially used '768px', but that caused the desktop navbar to collapse into the mobile menu at that exact width. Switching to '767px' keeps the desktop nav intact - caught this during testing.
  • I tested layouts at 375px (mobile portrait), 768px (tablet landscape), 1024px, and 1280px. Let me know if you want checks on any other breakpoints.

before:-
image

after:-
image

skip link proof
image

Metric Before After
Alerts 95 81
ARIA features 0 2
Features 6 8
AIM Score 7.1/10 7.1/10

AI Tool Disclosure

  • My contribution does not include any AI-generated content
  • My contribution includes AI-generated content, as disclosed below:
    • AI Tools: [Chat Gpt]
    • LLMs and versions: [GPT-4.1]
    • Prompts: [for review i used AI and also i was facing dificulty to understand certain things thats why i used it here]

Affirmation

Ensure TableBackQR10 and TableBackQR11 are inserted in the correct
position in the fallback _lang.odt template, matching the layout of
all other language-specific templates.

Closes OWASP#2133
… lang attribute, add skip-to-content link, fix how-to-play vw units
@Adarshkumar0509 Adarshkumar0509 force-pushed the fix/responsive-foundation branch from 0d0e8ec to c348059 Compare May 3, 2026 21:01
@Adarshkumar0509 Adarshkumar0509 marked this pull request as draft May 3, 2026 21:13
@Adarshkumar0509
Copy link
Copy Markdown
Contributor Author

@10-trix , @muhammad7865 , @immortal71 please take a look?

@Adarshkumar0509
Copy link
Copy Markdown
Contributor Author

once you guys are okay i will take it out of draft

@immortal71
Copy link
Copy Markdown
Contributor

@Adarshkumar0509 fix: use dynamic lang and add skip link

  • Change app.html to use %sveltekit.lang% for the lang attribute
  • Add “Skip to main content” link and #main-content wrapper for %sveltekit.body% to improve accessibility

@immortal71
Copy link
Copy Markdown
Contributor

@Adarshkumar0509 refactor: switch responsive rules to width-based breakpoints

  • Replace max-aspect-ratio media queries with max-width: 768px and 1024px across layout, hero, navbar, deck, card browser/preview, mappings, tables, and markdown renderers
  • Align mobile/tablet behavior to common width breakpoints for easier maintenance

@immortal71
Copy link
Copy Markdown
Contributor

chore: add missing EOF newlines

  • Add trailing newlines to Svelte and CSS files to satisfy POSIX and tooling expectations

@immortal71
Copy link
Copy Markdown
Contributor

@Adarshkumar0509 here is my main concern this pr has so many changes in main component which makes it too risky!!

@immortal71
Copy link
Copy Markdown
Contributor

immortal71 commented May 4, 2026

The i18n and skip-link fixes are solid, but the PR should ideally include before/after screenshots for mobile portrait, mobile landscape, tablet, and small desktop because the responsive changes affect many core components.

@immortal71
Copy link
Copy Markdown
Contributor

immortal71 commented May 4, 2026

Note this and do you have necessary devices above ? feel free to talk in grp If you need we can help each other, but the point it I will suggest checking responsiveness in every possible device !!

@Adarshkumar0509
Copy link
Copy Markdown
Contributor Author

okay on it.

@Adarshkumar0509
Copy link
Copy Markdown
Contributor Author

Adarshkumar0509 commented May 4, 2026

Hey @immortal71, i tried to address all the points
commit message suggestions i will kept that in mind for the future PRs. The EOF newlines point is also valid, I will add those in a follow-up.
On "too many changes in main components" the breakpoint changes are identical one line replacements, same pattern across all files, no logic touched. If sydseter prefers, I'm open to splitting into two PRs: one for lang/skip-link, one for breakpoints.

On screenshots looking identical i was also thinking this was expected. The old max-aspect-ratio bug only triggered in landscape mode (width > height). Portrait viewports were never broken. The regression I caught and fixed was at 768px landscape where my initial max-width: 768px was incorrectly showing the hamburger menu I changed it to max-width: 767px to fix this. Both sites now correctly show desktop nav at that size..

@Adarshkumar0509 Adarshkumar0509 force-pushed the fix/responsive-foundation branch from 7c432bd to 4cc785f Compare May 4, 2026 10:17
@Adarshkumar0509
Copy link
Copy Markdown
Contributor Author

@immortal71 @10-trix take a look buddy

@Mahaboobunnisa123
Copy link
Copy Markdown
Collaborator

This is a solid PR and the approach looks correct for a Phase 1 foundation. Focusing on fixing the responsive behavior first, along with lang handling and accessibility, makes sense and aligns well with the project priorities. The breakpoint refactor is an important fix, and the 767px adjustment shows careful testing rather than just applying a standard value. The reviewer guidance is also clear and helpful, and the screenshots make it easier to validate the changes.
Just a couple of minor points: the line about the ODT template change could be clarified a bit, and adding a short reason for choosing the 1024px breakpoint would help future maintainability. Overall, this is a good base to build on 👍

@Adarshkumar0509
Copy link
Copy Markdown
Contributor Author

hii @Mahaboobunnisa123 Thank you so much.

@ANNI160
Copy link
Copy Markdown
Contributor

ANNI160 commented May 4, 2026

Great work taking the initiative on Phase 1, @Adarshkumar0509. I pulled this branch locally to test. The switch from max-aspect-ratio to max-width: 767px was a smart catch, and the responsive layouts look great across mobile and tablet widths.

However, I noticed an issue with the accessibility changes: The 'Skip to main content' link does not appear to be working correctly. As shown in your 'after' screenshot, pressing Tab causes the focus to jump straight to the downward arrow icon in the center (routing to /#top), rather than revealing a visual 'Skip to main content' text link at the top of the page. You might need to double-check the HTML placement or the CSS :focus state for that specific skip link.

Also, I have one architectural recommendation: You mentioned the ODT template commit (0addcb9) was accidentally added. Because this PR touches 61 files focused on the UI foundation, I highly recommend dropping the ODT commit from this branch and opening it as a separate PR so it can be merged immediately.

(Also, as mentioned in #2194, I'd love to help the team out by building an automated Lighthouse/WCAG CI check via GitHub Actions to catch accessibility regressions automatically. Let me know when you have a second to add me to the Slack group!)

@Adarshkumar0509
Copy link
Copy Markdown
Contributor Author

Adarshkumar0509 commented May 4, 2026

hii @ANNI160 yeah sure you can help us in code reviewing and many other big issues are there as well means you can get a overall experience, is this fine for you? yeah i will look on your suggestions

@ANNI160
Copy link
Copy Markdown
Contributor

ANNI160 commented May 4, 2026

Sounds perfect to me! Happy to help review and tackle some of those other issues. I'll keep an eye out for the Slack invite so we can coordinate. Thanks, @Adarshkumar0509 !

@Adarshkumar0509
Copy link
Copy Markdown
Contributor Author

hii,@ANNI160 reviewing is much bigger task i love you accepted this and also there are some issues as well we will love if you also tackle them with us!

@ANNI160
Copy link
Copy Markdown
Contributor

ANNI160 commented May 4, 2026

Thanks, @Adarshkumar0509 ! I completely agree—reviewing is where we catch the really tricky edge cases. I'm excited to dive into the other open issues with the team as well. Just let me know once the Slack is set up so we can coordinate our next moves!

@Adarshkumar0509 Adarshkumar0509 marked this pull request as ready for review May 5, 2026 06:35
@Adarshkumar0509
Copy link
Copy Markdown
Contributor Author

hii @sydseter can you take a look at this? and suggest something

@sydseter sydseter merged commit db8839b into OWASP:master May 7, 2026
9 checks passed
@sydseter
Copy link
Copy Markdown
Collaborator

sydseter commented May 7, 2026

@Adarshkumar0509 Thank you for your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants