Use ownerdocument for document element

marionettejs / backbone.marionette

The Backbone Framework

https://marionettejs.com

Other

7.07k stars 1.26k forks source link

Use ownerdocument for document element #3655

Closed paulfalgout closed 5 years ago

paulfalgout commented 5 years ago

Resolves https://github.com/marionettejs/backbone.marionette/issues/3640

While working on this I found a bug where showing a view in a region with replaceElement: true that already had a view will not get the attach events because the event checks the region's el. So this also checks against the replaced view's el as a fix, but I need to add a test case to cover this logic branch.

[ ] Tests for replaceElement attached issue

paulfalgout commented 5 years ago

This PR is passing, but the gulp-coveralls is failing. https://github.com/marionettejs/backbone.marionette/issues/3656

coveralls commented 5 years ago

Coverage remained the same at 100.0% when pulling 2b6c335f56946fcad3af2e31fdd14e10c2698724 on paulfalgout:ownerdocument into bdf91fb2b5a392fc215fe53e52d9de21f2bd4756 on marionettejs:next.

paulfalgout commented 5 years ago

The bug was a false alarm. Couldn't actually get to that state because empty always resolved the el first. However while chasing that issue down I did look into the branch coverage issues mentioned here: https://github.com/marionettejs/backbone.marionette/issues/3652 I added them as a 2nd commit to this PR, but they can go in a separate PR if necessary.

I think after this v4.1 is ready to :shipit: