Fix some iframe edge cases #13650

JSteunou · 2018-09-14T13:26:05Z

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

facebook-github-bot · 2018-09-14T13:26:11Z

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 [email protected]. Thanks!

facebook-github-bot · 2018-09-14T13:30:05Z

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

sizebot · 2018-09-14T13:38:14Z

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 · 2018-09-14T14:00:38Z

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 · 2018-09-14T14:10:00Z

packages/react-dom/src/client/ReactDOMSelection.js

-  const doc = node.ownerDocument || document;
-  const win = doc ? doc.defaultView : window;
-  const selection = win.getSelection();
+  const {ownerDocument} = node;


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.

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

Update incoming

Should fix facebook#13648 by fallback on `window` when `document.defaultView` does not exists anymore

JSteunou · 2018-09-14T14:23:30Z

@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 · 2018-09-14T14:36:08Z

and yes the actual PR fixes the issue we have

gaearon · 2018-09-14T14:50:17Z

@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 · 2018-09-14T15:44:17Z

Thanks.

JSteunou · 2018-09-14T21:17:00Z

Thank you both!

einarq · 2018-10-20T07:32:09Z

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 · 2018-10-20T19:19:32Z

@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 · 2018-10-20T20:59:11Z

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 · 2018-10-20T21:27:20Z

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.

Should fix facebook#13648 by fallback on `window` when `document.defaultView` does not exists anymore

einarq · 2018-11-03T20:09:32Z

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 < [email protected]> 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 · 2018-11-03T20:15:07Z

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 <[email protected]> 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 · 2018-11-04T16:33:24Z

@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 · 2018-11-04T17:27:20Z

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 · 2018-11-05T10:32:24Z

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 <[email protected]> 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> > . >

Should fix facebook#13648 by fallback on `window` when `document.defaultView` does not exists anymore

facebook-github-bot added the CLA Signed label Sep 14, 2018

klamping mentioned this pull request Sep 14, 2018

React PR Test Report Listing klamping/react-webdriverio-tests#1

Open

acusti suggested changes Sep 14, 2018

View reviewed changes

Fix some iframe edge cases

ac7684b

Should fix facebook#13648 by fallback on `window` when `document.defaultView` does not exists anymore

JSteunou force-pushed the path-13648 branch from 9a5eaed to ac7684b Compare September 14, 2018 14:19

acusti approved these changes Sep 14, 2018

View reviewed changes

gaearon merged commit 9c961c0 into facebook:master Sep 14, 2018

dependencies bot mentioned this pull request Oct 1, 2018

Update react-dom in /modules/frontend from 16.3.0 to 16.5.2 project-accelerate/accelerate#277

Open

ilias-t mentioned this pull request Oct 1, 2018

Handle possible null return value for document.defaultValue #13757

Closed

Simek pushed a commit to Simek/react that referenced this pull request Oct 25, 2018

Fix some iframe edge cases (facebook#13650)

e86fefe

Should fix facebook#13648 by fallback on `window` when `document.defaultView` does not exists anymore

This was referenced Nov 1, 2018

Update react-dom in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#253

Closed

Update react in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#254

Closed

einarq mentioned this pull request Nov 5, 2018

Add back early return in setOffsets which was removed in 16.5.0 #14095

Merged

This was referenced Dec 1, 2018

Update react-test-renderer in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#312

Open

Update react in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#313

Open

jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019

Fix some iframe edge cases (facebook#13650)

82062fb

Should fix facebook#13648 by fallback on `window` when `document.defaultView` does not exists anymore

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some iframe edge cases #13650

Fix some iframe edge cases #13650

JSteunou commented Sep 14, 2018

facebook-github-bot commented Sep 14, 2018

facebook-github-bot commented Sep 14, 2018

sizebot commented Sep 14, 2018 •

edited

Loading

schedule

gaearon commented Sep 14, 2018

acusti Sep 14, 2018 •

edited

Loading

JSteunou Sep 14, 2018

JSteunou commented Sep 14, 2018

JSteunou commented Sep 14, 2018 •

edited

Loading

gaearon commented Sep 14, 2018

acusti left a comment

gaearon commented Sep 14, 2018

JSteunou commented Sep 14, 2018

einarq commented Oct 20, 2018

acusti commented Oct 20, 2018 •

edited

Loading

einarq commented Oct 20, 2018 via email

einarq commented Oct 20, 2018 via email

einarq commented Nov 3, 2018 via email

einarq commented Nov 3, 2018 via email

acusti commented Nov 4, 2018

einarq commented Nov 4, 2018 via email

einarq commented Nov 5, 2018 via email

Fix some iframe edge cases #13650

Fix some iframe edge cases #13650

Conversation

JSteunou commented Sep 14, 2018

facebook-github-bot commented Sep 14, 2018

facebook-github-bot commented Sep 14, 2018

sizebot commented Sep 14, 2018 • edited Loading

schedule

gaearon commented Sep 14, 2018

acusti Sep 14, 2018 • edited Loading

Choose a reason for hiding this comment

JSteunou Sep 14, 2018

Choose a reason for hiding this comment

JSteunou commented Sep 14, 2018

JSteunou commented Sep 14, 2018 • edited Loading

gaearon commented Sep 14, 2018

acusti left a comment

Choose a reason for hiding this comment

gaearon commented Sep 14, 2018

JSteunou commented Sep 14, 2018

einarq commented Oct 20, 2018

acusti commented Oct 20, 2018 • edited Loading

einarq commented Oct 20, 2018 via email

einarq commented Oct 20, 2018 via email

einarq commented Nov 3, 2018 via email

einarq commented Nov 3, 2018 via email

acusti commented Nov 4, 2018

einarq commented Nov 4, 2018 via email

einarq commented Nov 5, 2018 via email

sizebot commented Sep 14, 2018 •

edited

Loading

acusti Sep 14, 2018 •

edited

Loading

JSteunou commented Sep 14, 2018 •

edited

Loading

acusti commented Oct 20, 2018 •

edited

Loading