fix(form-field): incorrect assumptions about page direction #17395
+13
−17
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
crisbeto
added
P3
googlebot
added
the
cla: yes
These changes undo the changes from angular#15415 because they were making incorrect assumptions about the page starting off as LTR, and they were calling `updateOutlineGap` with an outdated value for the direction. This is causing issues like angular#17390. I'm not totally sure what was being fixed by these changes, because the test that was added didn't fail even if I reverted all of the changes. From what I can tell the problem angular#15415 was trying to solve was that the outline gap might be updated too early on a direction change, before the browser has had the chance to recalculate the layout. These changes switch to recalculating inside a `requestAnimationFrame` after direction changes, in order to give the browser enough time to recalculate the layout. Fixes angular#17390.
crisbeto
force-pushed
the
form-field-dir-change
branch
from
0221ca5 to
f759ca1
Compare
mmalerba
reviewed
Oct 14, 2019
| this.updateOutlineGap(); | ||
| this._previousDirection = this._dir.value; | ||
| if (typeof requestAnimationFrame === 'function') { | ||
| this._ngZone.runOutsideAngular(() => { |
There was a problem hiding this comment.
Shouldn't this whole if-else be wrapped in runOutsideAngular? Currently it looks like it runs in the zone if the browser doesn't support requestAnimationFrame and outside if it does
There was a problem hiding this comment.
updateOutlineGap doesn't do any zone-ey things, we only need to run requestAnimationFrame on the outside because it can trigger change detection otherwise.
mmalerba
added
pr: lgtm
action: merge
mmalerba
approved these changes
Oct 15, 2019
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These changes undo the changes from #15415 because they were making incorrect assumptions about the page starting off as LTR, and they were calling
updateOutlineGapwith an outdated value for the direction. This is causing issues like #17390.I'm not totally sure what was being fixed by these changes, because the test that was added didn't fail even if I reverted all of the changes. From what I can tell the problem #15415 was trying to solve was that the outline gap might be updated too early on a direction change, before the browser has had the chance to recalculate the layout. These changes switch to recalculating inside a
requestAnimationFrameafter direction changes, in order to give the browser enough time to recalculate.Fixes #17390.