#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
- Open Replace by Class sample.
- Open 'Replace' dialog.
- Type 'APOLLO' into 'Find what' field.
- Press 'Replace' button.
- Check 'Match case' ckeckbox.
- 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 11 years ago by
Status: | new → confirmed |
---|---|
Version: | 3.6.6 (SVN - trunk) → 3.0 |
comment:2 Changed 9 years ago by
Milestone: | → CKEditor 4.5.7 |
---|
comment:3 Changed 9 years ago by
Owner: | set to kkrzton |
---|---|
Status: | confirmed → assigned |
comment:4 Changed 9 years ago by
comment:6 Changed 9 years ago by
Status: | assigned → review |
---|
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 9 years ago by
Milestone: | CKEditor 4.5.7 → CKEditor 4.5.8 |
---|
comment:8 Changed 9 years ago by
Milestone: | CKEditor 4.5.8 → CKEditor 4.5.9 |
---|
comment:9 Changed 9 years ago by
Status: | review → review_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, callingdialog.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 9 years ago by
Status: | review_failed → assigned |
---|
comment:11 Changed 9 years ago by
Status: | assigned → review |
---|
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.
comment:12 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | review → closed |
Great job! Fixed with git:0f2ef4c49b (merged to master).
There is also a case with changing text in 'Find what' field:
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.