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 editing the search input when toggling the search pane #1072

Merged
merged 2 commits into from Feb 20, 2019

Conversation

Projects
None yet
3 participants
@rafeca
Copy link
Contributor

rafeca commented Feb 15, 2019

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

The find and replace package was causing some issues with the cursor when hiding and showing the search panel under some circumstances. I could find two scenarios where this was happening:

  • Scenario 1: When searching for an existing text but not selecting it from the document (by not pressing Enter after searching). This is the scenario described in #1067
  • Scenario 2: When searching for a non-existing text and then hiding and showing the search panel.

The issues comes from the fact that on every search the find view is re-setting the TextEditor grammar, and under some circumstances (when hiding and showing the panel) this causes a bad state on the TextEditor, which prevents the cursor from moving correctly within the find input.

I haven't been able to find the root issue that's causing the bad state on the TextEditor, but anyways this PR prevents it from happening by only setting the grammar of the TextEditor when it's needed.

I haven't added any automated test that verifies the correct behaviour, since it may be a bit complex to reproduce the issue programatically (haven't checked it though), but I'll happily add a test case if you think it's worth it :)

Alternate Designs

N/A

Benefits

Users won't get confused when toggling the search bar and not being able to change the search text.

Possible Drawbacks

N/A

Applicable Issues

This closes #1067

@lee-dohm

This comment has been minimized.

Copy link
Member

lee-dohm commented Feb 18, 2019

Since this is a fix for a regression, we should probably make sure we have at least one test for it.

@rafeca

This comment has been minimized.

Copy link
Contributor Author

rafeca commented Feb 20, 2019

Since this is a fix for a regression, we should probably make sure we have at least one test for it.

Thanks for the suggestion! I've managed to create a test that was failing before this fix, I'm not proud of the test since it's asserting an implementation detail of the editor (the selection DOM element size) but that's the only way that I've managed to assert the correct behaviour.

To give some context about the issue, the root problem can only be seen in the internal state of the TextEditor element: all the data structures that get exposed from the TextEditor are correct and the issue is only introduced when it calculates the selection dimensions, which end up being passed to the Highlight component.

From my investigation, this is what I think was going on before this PR:

  1. On every keystroke on the find text input, the TextBuffer of the find-and-replace TextEditor gets updated (via its setText() method). This causes a new DisplayLayer to be created (and all this makes the TextEditor to recalculate all the dimensions, via its updateSync() method. All this is the standard behaviour of TextEditor.
  2. When the user stops editing the find input (and after some debounce timeout), the search is performed, and then the grammar of the TextEditor is re-setted.
  3. Updating the grammar sets the languageMode of the TextBuffer associated to the editor. This calls the DisplayLayer which resets its screenLines array, but does not recalculate the TextEditor dimensions.
  4. If the user then closes the find panel, then the main editor gets focused just after the find panel gets hidden.
  5. When the main editor gets focused, the hidden input on the find TextEditor gets blurred and this causes a recalculation of the dimensions on the TextEditor. Since the grammar was updated on step 3 and the dimensions were not recalculated afterwards, this triggers a full recalculation of the selection (using this logic.
  6. Since the TextEditor is not visible in the DOM anymore (not sure if it's even attached), the calculation of dimensions returns 0 and causes the wrong selection.

Based on this, a better fix to this same issue would be to change the logic that handles the update of the grammar of a TextEditor to also trigger a recalculation of dimensions. Still, I think that the current PR is still valid since it prevents additional work on each keystroke.

@jasonrudolph
Copy link
Member

jasonrudolph left a comment

I've managed to create a test that was failing before this fix

Nice work. @rafeca! ⚡️

I noted one minor question/suggestion inline below, but overall, I think this is good to go. 🚀

@@ -109,6 +109,26 @@ describe("FindView", () => {
"current < pivot \\? left\\.push\\(current\\) : right\\.push\\(current\\);"
);
});

it('selects the text to find when the panel is shown', async () => {

This comment has been minimized.

@jasonrudolph

jasonrudolph Feb 20, 2019

Member

Should we change "shown" to "re-shown"?

I ask because the bug seems to involve re-showing the panel. The test below also seems to approximate this behavior (i.e., it pre-populates the text of the findEditor, similar to what would have happened when the find panel was first shown, and then it shows the find panel).

What do you think?

This comment has been minimized.

@rafeca

rafeca Feb 20, 2019

Author Contributor

Exactly! I'm gonna change it to "re-shown", naming is hard 😅

@rafeca rafeca force-pushed the fix/focus-search-input branch from 022f77c to 273a37e Feb 20, 2019

@rafeca rafeca force-pushed the fix/focus-search-input branch from 273a37e to 6c95e86 Feb 20, 2019

@rafeca rafeca merged commit d1bfe9c into master Feb 20, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rafeca rafeca deleted the fix/focus-search-input branch Feb 20, 2019

@rafeca rafeca self-assigned this Feb 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.