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

Fixing IE bug causing active element to be null causing render function to error #1943

Merged
merged 3 commits into from Aug 30, 2017

Conversation

Projects
None yet
4 participants
@JacksonJN
Copy link

JacksonJN commented Aug 22, 2017

Description

There is an issue in IE where document.activeElement becomes null sometimes. For example when you use removeChild to remove an element, the active element isn't reset. And sometimes in iframes.

When this happens using mithril, the render function throws an error on line 633.

This is an issue that has come up in other frameworks, for example in Angular.

Motivation and Context

In my case, I have a click event that removes an element that has an onremove function. That onremove function uses removeChild to remove the dom element. The same click event creates an element that runs .focus() inside a oncreate function. This causes the if statement on line 633 to pass and active.focus() to run, during which an error is caused by active being null.

How Has This Been Tested?

Confirmed this does not break any tests and does not cause any lint errors. I'm not sure what a new test would look like since activeElement can't be set.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated docs/change-log.md

@JacksonJN JacksonJN requested review from isiahmeadows and tivac as code owners Aug 22, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Aug 22, 2017

@JacksonJN thanks for the report/PR, are you sure that it is always null, never undefined?

Also, updating the mocks for having activeElement to optionally return null seems overkill, but the fix could be lost as well, could you add a comment along these lines?

// document.activeElement can return `null` in IE https://developer.mozilla.org/en-US/docs/Web/API/Document/activeElement
@JacksonJN

This comment has been minimized.

Copy link

JacksonJN commented Aug 22, 2017

@pygy I am pretty sure it is only null, based on the documentation and the instances of the bug that I've seen. Although not 100%. I could change it to check all falsy values:

if (active && $doc.activeElement !== active) active.focus()

Also I have added an inline comment about the bug.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Aug 22, 2017

@JacksonJN thanks :-)

A weak null check (!= null) covers both null and undefined and is faster than relying on truthiness.

@JacksonJN

This comment has been minimized.

Copy link

JacksonJN commented Aug 22, 2017

@pygy Done.

Also I just realized this should probably be released with 1.1.4 and not 2.0.0. Should I change the merge-to-branch to v1_1_x?

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Aug 22, 2017

@JacksonJN LGTM

Regarding the branch, I must admit I have little experience with gitflow-like setups and I don't know if one is preferable to the other... We'll have to cherry pick the fix to the other branch anyway...

@tivac @isiahmeadows @lhorie any preference here (and in general)?

@tivac

This comment has been minimized.

Copy link
Member

tivac commented Aug 22, 2017

I'd target the change to v1_1_x. Should probably open a new PR after that for porting to next?

There isn't a good answer for this flow that I'm aware of unless we want to try doing rebasing, which seems likely to end in failure/disappointment.

@tivac

tivac approved these changes Aug 22, 2017

Copy link
Member

tivac left a comment

Looks good to me! 👍

@isiahmeadows isiahmeadows removed their request for review Aug 23, 2017

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Aug 23, 2017

@pygy I don't care what we choose to do as long as it's not too complicated and it generally works. I've been time-limited lately, so it's not likely to affect me much.

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Aug 23, 2017

@tivac I'd rather merge this into next then because I don't think merging the patch into another branch would add @JacksonJN to the contributors lists (unless they do another PR).

@JacksonJN JacksonJN force-pushed the JacksonJN:fix-null-acitive-element branch from 4965aa9 to 5303e70 Aug 29, 2017

@JacksonJN

This comment has been minimized.

Copy link

JacksonJN commented Aug 29, 2017

I rebased and fixed merge conflicts. Is this ready to merge?

@isiahmeadows isiahmeadows merged commit 11183f3 into MithrilJS:next Aug 30, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Aug 30, 2017

@pygy Ready for v1 backport!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment