Skip to content
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

Support iframes (nested browsing contexts) in selection event handling #7866

Closed
wants to merge 5 commits into from

Conversation

acusti
Copy link
Contributor

@acusti acusti commented Oct 4, 2016

Fixes #427

dda228b depends on facebook/fbjs#188 to actually work. With the current version of fbjs, the doc argument will be ignored and getActiveElement will execute relative to the global document.

Had some more tests for ReactInputSelection that rely on DOM functionality that jsdom doesn’t yet support. Includes this block, for setSelection, but requires a working getSelection() implementation (see jsdom/jsdom#937):

    it('sets selection offsets on a contentEditable element', () => {
      var node = createAndMountElement('div', null, textValue);
      ReactInputSelection.setSelection(node, {start: 1, end: 10});
      expect(ReactInputSelection.getSelection(node)).toEqual({start: 1, end: 10});
    });

Also, had a suite for getSelectionInformation/restoreSelection for a focused element inside an iframe, but it requires jsdom to have accurate activeElement functionality with iframes. I made a PR to fix that part of jsdom (jsdom/jsdom#1621), so hopefully when that functionality gets added, I can add these tests in:

    it('gets and restores selection for inputs in an iframe that get remounted', () => {
      var iframe = document.createElement('iframe');
      document.body.appendChild(iframe);
      iframe.focus();
      var iframeDoc = iframe.contentDocument;
      var input = document.createElement('input');
      input.value = textValue;
      iframeDoc.body.appendChild(input);
      input.focus();
      input.selectionStart = 1;
      input.selectionEnd = 10;
      var selectionInfo = ReactInputSelection.getSelectionInformation();
      expect(selectionInfo.focusedElem).toBe(input);
      expect(selectionInfo.selectionRange).toEqual({start: 1, end: 10});
      expect(document.activeElement).toBe(iframe);
      expect(iframeDoc.activeElement).toBe(input);

      input.setSelectionRange(0, 0);
      iframeDoc.body.removeChild(input);
      expect(iframeDoc.activeElement).not.toBe(input);
      expect(input.selectionStart).not.toBe(1);
      expect(input.selectionEnd).not.toBe(10);
      iframeDoc.body.appendChild(input);
      ReactInputSelection.restoreSelection(selectionInfo);
      expect(iframeDoc.activeElement).toBe(input);
      expect(input.selectionStart).toBe(1);
      expect(input.selectionEnd).toBe(10);

      document.body.removeChild(iframe);
    });

If an input inside a same-domain iframe is the actual activeElement,
this change will make the ReactInputSelection module get from and
restore selection to that input, rather than the iframe element itself
Had to remove some tests because they rely on DOM functionality that
jsdom doesn’t yet support, like getSelection and iframe activeElement
@acusti
Copy link
Contributor Author

acusti commented Oct 6, 2016

I created a demo of a simple use case to illustrate the utility of this PR. I then duplicated the demo and have one using the current react and react-dom builds and one using built versions of those files made from this branch.

Without the changes in this branch: http://codepen.io/acusti/pen/kkBpWA?editors=0011
With the changes in this branch: http://codepen.io/acusti/pen/rrpKYZ?editors=0011

@acusti
Copy link
Contributor Author

acusti commented Oct 6, 2016

One issue that remains is that the selection in the textarea inside the iframe is lost after it gets rerendered with a changed value prop. Not sure if getSelectionInformation/restoreSelection can / should be further modified to apply to any iframes, not just the current focused one (in the demo, when you focus the text input that is outside of the iframe, the iframe is of course no longer the activeElement).

@acusti
Copy link
Contributor Author

acusti commented Oct 6, 2016

I updated the demo to include logic for maintaining the selectionStart and selectionEnd on the textarea whenever its value gets changed when it doesn’t have focus and it seems to work ok. Visually, you still lose the selection, but when you tab to change focus into the textarea, you get the correct selection back.

@acusti
Copy link
Contributor Author

acusti commented Oct 6, 2016

I made another demo that only changes the textarea value in the iframe on enter or blur and manually restores selection then focuses the iframe when appropriate (much like ReactInputSelection.restoreSelection).

Without the changes in this branch: http://codepen.io/acusti/pen/EgQxJG?editors=0011
With the changes in this branch + the React PR: http://codepen.io/acusti/pen/qaxBby?editors=0011

Makes me think that if ReactInputSelection.restoreSelection were updated to get and restore selection for all nested browsing contexts it had access to, it would be nice. I could make a build that does that to see how well it works, if that would make sense.

@acusti
Copy link
Contributor Author

acusti commented Oct 7, 2016

Continuing this ongoing conversation with myself, above I wrote that:

One issue that remains is that the selection in the textarea inside the iframe is lost after it gets rerendered with a changed value prop. Not sure if getSelectionInformation/restoreSelection can / should be further modified to apply to any iframes, not just the current focused one.

I’ve since figured out how focus events work in the browser across browsing contexts and made a new demo that properly restores focus as you go: http://codepen.io/acusti/pen/JRpBWv?editors=0011

It’s seamless and makes for a really nice UX, plus it uses native browser functionality so has full a11y. This made me realize that in order to make ReactInputSelection properly restoreSelection even with iframes being rendered, it should in fact be modified to apply to any iframes, not just the primary focused one. I’ll publish a branch that does that, make a build from it, and use it in the above demo without the custom focus management I added (lines 82 and 111-116) to see how well it works.

@acusti
Copy link
Contributor Author

acusti commented Oct 11, 2016

The new PR which gets and restores selections for any iframes, not just the primary focused one, is #7936

@acusti
Copy link
Contributor Author

acusti commented Oct 13, 2016

I’m thinking I should close this in favor of #7936, because that implementation seems more complete to me based on the current behavior of ReactInputSelection, but it’s hard for me to know what people would be interested in without hearing from anyone. Any thoughts, comments, opinions? Should I close this one? Close #7936?

mihaip added a commit to quip/react that referenced this pull request Jun 29, 2017
React generally handles being rendered into another window context correctly
(we have been doing this for a while in native Mac popovers).

The main place where there are global window/document accesses are in places
where we deal with the DOM selection (window.getSelection() and
document.activeElement). There has been some discussion about this in the
public React GitHub repo:

facebook/fbjs#188
facebook#7866
facebook#7936
facebook#9184

While this was a good starting point, those proposed changes did not go far
enough, since they assumed that React was executing in the top-most window,
and the focus was in a child frame (in the same origin). Thus for them it was
possible to check document.activeElement in the top window, find which iframe
had focus and then recurse into it. In our case, the controller and view frames
are siblings, and the top window is in another origin, so we can't use that code
path.

The main reason why we can't get the current window/document is that
ReactInputSelection runs as a transaction wrapper, which doesn't have access
to components or DOM nodes (and may run across multiple nodes). To work around
this I added a ReactLastActiveThing which keeps track of the last DOM node that
we mounted a component into (for the initial render) or the last component that
we updated (for re-renders). It's kind of gross, but I couldn't think of any
better alternatives.

All of the modifications are no-ops when not running inside a frame, so this
should have no impact for non-elements uses.

I did not update any of the IE8 selection API code paths, we don't support it.
mihaip added a commit to quip/react that referenced this pull request Sep 29, 2017
React generally handles being rendered into another window context correctly
(we have been doing this for a while in native Mac popovers).

The main place where there are global window/document accesses are in places
where we deal with the DOM selection (window.getSelection() and
document.activeElement). There has been some discussion about this in the
public React GitHub repo:

facebook/fbjs#188
facebook#7866
facebook#7936
facebook#9184

While this was a good starting point, those proposed changes did not go far
enough, since they assumed that React was executing in the top-most window,
and the focus was in a child frame (in the same origin). Thus for them it was
possible to check document.activeElement in the top window, find which iframe
had focus and then recurse into it. In our case, the controller and view frames
are siblings, and the top window is in another origin, so we can't use that code
path.

The main reason why we can't get the current window/document is that
ReactInputSelection runs as a transaction wrapper, which doesn't have access
to components or DOM nodes (and may run across multiple nodes). To work around
this I added a ReactLastActiveThing which keeps track of the last DOM node that
we mounted a component into (for the initial render) or the last component that
we updated (for re-renders). It's kind of gross, but I couldn't think of any
better alternatives.

All of the modifications are no-ops when not running inside a frame, so this
should have no impact for non-elements uses.

I did not update any of the IE8 selection API code paths, we don't support it.

(cherry picked from commit 94b759b in
the 0.14-stable. Appears to work mostly as is, needed to be updated to
take 5c5d2ec into account)
@acusti acusti closed this Oct 31, 2017
@acusti
Copy link
Contributor Author

acusti commented Oct 31, 2017

Closing in favor of #11410

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use the right document in iframe selection events
3 participants