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

Conversation

j3ski
Copy link
Contributor

@j3ski j3ski commented Aug 24, 2016

Check if MutationObserver is in window. If not fall back to DOMSubtreeModified event.

Closes: 9395

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@j3ski
Copy link
Contributor Author

j3ski commented Aug 24, 2016

I signed it!

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

1 similar comment
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@j3ski j3ski closed this Aug 24, 2016
@j3ski j3ski reopened this Aug 24, 2016
@j3ski j3ski changed the title fix: ie10 MutationObserver issue fix(tabs): ie10 MutationObserver issue Aug 24, 2016
@ThomasBurleson ThomasBurleson added needs: investigation The cause of this issue is not well understood and needs to be investigated by the team or community needs: manual testing This issue or PR needs to have some manual testing and verification done labels Aug 24, 2016
@ThomasBurleson ThomasBurleson added this to the 1.1.2 milestone Aug 24, 2016
observer.observe(element[0], config);
disconnect = observer.disconnect.bind(observer);
} else {
element.on('DOMSubtreeModified', mutationCallback);
Copy link
Member

Choose a reason for hiding this comment

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

@j3ski I haven't tried this out yet, but I believe that DOMSubtreeModified is deprecated and fires very often. It might be a good idea to see if this can either be switched to an Angular watcher that "only" fires once per digest, or look into debouncing the callback (You can use the $mdUtil.debounce method).

Copy link
Contributor Author

@j3ski j3ski Aug 25, 2016

Choose a reason for hiding this comment

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

@crisbeto Yes, DOMSubtreeModified is deprecated, but this is a fix for browsers which don't yet support MutationObserver. I re-implemented the functionality removed here. I don't think this can be switched to an Angular watcher, as the MutationObserver watches for subtree modifications, not attributes, but debouncing sounds like a good idea. Initially the DOMSubtreeModified would fire up to 9x more often than MutationObserver in my case. Debounced it fires roughly the same amount of times as MutationObserver (provided MO did fire), however it fires on more occasions.

@crisbeto crisbeto added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Aug 24, 2016
observer.observe(element[0], config);
disconnect = observer.disconnect.bind(observer);
} else {
var debounced = $mdUtil.debounce(mutationCallback, 10);
Copy link
Member

@crisbeto crisbeto Aug 25, 2016

Choose a reason for hiding this comment

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

This looks good, although could you change the debounce signature to $mdUtil.debounce(mutationCallback, 15, null, false)? This is because a couple of reasons:

  1. Passing in false as the last param means that it won't trigger a digest every time it gets invoked, which should help with the performance.
  2. Bumping the time is more of a nitpick, but I feel that it's something we can get away with since it's still less than 1 frame (which is ~16ms).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
Checked it, reduced the number of calls to mutationCallback by 1 in one scenario. Also the app seems to work smoother when loading the tabs, but it can be just my impression.

Copy link
Member

Choose a reason for hiding this comment

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

Great, I'll test it out manually a bit later.

@crisbeto crisbeto added needs: review This PR is waiting on review from the team and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: investigation The cause of this issue is not well understood and needs to be investigated by the team or community needs: manual testing This issue or PR needs to have some manual testing and verification done labels Aug 25, 2016
@crisbeto
Copy link
Member

crisbeto commented Aug 25, 2016

Code and functionality-wise LGTM @ThomasBurleson. The commits should be squashed though.

@crisbeto crisbeto assigned ThomasBurleson and unassigned crisbeto Aug 25, 2016
@ThomasBurleson ThomasBurleson added pr: merge ready This PR is ready for a caretaker to review and removed needs: review This PR is waiting on review from the team labels Aug 26, 2016
@hansl hansl merged commit bd70022 into angular:master Aug 31, 2016
devversion added a commit to devversion/material that referenced this pull request Sep 1, 2016
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.

5 participants