Opened 13 years ago
Closed 13 years ago
#6235 closed Bug (fixed)
BIDI: Applying direction to multi-paragraph selection within a div
Reported by: | Damian | Owned by: | Tobiasz Cudnik |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 3.4.2 |
Component: | General | Version: | 3.4 |
Keywords: | IBM | Cc: | Joe Kavanagh, Satya, satya_minnekanti@… |
Description (last modified by )
When selecting multiple paragraphs that reside inside a <div>, the direction attribute is being set on the parent <div> rather than the paragraphs.
<div> <p>Paragraph 1</p> <p>Paragraph 2</p> </div>
The expectation is that the individual paragraphs get their direction attribute set.
Attachments (3)
Change History (21)
comment:1 Changed 13 years ago by
Cc: | satya_minnekanti@… added |
---|
comment:2 Changed 13 years ago by
Milestone: | → CKEditor 3.4.1 |
---|---|
Status: | new → confirmed |
comment:3 Changed 13 years ago by
Owner: | set to Tobiasz Cudnik |
---|---|
Status: | confirmed → assigned |
comment:4 Changed 13 years ago by
Status: | assigned → pending |
---|
comment:5 Changed 13 years ago by
This is a good question.
To help understand the impact, is it possible to get a list of cases where this might apply? One other case I'm guessing that is similar is lists.
comment:6 Changed 13 years ago by
This is true for following elements:
- table
- ul
- ol
- blockquote
- div
When their children will be selected, direction will be applied to parent element. Ticket responsible for this is #5909.
comment:7 Changed 13 years ago by
Correct steps to reproduce the defect is:
<div>
<p>Paragraph 1</p>
<p>Paragraph 2</p>
<p>Paragraph 3</p>
<p>Paragraph 4</p>
</div>
Create 3 or more paragraphs inside a div and then select any 2 paragraphs and apply RTL direction to them.
Expected Result: RTL Language direction should be applied only to the selected 2 paragraphs.
Actual Result: RTL Language direction is applied to the whole div and RTL Language direction is applied to all the paragraphs.
comment:8 Changed 13 years ago by
Status: | pending → confirmed |
---|
satya's provided test should at least be targeted.
comment:9 Changed 13 years ago by
Attached patch handles last Satya's TC, although it's a logic in opposite direction that main ticket's TC.
Anyway i would like to have some review on this one too.
Changed 13 years ago by
Attachment: | 6235.patch added |
---|
comment:10 Changed 13 years ago by
Description: | modified (diff) |
---|
@Tobias, I don't know how the block grouping feature (when nested blocks exist, style applied to the root block instead of contents), but it's problematic, considering the following case when applying the 'RTL' style:
<div> <p dir="ltr">Paragraph 1</p> <p>Paragraph 2</p> </div>
I propose removing the 'getFullySelected' logic and just let it be as the same way with alignment and indentation command.
comment:11 Changed 13 years ago by
Status: | confirmed → assigned |
---|
Changed 13 years ago by
Attachment: | 6235_2.patch added |
---|
comment:12 Changed 13 years ago by
Status: | assigned → review |
---|
It turns out that main problem for this ticket was the TC from the 7th comment, not feature which groups direction from it's parent to the container, i've updated the patch which seems to solve this particular issue.
comment:13 Changed 13 years ago by
Status: | review → review_failed |
---|
After the patch 'getFullySelected' stops working, because of the following line:
parent.$ != selectedElement
Should be instead:
parent.equals( selectedElement )
comment:14 Changed 13 years ago by
Besides, we should not get confused by empty text nodes also:
if ( selectionStart.getIndex() > 0 || selectionStart.getIndex() + 1 < selectedElement.getChildCount() )
Changed 13 years ago by
Attachment: | 6235_3.patch added |
---|
comment:16 Changed 13 years ago by
Status: | review_failed → review |
---|
Proposed patch seems to be a better approach. I would give R+ for it.
comment:17 Changed 13 years ago by
Status: | review → review_passed |
---|
comment:18 Changed 13 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Fixed with [5977].
This feature interferer with a logic responsible for matching one fully selected element. In this case it's DIV, but after selecting all cells of a table, then it would change direction on table only.
Do you need this feature only for paragraphs ? What about paragraphs inside a table cell ?