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

Checkbox icon hidden in Safari #205

Closed
amsheehan opened this issue Jan 24, 2017 · 4 comments
Closed

Checkbox icon hidden in Safari #205

amsheehan opened this issue Jan 24, 2017 · 4 comments
Assignees

Comments

@amsheehan
Copy link
Contributor

amsheehan commented Jan 24, 2017

What MDC-Web Version are you using?

0.3.0

What browser(s) is this bug affecting?

Desktop Safari

What OS are you using?

macOS 10.12.2

What are the steps to reproduce the bug?

  1. run the demo server
  2. go to http://material-components-web.appspot.com/checkbox.html in Safari
  3. click once to select checkbox, click again to deselect
  4. subsequent clicks will show a filled in checkbox with no icon

What is the expected behavior?

check or indeterminate icons will be displayed

What is the actual behavior?

checkbox fills in with primary color, no icon is displayed

@traviskaufman
Copy link
Contributor

This is happening for our JS checkbox, but not for our regular checkboxes. Looks like it could be a regression of some sort, but definitely a bug. We should prioritize this.

@traviskaufman traviskaufman self-assigned this Jan 26, 2017
traviskaufman added a commit that referenced this issue Jan 26, 2017
Fixes issue where wrong event type was being used within
`getCorrectEventName` in `register`/`deregisterAnimationEndHandler`
adapter methods in vanilla component. Discovered while working on #205.
@traviskaufman
Copy link
Contributor

traviskaufman commented Jan 26, 2017

spent time on this today. It's proving extremely difficult to debug animations in Safari.

Looking at the inspector, everything basically looks correct. The styles inspector shows the styles which should be applied, but are not. Manually toggling and un-toggling the styles causes the element to look correct again 🤦‍♂️ 🤦‍♀️ 🌎 💥

The first thing I noticed is that in our adapter, we're not attaching the correct animation events for getCorrectEventName; we're passing animation as the arg rather than animationend. I fixed this (#220), but unfortunately it did not solve the problem.

I tried following the advice in this stackoverflow post regarding enabling Safari's internal debug menu. I did that and played around with it and couldn't seem to identify any of the issues.

I will keep experimenting to see if I can find some way of figuring out why this isn't working, but unfortunately it does look like a browser issue. Checkboxes work fine in every other browser. I see three immediate options:

  1. Try toggling certain style properties back and forth within our animation end handler to see if we can force safari into applying the proper styles.
  2. Feature-detecting Safari - yet again - and disabling enhanced animations when detected.
  3. Change the transition UX of the checkbox to be more Safari-friendly

All of these are suboptimal, but there's no way around dealing with browser inconsistencies.

@traviskaufman
Copy link
Contributor

traviskaufman commented Jan 30, 2017

Doesn't work in Mobile Safari either. Will try and git bisect to see if there was a change that broke it. Otherwise, it may have been an OS update that caused the breakage, in which case we're in trouble...we can try creating a simple repro of the issue and file a bug with webkit.

@traviskaufman
Copy link
Contributor

Other option could be to use an mdc-checkbox-upgraded class, remove all default styling from checkboxes, and use a forwards animation-fill-mode property.

traviskaufman added a commit that referenced this issue Jan 30, 2017
Fixes issue where wrong event type was being used within
`getCorrectEventName` in `register`/`deregisterAnimationEndHandler`
adapter methods in vanilla component. Discovered while working on #205.
traviskaufman added a commit that referenced this issue Feb 10, 2017
- Adds a mdc-checkbox--upgraded mod class which is attached by the
  foundation when a JS checkbox is used.
- Disables all transitions when the animation classes are used. Fixes an
  issue in Safari where the the paint/compositing looked janky/broken
  due to animations and transitions conflicting with one another.
- (tech debt) Removed template strings from cssClasses object. This will
  be required in order for our internal infra to work correctly.

Fixes #205
traviskaufman added a commit that referenced this issue Feb 10, 2017
- Adds a mdc-checkbox--upgraded mod class which is attached by the
  foundation when a JS checkbox is used.
- Disables all transitions when the animation classes are used. Fixes an
  issue in Safari where the the paint/compositing looked janky/broken
  due to animations and transitions conflicting with one another.
- (tech debt) Removed template strings from cssClasses object. This will
  be required in order for our internal infra to work correctly.

Fixes #205
traviskaufman added a commit that referenced this issue Feb 13, 2017
…ses (#285)

- Adds a mdc-checkbox--upgraded mod class which is attached by the
  foundation when a JS checkbox is used.
- Disables all transitions when the animation classes are used. Fixes an
  issue in Safari where the the paint/compositing looked janky/broken
  due to animations and transitions conflicting with one another.
- (tech debt) Removed template strings from cssClasses object. This will
  be required in order for our internal infra to work correctly.

Fixes #205
cristobalchao pushed a commit that referenced this issue Feb 24, 2017
…ses (#285)

- Adds a mdc-checkbox--upgraded mod class which is attached by the
  foundation when a JS checkbox is used.
- Disables all transitions when the animation classes are used. Fixes an
  issue in Safari where the the paint/compositing looked janky/broken
  due to animations and transitions conflicting with one another.
- (tech debt) Removed template strings from cssClasses object. This will
  be required in order for our internal infra to work correctly.

Fixes #205
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants