Fix some iframe edge cases

[JSteunou]

Should fix #13648 by being as protective in setOffsets than in getOffsets

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[sizebot]

Details of bundled changes.

Comparing: 8bc0bca...ac7684b

schedule

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
schedule.development.js	n/a	n/a	0 B	19.17 KB	0 B	5.74 KB	UMD_DEV
schedule.production.min.js	n/a	n/a	0 B	3.16 KB	0 B	1.53 KB	UMD_PROD

Generated by 🚫 dangerJS

[gaearon]

Can you verify this fixes it? Also it would really be helpful to add a case to this fixture:

https://github.com/facebook/react/tree/master/fixtures/dom/src/components/fixtures/selection-events

[acusti]

Thanks for tracking this down! This is definitely an oversight of #12037 that we didn’t address.

For context, the reason that getOffsets uses a different method to get a relative window and document object than setOffsets is that getOffsets only needs to use the relative window (which has getSelections), whereas setOffsets needs to use both the window object (for getSelection, again), and the document object (for createRange). So this should really be a hybrid of the two approaches, i.e.:

  const doc = node.ownerDocument || document;
  const win = (doc && doc.defaultView) || window;
  ...
  const range = doc.createRange();

Where it still guards against a missing ownerDocument by falling back to the global and also guards against the case of having an ownerDocument that’s missing a defaultView.

[JSteunou]

I understand, I changed the logic a bit by over-protecting this path

Update incoming

[JSteunou]

@gaearon I have difficulties to reproduce this bug oustide our environment. It's a complicated situation involving a css transition between a state having a wysiwyg jquery editor inside an iframe inside a component and another simple state without this iframe.

Could someone guide me to add a test case?

[JSteunou]

and yes the actual PR fixes the issue we have

[gaearon]

@acusti Can you sign off on the final version please? If it looks good to you I'll get it in.

[acusti]

Looks great!

[gaearon]

Thanks.

[JSteunou]

Thank you both!

[einarq]

Was this tested in Edge? I saw a bug related to pasting stuff in a TinyMCE editor when using 16.5.1, upgrading to 16.5.2 solved it. But it is still failing in Edge. The issue might be related to something entirely different of course, but it goes away if I downgrade to 16.4.2.
The error coming from TinyMCE is "Unable to get property 'moveToBookmark' of undefined or null reference", not sure if that helps at all.

[acusti]

@einarq Can you post a link to a repo or even just a code sample that makes it possible to reproduce the error on Edge? I’m most interested in how you are including and rendering the TinyMCE editor, but anything that will help me to reproduce it would be helpful. If the error is coming from TinyMCE itself, I’m not sure how it’s related to the changes between 16.4.2 and 16.5.x, but I would be happy to take a look to try to figure it out.

[einarq]

I’ll see if I can create a repro and provide some more information next week, but I was just mainly wondering if the fix for the issue was tested in Edge (assuming there was a repro for the bug that was fixed).

…

On 20 Oct 2018, at 21:24, Andrew Patton ***@***.***> wrote: @einarq Can you post a link to a repo or even just a code sample that makes it possible to reproduce the error on Edge? I’m most interested in how you are including and rendering the TinyMCE editor, but anything that will help me to reproduce it. If the error is coming from TinyMCE itself, I’m not sure how it’s related to the changes between 16.4.2 and 16.5.x, but I would be happy to take a look to try to figure it out. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[einarq]

And btw, I think it’s more related to the fact that TinyMCE is using an iframe (which this issue is about) than something TinyMCE is doing specifically. We have an editable list/grid where every row contains a TinyMCE field. Pasting in a large nr of rows (we create new rows when pasting multiline content) causes an error related to “moveToBookmark” or something. With 16.5.1 it happened in every browser, with 16.5.2 only happens in Edge. 16.4.2 doesn’t have the bug at all. Sorry if this isn’t very helpful, thanks.

…

On 20 Oct 2018, at 21:24, Andrew Patton ***@***.***> wrote: @einarq Can you post a link to a repo or even just a code sample that makes it possible to reproduce the error on Edge? I’m most interested in how you are including and rendering the TinyMCE editor, but anything that will help me to reproduce it. If the error is coming from TinyMCE itself, I’m not sure how it’s related to the changes between 16.4.2 and 16.5.x, but I would be happy to take a look to try to figure it out. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[einarq]

I have traced the problem to the change in setOffsets between 16.4.2 and 16.5.0. As previously mentioned we have a TinyMCE editor in every row of a list, which supports paste to add more rows. When pasting just 5 rows, setOffsets does not get called at all, but when pasting for instance 100 rows, setOffsets gets called and fails with "Object expected". Perhaps some type of timing issue? A can try to create an example but I suspect it would be difficult due to the complexity of the scenario with TinyMCE being used etc. Could we first check if this code even works in Edge? Do you have a repro case for this bug that was fixed, and if so perhaps that could be tested with Edge? Thanks, Einar On Sat, Oct 20, 2018 at 11:27 PM Einar Paul Qvale < einar.paul.qvale@gmail.com> wrote:

…

And btw, I think it’s more related to the fact that TinyMCE is using an iframe (which this issue is about) than something TinyMCE is doing specifically. We have an editable list/grid where every row contains a TinyMCE field. Pasting in a large nr of rows (we create new rows when pasting multiline content) causes an error related to “moveToBookmark” or something. With 16.5.1 it happened in every browser, with 16.5.2 only happens in Edge. 16.4.2 doesn’t have the bug at all. Sorry if this isn’t very helpful, thanks. On 20 Oct 2018, at 21:24, Andrew Patton ***@***.***> wrote: @einarq <https://github.com/einarq> Can you post a link to a repo or even just a code sample that makes it possible to reproduce the error on Edge? I’m most interested in how you are including and rendering the TinyMCE editor, but anything that will help me to reproduce it. If the error is coming from TinyMCE itself, I’m not sure how it’s related to the changes between 16.4.2 and 16.5.x, but I would be happy to take a look to try to figure it out. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13650 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAM_D8hZ99U8uqrtzW3hFjEnxl21jx6Lks5um3iLgaJpZM4WpROM> .

[einarq]

It fails due to win.getSelection being undefined. I see that in 16.4.2 this function had an early return statement for when getSelection is undefined, could we add that back in? Changing setOffsets to this makes it work again for me: const doc = node.ownerDocument || document; const win = doc ? doc.defaultView : window; if (!win.getSelection) { return; } On Sat, Nov 3, 2018 at 9:09 PM Einar Paul Qvale <einar.paul.qvale@gmail.com> wrote:

…

I have traced the problem to the change in setOffsets between 16.4.2 and 16.5.0. As previously mentioned we have a TinyMCE editor in every row of a list, which supports paste to add more rows. When pasting just 5 rows, setOffsets does not get called at all, but when pasting for instance 100 rows, setOffsets gets called and fails with "Object expected". Perhaps some type of timing issue? A can try to create an example but I suspect it would be difficult due to the complexity of the scenario with TinyMCE being used etc. Could we first check if this code even works in Edge? Do you have a repro case for this bug that was fixed, and if so perhaps that could be tested with Edge? Thanks, Einar On Sat, Oct 20, 2018 at 11:27 PM Einar Paul Qvale < ***@***.***> wrote: > And btw, I think it’s more related to the fact that TinyMCE is using an > iframe (which this issue is about) than something TinyMCE is doing > specifically. > We have an editable list/grid where every row contains a TinyMCE field. > Pasting in a large nr of rows (we create new rows when pasting multiline > content) causes an error related to “moveToBookmark” or something. With > 16.5.1 it happened in every browser, with 16.5.2 only happens in Edge. > 16.4.2 doesn’t have the bug at all. > > Sorry if this isn’t very helpful, thanks. > > On 20 Oct 2018, at 21:24, Andrew Patton ***@***.***> wrote: > > @einarq <https://github.com/einarq> Can you post a link to a repo or > even just a code sample that makes it possible to reproduce the error on > Edge? I’m most interested in how you are including and rendering the > TinyMCE editor, but anything that will help me to reproduce it. If the > error is coming from TinyMCE itself, I’m not sure how it’s related to the > changes between 16.4.2 and 16.5.x, but I would be happy to take a look to > try to figure it out. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#13650 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAM_D8hZ99U8uqrtzW3hFjEnxl21jx6Lks5um3iLgaJpZM4WpROM> > . > >

[acusti]

@einarq Thanks for tracking that down! If you want to make a PR that adds an early return if (!win.getSelection) to the top of setOffsets with the explanation of what it addresses, I imagine that would be easy to get merged. I’m puzzled as to why it’s still necessary, considering that support for window.getSelection was added in IE9, and:

const win = (doc && doc.defaultView) || window;

will ensure that even if doc.defaultView is not a proper Window object, we’ll still get the window global, which must have the getSelection method on it. Only thing I could imagine is that doc.defaultView is present but is some kind of limited version of a Window object that doesn’t have the full APIs, maybe for cross-domain security reasons?

But again, having an early return there is harmless, so I’m fine with it, even though I don’t understand why it’s needed.

[einarq]

Ok, thanks. Just need to figure out how :)

…

On Sun, Nov 4, 2018 at 5:35 PM Andrew Patton ***@***.***> wrote: @einarq <https://github.com/einarq> Thanks for tracking that down! If you want to make a PR that adds an early return if (!win.getSelection) to the top of setOffsets with the explanation of what it addresses, I imagine that would be easy to get merged. I’m puzzled as to why it’s still necessary, considering that support for window.getSelection was added in IE9, and: const win = (doc && doc.defaultView) || window; will ensure that even if doc.defaultView is not a proper Window object, we’ll still get the window global, which must have the getSelection method on it. Only thing I could imagine is that doc.defaultView is present but is some kind of limited version of a Window object that doesn’t have the full APIs, maybe for cross-domain security reasons? But again, having an early return there is harmless, so I’m fine with it, even though I don’t understand why it’s needed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13650 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAM_D2qynuonb5G6D3kHRagSJdC_hR4Sks5urxdhgaJpZM4WpROM> .

[einarq]

Done. Uncharted territory for me (until now), so hope I did it correctly. #14095 Thanks in advance for any help in getting it merged :) Einar On Sun, Nov 4, 2018 at 6:27 PM Einar Paul Qvale <einar.paul.qvale@gmail.com> wrote:

…

Ok, thanks. Just need to figure out how :) On Sun, Nov 4, 2018 at 5:35 PM Andrew Patton ***@***.***> wrote: > @einarq <https://github.com/einarq> Thanks for tracking that down! If > you want to make a PR that adds an early return if (!win.getSelection) > to the top of setOffsets with the explanation of what it addresses, I > imagine that would be easy to get merged. I’m puzzled as to why it’s still > necessary, considering that support for window.getSelection was added in > IE9, and: > > const win = (doc && doc.defaultView) || window; > > will ensure that even if doc.defaultView is not a proper Window object, > we’ll still get the window global, which must have the getSelection > method on it. Only thing I could imagine is that doc.defaultView is > present but is some kind of limited version of a Window object that > doesn’t have the full APIs, maybe for cross-domain security reasons? > > But again, having an early return there is harmless, so I’m fine with it, > even though I don’t understand why it’s needed. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#13650 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAM_D2qynuonb5G6D3kHRagSJdC_hR4Sks5urxdhgaJpZM4WpROM> > . >