Opened 4 years ago

Closed 4 years ago

#12178 closed Bug (fixed)

[Blink/Webkit] Iterator does not return block if selection is located at the end of it

Reported by: Piotrek Koszuliński Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.4.4
Component: Core : Selection Version:
Keywords: Blink Webkit Cc:

Description

  1. Open any sample.
  2. Make following selection:
    <p>foo</p>
    <p>bar[</p>
    <p>bo]m</p>
    

E.g. select "bo" and press shift+left.

  1. Apply some format.
  • Expected: Format is applied to both paragraphs, because selection clearly starts in <p>bar<p> (you can verify that by using some different format and checking elements path).
  • Actual: Format is applied only to the last paragraph ("bom").

Change History (8)

comment:1 Changed 4 years ago by Piotrek Koszuliński

Keywords: Blink Webkit added
Status: newconfirmed

comment:2 Changed 4 years ago by Piotrek Koszuliński

Summary: [Blink/Webkit] Iterator does not return block whose end is selected[Blink/Webkit] Iterator does not return block if selection is located at the end of it

comment:3 Changed 4 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.3CKEditor 4.4.4

The 4.4.3 milestone was shortened, so the remaining tickets must be postponed.

comment:4 Changed 4 years ago by Piotrek Koszuliński

Owner: set to Piotrek Koszuliński
Status: confirmedassigned

comment:5 Changed 4 years ago by Piotrek Koszuliński

Status: assignedreview

Pushed branch:t/12178 on pre-review.

First I tried to use 3rd range.shrink() parameter: git:63b058a. However, this broke one odd iterator test which I couldn't understand until I noticed what strange things rangesIterator#getNextRange does. It creates bookmarks regardless of location, what makes sense, but we end up with spans inside <table>. So if range.shrink() on range containing table cell does not work (when 3rd parameter is true), we start auto paragraphing inside <table> tag and in similar locations - e.g. in case of entire table selection.

So I understood that we want to prevent shrinking range only if its start or end is located at block boundary - e.g. <p>foo[</p>. I implemented custom logic inside iterator that drives this and AFAICS everything works fine.

However, range#shrink's 3rd parameter is shrinkOnBlockBoundary and <td> is not a block boundary when we take element path's meaning. In such case we should fix range#shrink (and use the 3rd parameter), not the iterator. On the other hand I'm afraid that in this case "block" means "not inline element" and we could do some damage.

BTW. I was looking at range#checkEnd/StartOfBlock which I think I could perhaps use instead of the function I created, but it's scary to use undocumented method which oddly has no common fragment with range.checkBoundaryOfElement. Do you recall what do these methods do?

comment:6 in reply to:  5 Changed 4 years ago by Frederico Caldeira Knabben

Replying to Reinmar:

Pushed branch:t/12178 on pre-review.

First I tried to use 3rd range.shrink() parameter: git:63b058a. However, this broke one odd iterator test which I couldn't understand until I noticed what strange things rangesIterator#getNextRange does. It creates bookmarks regardless of location, what makes sense, but we end up with spans inside <table>. So if range.shrink() on range containing table cell does not work (when 3rd parameter is true), we start auto paragraphing inside <table> tag and in similar locations - e.g. in case of entire table selection.

So I understood that we want to prevent shrinking range only if its start or end is located at block boundary - e.g. <p>foo[</p>. I implemented custom logic inside iterator that drives this and AFAICS everything works fine.

Looks fine for me.

However, range#shrink's 3rd parameter is shrinkOnBlockBoundary and <td> is not a block boundary when we take element path's meaning. In such case we should fix range#shrink (and use the 3rd parameter), not the iterator. On the other hand I'm afraid that in this case "block" means "not inline element" and we could do some damage.

Yes, I think that <td> is really supposed to be a block element here, so better not to touch it.

BTW. I was looking at range#checkEnd/StartOfBlock which I think I could perhaps use instead of the function I created, but it's scary to use undocumented method which oddly has no common fragment with range.checkBoundaryOfElement. Do you recall what do these methods do?

These methods should do the very same as checkBoundaryOfElement does. I think that checkBoundaryOfElement has been simply introduced later, so we have a bit of code duplication here.


Should we have a R+ here so?

comment:7 Changed 4 years ago by Piotrek Koszuliński

I checked this on iterator tests:

console.log( startAtInnerBoundary, endAtInnerBoundary, range.checkEndOfBlock(), range.checkStartOfBlock() );

It rarely gives ( X, Y, X, Y ) values or any other pattern. The problem I think is with which range's boundary is on which block's boundary. As I documented, this is what we need in iterator:

			// Remember if following situation takes place:
			// * startAtInnerBoundary: <p>foo[</p>...
			// * endAtInnerBoundary: ...<p>]bar</p>

I think that range's methods will rather look for: <p>[foo... and <p>bar]</p>, so the opposite.

So I'll stick to my functions. I'll do last tests and close ticket if everything works fine.

comment:8 Changed 4 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed

Fixed on master with git:5ea2a38.

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