Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#6492 closed New Feature (fixed)

Populate the Find dialog with the selected text.

Reported by: Joe Kavanagh Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.6.1
Component: UI : Dialogs Version: 3.0
Keywords: IBM Cc: Damian

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 (4)

6492.patch (1.7 KB) - added by Vicul 10 years ago.
New diff file
6492_2.patch (1.4 KB) - added by Vicul 10 years ago.
patch_2
6492_3.patch (1.4 KB) - added by Frederico Caldeira Knabben 10 years ago.
6492_4.patch (2.0 KB) - added by Garry Yao 10 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 10 years ago by Garry Yao

Status: newconfirmed

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

comment:2 Changed 10 years ago by Sa'ar Zac Elias

Version: 3.5 (SVN - 3.5.x)

comment:3 Changed 10 years ago by Frederico Caldeira Knabben

The plain full selection is ok as well.

comment:4 Changed 10 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 10 years ago by Vicul

Keywords: HasPatch added; IBM removed
Version: 3.5.4 (SVN - trunk)

comment:6 Changed 10 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 10 years ago by Wiktor Walc

Version: 3.5.4 (SVN - trunk)3.0

comment:8 Changed 10 years ago by Frederico Caldeira Knabben

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

Changed 10 years ago by Vicul

Attachment: 6492.patch added

New diff file

comment:9 Changed 10 years ago by Frederico Caldeira Knabben

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 10 years ago by Frederico Caldeira Knabben

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 10 years ago by Vicul

Attachment: 6492_2.patch added

patch_2

comment:11 Changed 10 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 10 years ago by Frederico Caldeira Knabben

Attachment: 6492_3.patch added

comment:12 Changed 10 years ago by Frederico Caldeira Knabben

Owner: set to Frederico Caldeira Knabben
Status: confirmedreview

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 10 years ago by Garry Yao

Status: reviewreview_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 10 years ago by Garry Yao

Attachment: 6492_4.patch added

comment:14 Changed 10 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 10 years ago by Garry Yao

Owner: changed from Frederico Caldeira Knabben to Garry Yao
Status: review_failedreview

comment:16 Changed 10 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 10 years ago by Frederico Caldeira Knabben

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

comment:18 Changed 10 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.6.1
Status: reviewreview_passed

comment:19 Changed 10 years ago by Garry Yao

Fixed with [6909], thanks Vicul.

comment:20 Changed 10 years ago by Garry Yao

Keywords: HasPatch removed
Resolution: fixed
Status: review_passedclosed

Doc updated with [6910].

comment:21 Changed 10 years ago by Sa'ar Zac Elias

The patch broke Chrome, fixed with [6914].

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