Ticket #6492 (closed New Feature: fixed)

Opened 4 years ago

Last modified 3 years ago

Populate the Find dialog with the selected text.

Reported by: JoeK Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.6.1
Component: UI : Dialogs Version: 3.0
Keywords: IBM Cc: damo

Description

If the user selects some text and then opens the Find dialog the Find field would be automatically populated with the selected text.

This is the behaviour in most applications. It is quite useful when searching for other occurrences of some existing text.

Attachments

6492.patch (1.7 KB) - added by Vicul 3 years ago.
New diff file
6492_2.patch (1.4 KB) - added by Vicul 3 years ago.
patch_2
6492_3.patch (1.4 KB) - added by fredck 3 years ago.
6492_4.patch (2.0 KB) - added by garry.yao 3 years ago.

Change History

comment:1 Changed 4 years ago by garry.yao

  • Status changed from new to confirmed

In general, the field is filled only if a word is selected.

comment:2 Changed 4 years ago by Saare

  • Version 3.5 (SVN - 3.5.x) deleted

comment:3 Changed 4 years ago by fredck

The plain full selection is ok as well.

comment:4 Changed 3 years ago by Vicul

For adding that feature is necessary to do some modifications in the file:

/trunk/_source/plugins/find/dialogs/find.js

  1. Add the new function:
  • before modification:

function getSearchRange( isDefault )

{

var searchRange,

sel = editor.getSelection(),

body = editor.document.getBody();

if ( sel && !isDefault )

{

searchRange = sel.getRanges()[ 0 ].clone();

searchRange.collapse( true );

}

else

{

searchRange = new CKEDITOR.dom.range();

searchRange.setStartAt( body, CKEDITOR.POSITION_AFTER_START );

}

searchRange.setEndAt( body, CKEDITOR.POSITION_BEFORE_END );

return searchRange;

}

  • after modification

function getSearchRange( isDefault )

{

var searchRange,

sel = editor.getSelection(),

body = editor.document.getBody();

if ( sel && !isDefault )

{

searchRange = sel.getRanges()[ 0 ].clone();

searchRange.collapse( true );

}

else

{

searchRange = new CKEDITOR.dom.range();

searchRange.setStartAt( body, CKEDITOR.POSITION_AFTER_START );

}

searchRange.setEndAt( body, CKEDITOR.POSITION_BEFORE_END );

return searchRange;

}

function getSelectedText( editor )

{

var selectedText = ;

var selection = editor.getSelection();

if ( selection.getType() == CKEDITOR.SELECTION_TEXT )

{

if ( CKEDITOR.env.ie )

{

selection.unlock(true);

selectedText = selection.getNative().createRange().text;

}

else

{

selectedText = selection.getNative();

}

}

return(selectedText);

}

  1. Add the following code in the function onShow : function():
  • before modification:

onShow : function()

{

Establish initial searching start position.

finder.searchRange = getSearchRange();

this.selectPage( startupPage );

},

  • after modification

onShow : function()

{

Establish initial searching start position.

finder.searchRange = getSearchRange();

this.selectPage( startupPage );

set the user selected text

var editor = this.getParentEditor();

var selectedContent = getSelectedText( editor ).toString();

var pageId = this._.currentTabId;

patternFieldId = pageId === 'find' ? 'txtFindFind' : 'txtFindReplace';

patternField = this.getContentElement( pageId, patternFieldId );

patternField.setValue(selectedContent);

},

comment:5 Changed 3 years ago by Vicul

  • Keywords HasPatch added; IBM removed
  • Version set to 3.5.4 (SVN - trunk)

comment:6 Changed 3 years ago by garry.yao

  • Keywords IBM added

@Vicul thanks for the patch, but could u please created a diff file for what you're patching?

comment:7 Changed 3 years ago by wwalc

  • Version changed from 3.5.4 (SVN - trunk) to 3.0

comment:8 Changed 3 years ago by fredck

@Vicul, please provide us a SVN patch based on trunk. Thanks!

Changed 3 years ago by Vicul

New diff file

comment:9 follow-up: ↓ 10 Changed 3 years ago by fredck

Some issues on the patch:

  • The toString() call must be done in the getSelectedText function directly (line 615), to avoid it being done on onShow (line 877).
  • The patternFieldId and patternField seem to be missing the "var" declaration.
  • return(selectedText) should not have the parenthesis.
  • With IE, the current selection get lost if you open the dialog and close it immediately.
  • With IE, it doesn't work well. Sometimes the field doesn't get filled. This seems to be random and may be related to the browser speed of working with the selection.
  • There are several unnecessary TABs in the code. (lines 70, 877 and 881)

comment:10 in reply to: ↑ 9 Changed 3 years ago by fredck

Replying to fredck:

  • With IE, the current selection get lost if you open the dialog and close it immediately.

As for this issue, it seems to be enough to simply lock the selection again, after unlocking it.

Changed 3 years ago by Vicul

patch_2

comment:11 Changed 3 years ago by Vicul

I fixed those issues except on IE: I could not catch that situation, tried at several PCs for an hour, please specify additional conditions.

Changed 3 years ago by fredck

comment:12 Changed 3 years ago by fredck

  • Status changed from confirmed to review
  • Owner set to fredck

Calling finder.find() to solve the IE issue was a bit of an ugly hack. It also made it loose the selection again when the selection involved more than one paragraph. The solution for it was already on my last comment, by simply calling selection.lock() again.

As for the random IE issue, seems still to happen, but it's not related to this ticket. It's supposed to be an issue with the selection system which deserves a dedicated ticket as soon as this one gets committed.

The new patch also includes minor fixes in the code style and minor changes.

comment:13 Changed 3 years ago by garry.yao

  • Status changed from review to review_failed

As for the random IE issue, seems still to happen...

I can hardly call it a random issue as the issue is reproduced for every fresh opening of the dialog, I'm not sure if there's a solution exist regard this particular selection system bug (as it might be a browser bug), but we can definitely avoid it - considering that getSelectedText is really a highly requested method by the community, baking it into dom.selection definitely make sense so lock could consider it as well.

Changed 3 years ago by garry.yao

comment:14 Changed 3 years ago by garry.yao

Besides, populated text must be full selected and page select must happen after it to avoid an existing IE6/7 hack.

comment:15 Changed 3 years ago by garry.yao

  • Owner changed from fredck to garry.yao
  • Status changed from review_failed to review

comment:16 Changed 3 years ago by Vicul

Considered your patches, thanks for assisting with those issues. Garry, can't follow your last comment, could you explain it detaily. Still can't reproduce random fail of IE, although been testing it in IE 7,8 even with huge amount of text. And generally please resume what else left to be done from my side to finish it up. Thank you.

comment:17 Changed 3 years ago by fredck

@Vicul, there is nothing else left here. We'll be working to close this ticket now. Thanks for the contribution!

comment:18 Changed 3 years ago by fredck

  • Status changed from review to review_passed
  • Milestone set to CKEditor 3.6.1

comment:19 Changed 3 years ago by garry.yao

Fixed with [6909], thanks Vicul.

comment:20 Changed 3 years ago by garry.yao

  • Keywords HasPatch removed
  • Status changed from review_passed to closed
  • Resolution set to fixed

Doc updated with [6910].

comment:21 Changed 3 years ago by Saare

The patch broke Chrome, fixed with [6914].

Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy