Declare skin to be responsive, remove extra viewport tag (fixes #470) #471
Conversation
…ssionalWiki#470) * Tell the MW core that this skin supports responsive behavior. * Stop manually emitting a "viewport" meta tag from Chameleon. MW `Skin` always emits a "viewport" meta tag --- but it needs to know if a skin supports responsive behavior in order to emit the correct tag. On top of that, the additional tag which `Chameleon` was emitting was either redundant, or had different/competing values for `width` (so some mobile browsers still performed non-responsive rendering, despite the extra tag).
📝 WalkthroughWalkthroughThe changes remove the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related issues
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🔇 Additional comments (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| return new Chameleon( [ | ||
| 'name' => 'chameleon', | ||
| 'styles' => $styles, | ||
| 'responsive' => true, |
There was a problem hiding this comment.
Is this a new MediaWiki feature? Since when is it available?
There was a problem hiding this comment.
I think it's been there "for a while". As far as Chameleon support is concerned, it's available in MW 1.39+.
There was a problem hiding this comment.
From a quick git blame, it looks like emitting the viewport tag for a true responsive option goes back to at least 2020-08-03 (204ddf052296), and emitting the fixed-width viewport tag for the non-responsive case was first added 2 years later, in 2022-05-09 (f5afb5c48c9a). (So, 2022-05 is when the two competing viewport tags would have started appearing in the HTML output with Chameleon.)
| parent::initPage( $out ); | ||
|
|
||
| // Enable responsive behaviour on mobile browsers | ||
| $out->addMeta( 'viewport', 'width=device-width, initial-scale=1, shrink-to-fit=no' ); |
There was a problem hiding this comment.
This is based on Bootstrap 4's recommended meta tag:
https://getbootstrap.com/docs/4.6/getting-started/introduction/#responsive-meta-tag
Can you confirm that the difference between that and MediaWiki's tag does not cause issues or changes in mobile behavior?
..., shrink-to-fit=no"/>
..., user-scalable=yes, minimum-scale=0.25, maximum-scale=5.0"/>
(from #470 (comment))
There was a problem hiding this comment.
FWIW, my (limited) understanding is that shrink-to-fit is a 10-year-old Safari-specific "band-aid": https://www.scottohara.me/blog/2018/12/11/shrink-to-fit.html
Conversely, user-scalable=yes is the default value for viewport meta tags. (And the default min/max are 0.1 and 10, according to MDN.)
(As far as real-world testing, all I did is check that the PR made my observed misbehavior go away, on a handful of browsers on Android.)
3 participants
MW
Skinalways emits a "viewport" meta tag --- but it needs to know if a skin supports responsive behavior in order to emit the correct tag.On top of that, the additional tag which
Chameleonwas emitting was either redundant, or had different/competing values forwidth(so some mobile browsers still performed non-responsive rendering, despite the extra tag).Summary by CodeRabbit
New Features
Refactor