Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

fix(textarea): scrolling, text selection, reduced DOM manipulation. #7553

Closed
wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

  • Fixes some jumping when the textarea is being resized.
  • Fixes mdSelectOnFocus not working in Edge and being unreliable in Firefox.
  • Fixes textarea not being scrollable once it is past it's minimum number of rows.
  • Fixes textarea not being scrollable if mdNoAutogrow is specified.
  • Tries to reduce the number of event listeners and the amount of DOM manipulation when resizing the textarea.

Closes #7487.

@crisbeto crisbeto added the needs: review This PR is waiting on review from the team label Mar 13, 2016
// This is necessary, because browsers fire a `mouseup` right after the element
// has been focused. In some browsers (Firefox in particular) this can clear the
// selection. There are examples of the problem in issue #7487.
element.on('mouseup', preventMouseUp);
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make more sense to keep the mouseup event registered and just change a state in the directive?. Adding / Removing events is more harmful, than just an empty event handler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nevermind, we can keep track of it in the mouseup, I'll give it a try.
That would mean adding another handler for blur, though.

element[0].select();
// Use HTMLInputElement#select to fix firefox select issues.
// The debounce is here for Edge's sake, otherwise the selection doesn't work.
preventMouseUp = true;
Copy link
Member

Choose a reason for hiding this comment

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

What happens if I focus the element using keyboard interaction? The mouseup event can't be triggered.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, it actually prevents the first click afterwards. Not sure how we could distinguish between the two though.

Copy link
Member

Choose a reason for hiding this comment

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

When is the mouseup event usually called? Before the 1ms delay? I've got no time, to test that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have it running atm either, but from what I remember it's afterwards. I'll take a closer look at it later.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's good now with resetting it from inside the $timeout. Also had time to test it out in most browsers.

Copy link
Member

Choose a reason for hiding this comment

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

That's exactly what I've thought about. Great 👍

@devversion devversion added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Mar 14, 2016
* Fixes `mdSelectOnFocus` not working in Edge and being unreliable in Firefox.
* Fixes `textarea` not being scrollable once it is past it's minimum number of rows.
* Fixes `textarea` not being scrollable if `mdNoAutogrow` is specified.
* Tries to reduce the number of event listeners and the amount of DOM manipulation when resizing the `textarea`.

Closes angular#7487.
@crisbeto crisbeto added pr: merge ready This PR is ready for a caretaker to review and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: review This PR is waiting on review from the team labels Mar 14, 2016
onChangeTextarea();
return value;
}
var minRows = attr.hasOwnProperty('rows') ? parseInt(attr.rows) : NaN;
Copy link
Member

Choose a reason for hiding this comment

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

How can I now let the textarea grow?

Copy link
Member Author

Choose a reason for hiding this comment

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

You don't specify the rows. It works in the same way as before, I just made it a one-liner.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, just rechecked with the latest version. Everything works fine 👍

@crisbeto crisbeto deleted the fix/textarea-grow branch March 14, 2016 18:05
ThomasBurleson pushed a commit that referenced this pull request Mar 16, 2016
* Fixes `mdSelectOnFocus` not working in Edge and being unreliable in Firefox.
* Fixes `textarea` not being scrollable once it is past it's minimum number of rows.
* Fixes `textarea` not being scrollable if `mdNoAutogrow` is specified.
* Tries to reduce the number of event listeners and the amount of DOM manipulation when resizing the `textarea`.

Fixes #7487. Closes #7553
@dkirsanov
Copy link

dkirsanov commented May 5, 2017

Hope that PR solves that issue too:
#10301

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr: merge ready This PR is ready for a caretaker to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants