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

[Piotrek Koszuliński]

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.

3. 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").

[Piotrek Koszuliński]

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

[Piotrek Koszuliński]

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?

[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?

[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.

[Piotrek Koszuliński]

Fixed on master with ​git:5ea2a38.