Opened 17 years ago
Closed 17 years ago
#3231 closed Bug (fixed)
Unable to apply inline styles when selection touches end of area with the same style already applied.
| Reported by: | Martin Kou | Owned by: | Garry Yao |
|---|---|---|---|
| Priority: | Must have (possibly next milestone) | Milestone: | CKEditor 3.0 |
| Component: | Core : Styles | Version: | SVN (CKEditor) - OLD |
| Keywords: | Oracle Review+ | Cc: | Senthil |
Description
To reproduce:
- Open replacebyclass.html in Windows Firefox.
- Double click on the first word "This ".
- Click the Bold button - the text is bolded.
- Double click on the second word "is ".
- Click the Bold button again.
- Nothing happens.
Attachments (3)
Change History (14)
comment:1 Changed 17 years ago by
| Owner: | set to Garry Yao |
|---|---|
| Status: | new → assigned |
comment:2 Changed 17 years ago by
| Resolution: | → duplicate |
|---|---|
| Status: | assigned → closed |
comment:3 Changed 17 years ago by
| Resolution: | duplicate |
|---|---|
| Status: | closed → reopened |
Changed 17 years ago by
| Attachment: | 3231.patch added |
|---|
comment:4 Changed 17 years ago by
| Keywords: | Review? added |
|---|
The proposed path now checking the range root node instead of the first node for disambiguition explained here.
comment:5 Changed 17 years ago by
| Priority: | Normal → High |
|---|
comment:6 Changed 17 years ago by
| Keywords: | Review- added; Review? removed |
|---|
i think the right fix here would have getStartElement returning the proper element in this situations.
After a chat with Garry, it looks like the correct solution would have this range...
<p> <strong>This ^</strong>is^ some ....
... getting fixed to this...
<p> <strong>This </strong>^is^ some ....
... in this case before trying to detect the start element (for non collapsed selections only).
Changed 17 years ago by
| Attachment: | 3231_2.patch added |
|---|
comment:7 Changed 17 years ago by
| Keywords: | Review? added; Review- removed |
|---|
The new patch is proposing the following changes:
- Introduce a new method of 'shrink' as well as related TC, which helps to deal with the above situation;
- Moving 'getChild' and 'getChildCount' from node.js into element.js where they should belonged to.
comment:8 Changed 17 years ago by
| Keywords: | Review- added; Review? removed |
|---|
The solution is good, and it works well.
I just see one problem here. You are introducing a new public function to the API (range::shrink), but that function is not complete because it doesn't consider two features that we don't need to care about for now:
- It acts only in the start boundary (as specified in the comments).
- It should have no effect in a collapsed range.
As we don't need the above features, it makes less sense placing them into the API just to make the shrink function complete. So I think it's better to simply move the code into line 626 at the selection plugin (adding a comment about it, pointing to this ticket).
Also, some minor enhancements to the code:
- Instead of "getText().length" you can use "getLength()".
- "this.startContainer" has been used four times in that code block. It would be better to have a local variable to point to it.
Changed 17 years ago by
| Attachment: | 3231_3.patch added |
|---|
comment:9 Changed 17 years ago by
| Keywords: | Review? added; Review- removed |
|---|
comment:10 Changed 17 years ago by
| Keywords: | Review+ added; Review? removed |
|---|
Instead of doing do{}while(true), you can can go simpler with while(true){} . Please fix it before committing.
comment:11 Changed 17 years ago by
| Resolution: | → fixed |
|---|---|
| Status: | reopened → closed |

Replying to martinkou:
After this step, Firefox NativeRange is weirdly report the selection as:
which confuse our selection checking system which just check the first element path.
This's been already reported at #2767.