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(input): horizontal overflow in IE and Edge #2784

Merged
merged 8 commits into from
Feb 9, 2017

Conversation

crisbeto
Copy link
Member

Currently input labels are set to 133% width and are scaled down to fit into their container. This works fine in most browsers, however it causes horizontal overflows on IE and Edge, because they don't consider the transform when determining whether an element overflows. This change fixes it by hiding all of the overflowing content.

@mmalerba I didn't notice any issues when going through the input container demo page, but do you think that this may have some unexpected side-effects?

Currently input labels are set to 133% width and are scaled down to fit into their container. This works fine in most browsers, however it causes horizontal overflows on IE and Edge, because they don't consider the `transform` when determining whether an element overflows. This change fixes it by hiding all of the overflowing content.
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 24, 2017
@mmalerba
Copy link
Contributor

In most cases this will be fine, but users are allowed to put custom content inside the md-input-container and if they try to put some absolutely positioned thing that intentionally overflows, this would clip it. Could we instead put a wrapper around the label and just clip that?

@crisbeto
Copy link
Member Author

That should work as well. I'll give it a try.

@crisbeto
Copy link
Member Author

crisbeto commented Jan 25, 2017

If it's just a div with overflow: hidden, it doesn't help since the label's dimensions are based on the closest element that's relative. The other approach that I tried was having a wrapper that is position: absolute instead, but that ends up clipping the label when the input is focused.

This approach may be the simplest one without a lot of refactoring.

@mmalerba
Copy link
Contributor

@jelbourn how do you feel about this?

@mmalerba
Copy link
Contributor

@crisbeto Just to make sure I'm looking at the same bug, are you referring to the fact that when it's animating the placeholder overflows? because it looks like after it finishes animating its not a problem. how do you feel about this as an alternate solution? master...mmalerba:input-overflow

@crisbeto
Copy link
Member Author

That works pretty well @mmalerba, although I'm not a huge fan of the  . I tried working around it with a pseudo element, but it didn't work. I can play around with it tomorrow.

@crisbeto
Copy link
Member Author

@mmalerba I re-did it, based on your changes. I worked around the   with a pseudo element so we can keep everything inside the CSS. Could you re-review?


// Keeps the element height since the placeholder text is `position: absolute`.
&::after {
content: '\\00a0';
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if its worth making this a separate class, seems potentially useful in other circumstances e.g. cdk-no-collapse or something

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, not sure about it. In this case it's useful, but in others where there may be content in the element, it may end up pushing it.

@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Jan 31, 2017
@kara kara added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Feb 4, 2017
@kara
Copy link
Contributor

kara commented Feb 4, 2017

@crisbeto Can you rebase?

# Conflicts:
#	src/lib/input/input-container.html
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Feb 4, 2017
# Conflicts:
#	src/lib/input/input-container.html
@kara kara added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Feb 9, 2017
@kara
Copy link
Contributor

kara commented Feb 9, 2017

@crisbeto Rebase again? Sorry

# Conflicts:
#	src/lib/input/input-container.html
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Feb 9, 2017
@tinayuangao tinayuangao merged commit e0fe635 into angular:master Feb 9, 2017
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants