Ticket #6235 (closed Bug: fixed)

Opened 4 years ago

Last modified 4 years ago

BIDI: Applying direction to multi-paragraph selection within a div

Reported by: damo Owned by: tobiasz.cudnik
Priority: Normal Milestone: CKEditor 3.4.2
Component: General Version: 3.4
Keywords: IBM Cc: JoeK, Satya, satya_minnekanti@…

Description (last modified by garry.yao) (diff)

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

6235.patch (1.2 KB) - added by tobiasz.cudnik 4 years ago.
6235_2.patch (1.1 KB) - added by tobiasz.cudnik 4 years ago.
6235_3.patch (1.7 KB) - added by garry.yao 4 years ago.

Change History

comment:1 Changed 4 years ago by satya

  • Cc satya_minnekanti@… added

comment:2 Changed 4 years ago by Saare

  • Status changed from new to confirmed
  • Milestone set to CKEditor 3.4.1

comment:3 Changed 4 years ago by tobiasz.cudnik

  • Owner set to tobiasz.cudnik
  • Status changed from confirmed to assigned

comment:4 Changed 4 years ago by tobiasz.cudnik

  • Status changed from assigned to pending

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 ?

comment:5 Changed 4 years ago by damo

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 4 years ago by tobiasz.cudnik

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 4 years ago by satya

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 4 years ago by garry.yao

  • Status changed from pending to confirmed

satya's provided test should at least be targeted.

comment:9 Changed 4 years ago by tobiasz.cudnik

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 4 years ago by tobiasz.cudnik

comment:10 Changed 4 years ago by garry.yao

  • 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 4 years ago by tobiasz.cudnik

  • Status changed from confirmed to assigned

Changed 4 years ago by tobiasz.cudnik

comment:12 Changed 4 years ago by tobiasz.cudnik

  • Status changed from assigned to 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 4 years ago by garry.yao

  • Status changed from review to review_failed

After the patch 'getFullySelected' stops working, because of the following line:

parent.$ != selectedElement

Should be instead:

parent.equals( selectedElement )

comment:14 Changed 4 years ago by garry.yao

Besides, we should not get confused by empty text nodes also:

if ( selectionStart.getIndex() > 0 || selectionStart.getIndex() + 1 < selectedElement.getChildCount() ) 

Changed 4 years ago by garry.yao

comment:15 Changed 4 years ago by garry.yao

Proposing here a simpler way of checking fully selected block.

comment:16 Changed 4 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

Proposed patch seems to be a better approach. I would give R+ for it.

comment:17 Changed 4 years ago by garry.yao

  • Status changed from review to review_passed

comment:18 Changed 4 years ago by tobiasz.cudnik

  • Status changed from review_passed to closed
  • Resolution set to fixed

Fixed with [5977].

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