-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
Fix some iframe edge cases #13650
Fix some iframe edge cases #13650
Conversation
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! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Details of bundled changes.Comparing: 8bc0bca...ac7684b schedule
Generated by 🚫 dangerJS |
Can you verify this fixes it? Also it would really be helpful to add a case to this fixture: |
const doc = node.ownerDocument || document; | ||
const win = doc ? doc.defaultView : window; | ||
const selection = win.getSelection(); | ||
const {ownerDocument} = node; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
9a5eaed
to
ac7684b
Compare
@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? |
and yes the actual PR fixes the issue we have |
@acusti Can you sign off on the final version please? If it looks good to you I'll get it in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Thanks. |
Thank you both! |
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. |
@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. |
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.
|
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
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>
.
|
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>
> .
>
>
|
@einarq Thanks for tracking that down! If you want to make a PR that adds an early return const win = (doc && doc.defaultView) || window; will ensure that even if 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. |
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>
.
|
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>
> .
>
|
Should fix facebook#13648 by fallback on `window` when `document.defaultView` does not exists anymore
Should fix #13648 by being as protective in
setOffsets
than ingetOffsets