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

Fix <RichTextInput> not working on IE11 #2676

Merged
merged 1 commit into from Jan 12, 2019
Merged

Conversation

phacks
Copy link
Contributor

@phacks phacks commented Dec 18, 2018

Fixes #2675

The underlying problem is in the css-vendor library, specifically in the cssVendor.supportedValue method that breaks on IE11 with content and '' as parameters.

the content: '' attribute is currently not working on Chrome either.

@Kmaschta
Copy link
Contributor

Are you sure this change doesn't affect the display of the component?
I would use content: ' ' with a space, but I'm not sure.

@phacks
Copy link
Contributor Author

phacks commented Dec 18, 2018

@Kmaschta My guess is that since the property is considered as invalid in Chrome (see screenshot), it currently has no effect on the display. I can however try and screenshot multiple situations in Chrome and see if each of them works in IE11:

  • current;
  • without the content (the suggested PR);
  • with a proper content: ' '

I may have some time today, otherwise tomorrow

image

@Kmaschta Kmaschta added this to the 2.5.3 milestone Dec 18, 2018
@Kmaschta
Copy link
Contributor

Nvm you're right, if it doesn't work on Chrome and it breaks IE, this make no sense to keep it.

@fzaninotto
Copy link
Member

The empty content was added in aad754a, probably for a good reason. Removing the content field in the CSS style probably removes the entire ::before and ::after styles (see https://stackoverflow.com/questions/17067918/why-do-the-before-and-after-pseudo-elements-require-a-content-property), so I don't think this patch is harmless.

@phacks
Copy link
Contributor Author

phacks commented Dec 19, 2018

@fzaninotto Good catch! I’ll do a more thorough review of any impacts (and/or try to set the content to ' ' instead of '') and will get back to you with what I found.

@fzaninotto fzaninotto removed this from the 2.5.3 milestone Jan 2, 2019
Fixes marmelab#2675

The underlying problem is in the [css-vendor](https://github.com/cssinjs/css-vendor) library, specifically in the cssVendor.supportedValue method that breaks on IE11 with content and '' as parameters.

the `content: ''` attribute is currently not working on Chrome either.
@phacks
Copy link
Contributor Author

phacks commented Jan 12, 2019

@fzaninotto Hi, and happy new year 🥳

Found a way better approach to solving this: as per cssinjs/jss#866 (JSS is a dependency of MaterialUI), the property content: '' is invalid (which explains why it wasn’t properly parsed by Chrome).

To translate the proper CSS content: '' in JSS, one must specify content: '""'. I updated the PR to reflect this.

Works like a charm now! However, there still is an issue on the layout (long text requires horizontal scrolling), which I’ll try to fix in a future PR (see screenshot):

image

Thanks!

@phacks
Copy link
Contributor Author

phacks commented Jan 12, 2019

Note : this fix has the side effect of repairing the “selected” visual hints for the input.

Before:

jan-12-2019 16-15-07

After:

jan-12-2019 16-14-28

Is that OK for you?

@djhi
Copy link
Contributor

djhi commented Jan 12, 2019

Thanks!

@djhi djhi added this to the 2.6.2 milestone Jan 12, 2019
@djhi djhi merged commit a3e0115 into marmelab:master Jan 12, 2019
@phacks phacks deleted the patch-1 branch January 12, 2019 22:29
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.

None yet

4 participants