Fix: Move Line Up/Down collapses inline editor when moving past the start/end #2431

Merged
merged 6 commits into from Feb 19, 2013

Conversation

Projects
None yet
2 participants
Contributor

TomMalbran commented Dec 22, 2012

This is a possible fix for the issue adobe/brackets#1933.

When moving a line down on an inline editor, the problem seems to be moving the line to the last line and not moving the last line beyond the inline widget. But moving the line beyond the inline widgets breaks getLastVisibleLine() which starts giving 1 line less than it should and then breaking the move line up.

So this change doesn't move down in an inline editor when trying to move the line to the last visible line or when moving the last visible line down.

jasonsanjose was assigned Jan 2, 2013

Member

jasonsanjose commented Jan 3, 2013

Reviewing

Member

jasonsanjose commented Jan 3, 2013

Tried out your fix and I'm seeing a few issues. You should be able to see them as well in the getting started project. Open an inline editor on line 14 on the h2. This opens main.css lines 19-21. Reset the file after each bug described.

  • Attempt to move line 20 down. Result: Does not move.
  • Attempt to move line 21 up. Result: Line 20 becomes line 22.
Member

jasonsanjose commented Jan 4, 2013

@TomMalbran Looks like this pull request was submitted right before our winter break. Just pinging you again in case you've been on vacation. Thanks for the contribution.

Contributor

TomMalbran commented Jan 7, 2013

I knew it was before the vacations, but I got the idea about the fix right back then, and yes, I am on vacations and with poor internet connection. I'll be back in a few days anyway and try to fix this.

  • Moving the line 20 down on a 19-21 widget closed it, so as a fix, I blocked it. There might be a better way to fix this, but not within the function.
  • I didnt noticed the other issue, so the solution would be to block the moving up of the last line.
Contributor

TomMalbran commented Jan 10, 2013

I fixed the second issue mentioned.

But I cant find a way to fix the first one, since the move line up generates a document change that gets into the special case of TextRange.prototype._applySingleChangeToRange which ultimately removes the link to the inline editor and closing it. I am not sure if there is a way to avoid this, but anyway, in the bug issue it was expected that the text shouldn't move if moving it would cause the inline editor to close, so this fix does this, even thought the line that causes the editor to close is not the last one as mentioned but the second to last, which is the one that I blocked from moving down.

Member

jasonsanjose commented Jan 19, 2013

I wasn't able to get this tested in time for sprint 19. We'll take a look at this again for sprint 20.

Member

jasonsanjose commented Feb 15, 2013

@TomMalbran more apologies for being out sick the last week. Do you have time to try out your fix now that CodeMirror 3 has landed?

Member

jasonsanjose commented Feb 15, 2013

Actually, I had some time to try this out with cmv3. I'm still seeing some issues.

  1. Open inline editor on line 51 in getting started (samp tag)
  2. Move line 26 up
  3. Result below:
samp
{
    display: none;

    /* hide <samp> from the browser so we can show cool features in Edge Code */}
Contributor

TomMalbran commented Feb 15, 2013

Yes I just tested it on the cmv3 and found the same bug. I am trying to fix that right now.

Contributor

TomMalbran commented Feb 15, 2013

That didn't completely fixed the problem. Trying again.

Contributor

TomMalbran commented Feb 15, 2013

@jasonsanjose Now it should be properly fixed. I tested on several inline and main editors and it worked on all of them.

Member

jasonsanjose commented Feb 19, 2013

Looks good. Merging.

@jasonsanjose jasonsanjose added a commit that referenced this pull request Feb 19, 2013

@jasonsanjose jasonsanjose Merge pull request #2431 from TomMalbran/tom/fix-issue-1933
Fix: Move Line Up/Down collapses inline editor when moving past the start/end
84e4a7a

@jasonsanjose jasonsanjose merged commit 84e4a7a into adobe:master Feb 19, 2013

1 check passed

default The Travis build passed
Details

TomMalbran deleted the TomMalbran:tom/fix-issue-1933 branch Feb 19, 2013

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