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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃枍 [Story interactive] Adapt slider styles to work on Firefox #35432

Merged
merged 6 commits into from Jul 28, 2021

Conversation

mszylkowski
Copy link
Contributor

@mszylkowski mszylkowski commented Jul 28, 2021

Firefox renderer doesn't follow certain things that Chrome does:

  • Input elements (and other replaced content) don't have pseudo-elements
  • Thumb has different vendor prefix that needs to be on a different CSS ruleset (or it breaks on Chrome)
  • z-index for thumbs follow the input element's value.
  • Input element has different styling

Demo: https://stories-demos-matias.web.app/examples/amp-story/interactive_slider.html

BeforeAfter

@mszylkowski mszylkowski self-assigned this Jul 28, 2021
@mszylkowski mszylkowski added this to In progress in wg-stories Sprint via automation Jul 28, 2021
@mszylkowski mszylkowski marked this pull request as ready for review July 28, 2021 14:10
@amp-owners-bot
Copy link

Hey @gmajoulet! These files were changed:

extensions/amp-story-interactive/0.1/amp-story-interactive-slider.css

var(--interactive-accent-color) var(--percentage-corrected)
) !important;
opacity: 0.3 !important;
z-index: 0 !important;
}

.i-amphtml-story-interactive-slider-input::-webkit-slider-thumb {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the .i-amphtml-story-interactive-slider-input::-moz-range-thumb selector be added to this ruleset with the addition of border: none !important;?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's sth about the selector that breaks when we specify the two rules in the same ruleset like a, b {}. I think because Chrome doesn't know about the Firefox prefixes, it invalidates the ruleset. That's why we need 2 separate rulesets here.

More info at https://css-tricks.com/styling-cross-browser-compatible-range-inputs-css/

z-index: 2 !important;
}

.i-amphtml-story-interactive-mid-selection .i-amphtml-story-interactive-slider-input::-moz-range-thumb {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above, maybe this selector can be added to the .i-amphtml-story-interactive-mid-selection .i-amphtml-story-interactive-slider-input::-webkit-slider-thumb ruleset.

Copy link
Contributor

@processprocess processprocess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSS can be combined in a couple places but this looks good to me!

@mszylkowski
Copy link
Contributor Author

@processprocess does not work, since combining the rules will make both browsers fail to parse the selector.

Check https://codepen.io/mszylkowski/pen/eYWMdPe

@processprocess
Copy link
Contributor

@processprocess does not work, since combining the rules will make both browsers fail to parse the selector.

Check https://codepen.io/mszylkowski/pen/eYWMdPe

That's so crazy! Thanks for making the demo and explaining.
Please leave a comment in the code before merging so we don't accidentally try to refactor it in the future!

@mszylkowski mszylkowski merged commit 2d0c097 into ampproject:main Jul 28, 2021
wg-stories Sprint automation moved this from In progress to Done Jul 28, 2021
@mszylkowski mszylkowski deleted the slider_firefox branch July 28, 2021 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants