Opened 4 years ago

Closed 2 years ago

Last modified 2 years ago

#11697 closed Bug (fixed)

It is possible to replace content with wrong letter case

Reported by: Piotr Jasiun Owned by: kkrzton
Priority: Normal Milestone: CKEditor 4.5.9
Component: General Version: 3.0
Keywords: Cc:

Description

  1. Open Replace by Class sample.
  2. Open 'Replace' dialog.
  3. Type 'APOLLO' into 'Find what' field.
  4. Press 'Replace' button.
  5. Check 'Match case' ckeckbox.
  6. Press 'Replace' button.

Result: 'Apollo' has been replaced even though the 'Match case' was checked and case did not mach.

Probably 'Match case' button (and the rest of the find Options) should reset find tool.

Change History (12)

comment:1 Changed 4 years ago by Jakub Ś

Status: newconfirmed
Version: 3.6.6 (SVN - trunk)3.0

comment:2 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.7

comment:3 Changed 3 years ago by kkrzton

Owner: set to kkrzton
Status: confirmedassigned

comment:4 Changed 3 years ago by kkrzton

There is also a case with changing text in 'Find what' field:

  1. Open Replace by Class sample.
  2. Open 'Replace' dialog.
  3. Type 'APOLLO' into 'Find what' field.
  4. Press 'Replace' button.
  5. Type 'APOLLO 11' into 'Find what' field.
  6. Press 'Replace' button.

Result: 'Apollo' has been replaced.

Probably the new text should be searched for and selected (or not if not found). The next 'Replace' button click should replace text.

comment:5 Changed 3 years ago by kkrzton

I think it should be fixed along with #11968.

comment:6 Changed 3 years ago by kkrzton

Status: assignedreview

Changes in t/11697.

The fix introduces slight change in find/replace behavior. If match options (match case, match words) or pattern were changed after find had been performed (and text selected), the next find operation (find button click) does not perform replace operation. Instead another find is done to rematch text.

comment:7 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.7CKEditor 4.5.8

comment:8 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.8CKEditor 4.5.9

comment:9 Changed 2 years ago by Marek Lewandowski

Status: reviewreview_failed

The solution looks to be good, there are only couple style issues:

  • I'm missing manual tc.
  • Unit tests: assert.isFalse( dialog.parts.dialog.isVisible(), 'dialog is hidden' ); - there is no reason for us to test if hiding dialog works, calling dialog.getButton( 'cancel' ).click(); is just enough.
  • Unit tests: could you add assetr messages / comments to assertions in "test find and replace with pattern change - add text after pattern" and "test find and replace with pattern change - add text before pattern"? Without it it's difficult to see difference between the two.
  • Please rebase it on top of the master branch. There are conflicts, but should be fairly easy to resolve.

comment:10 Changed 2 years ago by kkrzton

Status: review_failedassigned

comment:11 Changed 2 years ago by kkrzton

Status: assignedreview

Changes in t/11697.

  • Added manual tc.
  • Added assert messages in unit tests, also rearranged the code and test names so it's easier to understand what's going on at first glance.
  • Rebased on top of the master, also squashed some commits.
Last edited 2 years ago by kkrzton (previous) (diff)

comment:12 Changed 2 years ago by Marek Lewandowski

Resolution: fixed
Status: reviewclosed

Great job! Fixed with git:0f2ef4c49b (merged to master).

Last edited 2 years ago by Marek Lewandowski (previous) (diff)
Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy