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

fix point transform for insert_text to account for affinity #4848

Merged
merged 2 commits into from
Feb 23, 2022

Conversation

rockettomatooo
Copy link
Contributor

Description
When one creates a RangeRef with affinity outward and the ref becomes collapsed, it can't be expanded again with an insert_text operation (although it should IMO).

Example
Lets take an example string (and not care about paths for simplicity sake)

This is a test

The RangeRef is the following

{ 
  affinity: 'outward',
  current: { 
    anchor: { offset: 4 }, 
    focus: { offset: 4 } 
  },
}

Now if the user types something at offset 4 (right after the first word) the RangeRef right now changes to the following

{ 
  affinity: 'outward',
  current: { 
    anchor: { offset: 5 }, 
    focus: { offset: 5 } 
  },
}

Although I naively expected it to be this.

{ 
  affinity: 'outward',
  current: { 
    anchor: { offset: 4 }, 
    focus: { offset: 5 } 
  },
}

Context

If you take a collapsed RangeRef with affinity outward and apply an insert_text operation at the offset of where the Range is the following happens:
Because the collapsed ranges count as forward pointing ranges, the Range.transform() will determine the anchors' affinity as backward. Therefor it will run the Point.transform() on the anchor with that affinity. But, because the Point.transform() doesn't account for the affinity, the offset of the anchor gets incremented regardless. The same happens with the focus thus the Range will stay collapsed.

Checks

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)
  • You've added a changeset if changing functionality. (Add one with yarn changeset add.)

@changeset-bot
Copy link

changeset-bot bot commented Feb 21, 2022

🦋 Changeset detected

Latest commit: f17b297

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
slate Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@rockettomatooo
Copy link
Contributor Author

@BrentFarese @dylans I'm not sure if I also need to touch remove_text operations. The use case would be when you remove something where the focus is and you want it to stay rather than shrink (if that makes any sense). I can add this as well.
But I don't think its as straight forward and I'm not sure I understand the contents of the remove_text operation enough. From what it looks like, the offset for remove_text operation is the new offset (meaning the offset after the contents was removed) whereas in an insert_text operation, the offset is the offset before the text was inserted.
Right now, the remove_text operation checks for op.offset <= offset but I'm not sure this is actually correct. Shouldn't it be op.offset + op.text.length <= offset? wdyt?

@rockettomatooo
Copy link
Contributor Author

As for the failing integration test: The iframe example seems to be failing because it can't find an element with role="textbox" within the iframe (in time). This doesn't seem to be related to the changes in this PR. I ran it locally a couple of times and those integration tests seem a bit flaky. Any ideas?

@dylans dylans merged commit 482b090 into ianstormtaylor:main Feb 23, 2022
@github-actions github-actions bot mentioned this pull request Feb 23, 2022
@rockettomatooo rockettomatooo deleted the fix-point-transform branch February 24, 2022 08:43
DougReeder pushed a commit to DougReeder/slate that referenced this pull request Apr 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants