Fix issue with selection being lost when InsertListCommand changes the list type.

Fix issue with selection being lost when InsertListCommand changes the list type.

When changing the list type (from ordered to unordered or vice-versa) of a list item,
a new list containing that list item is created.
Now, if that same list item (contained within the new list) is selected in its entirety and then
its list type reversed again, it merges itself back into the original list.
However, post this merge, the selection over that list item is lost.

The selection is lost because we lose the 'start' of the selection. We thus ought to maintain
the start index of the selection in the same way we maintain the end index of the selection before
merging the two lists.(Done in InsertListCommand::doApply())

The reason why this behavior is observed only when a list item is selected in its entirety is
because we do preserve the start of the selection in case the original selection spans across
multiple paragraphs, but skip this (the if check in doApply()) if the start and the end of the
selection lie on the same line.

So now, we simply need to maintain the indices for the start and the end of the selection before
we insert or merge new lists. Afterwards, we can reset the ending selection using these start
and end indices.

The selection range for the existing test, insert-list-nested-with-orphaned.html changes with
this fix. It now sets the selection over the entire list, as expected.
The test initially sets the selection over the entire list which was later lost after
performing the insertOrderedList command.

BUG=328030
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177084

[yosin_UTC9, 2013-12-13 01:10:40]

https://codereview.chromium.org/114043002/diff/1/LayoutTests/editing/execComm...
File
LayoutTests/editing/execCommand/selection-after-switch-type-of-listitem.html
(right):

https://codereview.chromium.org/114043002/diff/1/LayoutTests/editing/execComm...
LayoutTests/editing/execCommand/selection-after-switch-type-of-listitem.html:15:
document.execCommand('InsertUnOrderedList', false, null);
nit: we don't need to specify |false| and |null|.

https://codereview.chromium.org/114043002/diff/1/Source/core/editing/InsertLi...
File Source/core/editing/InsertListCommand.cpp (right):

https://codereview.chromium.org/114043002/diff/1/Source/core/editing/InsertLi...
Source/core/editing/InsertListCommand.cpp:399:
listItemElement->document().updateLayoutIgnorePendingStylesheets();
nit: Just |document()|, which is member function of EditCommand.
Why do we need to update layout? We should avoid explicit call of update layout.

[arpitab_, 2013-12-13 12:00:43]

Hi Yoshi,
Thanks for the review. Have uploaded another patch after fixing the nits.

On 2013/12/13 01:10:40, Yoshi wrote:
>
https://codereview.chromium.org/114043002/diff/1/LayoutTests/editing/execComm...
> File
> LayoutTests/editing/execCommand/selection-after-switch-type-of-listitem.html
> (right):
> 
>
https://codereview.chromium.org/114043002/diff/1/LayoutTests/editing/execComm...
>
LayoutTests/editing/execCommand/selection-after-switch-type-of-listitem.html:15:
> document.execCommand('InsertUnOrderedList', false, null);
> nit: we don't need to specify |false| and |null|.
Done.

> 
>
https://codereview.chromium.org/114043002/diff/1/Source/core/editing/InsertLi...
> File Source/core/editing/InsertListCommand.cpp (right):
> 
>
https://codereview.chromium.org/114043002/diff/1/Source/core/editing/InsertLi...
> Source/core/editing/InsertListCommand.cpp:399:
> listItemElement->document().updateLayoutIgnorePendingStylesheets();
> nit: Just |document()|, which is member function of EditCommand.
Done.

> Why do we need to update layout? We should avoid explicit call of update
layout.
The reason why we need to do an explicit layout here is because while merging
the list the
inline boxes get destroyed. Thus, in order to obtain the correct position form
visiblePositionForIndex()
we need to do a layout. For similar reasons, we do an explicit layout even for
the scenario when a new list is created [ln: 386].
If say we do not call this layout here, the first call to
visiblePositionForIndex() to obtain a valid start of selection will yield an
incorrect position. However while trying to create a VisiblePosition we do
perform a layout.
Thus subsequent calls to visiblePositionForIndex() shall return correct
positions.

[leviw_travelin_and_unemployed, 2013-12-18 21:39:42]

> Fixing selection over list items when changing the list order type of the
existing list items.
Nit: Fix issue with selection being lost when InsertListCommand changes the list
type?

> When trying to change list the order type of an existing list item selected 
> in its entirety, the selection is not reset properly. 
> When the list order type is changed once, a new list with an order different
than 
> the existing one is created. The selection is maintained in this case. 
> 
> When for the same selection (the same list item) we again change the list
order, thereby 
> causing it to merge with the previously existing list, the start of the
original 
> selection is lost and needs to be restored. 
> For achieving the same we need to store the index for start of the selection
as we 
> do for the end of the selection as well. 

I can't follow these paragraphs.

> So now, we simply need to maintain the indexes for the start and the end of
the 
> selection before we insert or merge new lists. Post this, we can reset the
ending 
Nit: s/Post this/Afterwards
> selection using these start and end indexes. 
Nit: indices.

> However, the first call to visiblePositionForIndex(), for either obatining the
start 
Nit: obtaining
> or the end VisiblePosition, does not return the correct position based on the
passed 
> index. This is because in listifyParagraph(), for the scenario when we merge
the 
> list into an existing list, the inlineBoxes may get destroyed. Thus a layout
becomes 
> necessary here in order to obtain the correct position. 
> The same change has now been added in listifyParagraph(). Note that an update
layout 
> is already being done for the scenario when a new list is inserted. 
It seems wrong to place the layout in listifyParagraph if it's only necessary
for this scenario. I'm also surprised we'd ever create a VisiblePosition from an
index without ensuring layout is up-to-date... I believe that's an invariant
that should be enforced in that function itself.

> The selection range for the existing test,
insert-list-nested-with-orphaned.html changes 
> with this fix. It now sets the selection over the entire list, as expected. 
> The test initually sets the selection over the entire list which was later
lost incorrectly 
Nit: initially
> after performing the insertOrderedList command. This is fixed now and thus the
selection 
> over the contents is retained.
Nit: this last sentence is redundant.

https://codereview.chromium.org/114043002/diff/60001/Source/core/editing/Inse...
File Source/core/editing/InsertListCommand.cpp (right):

https://codereview.chromium.org/114043002/diff/60001/Source/core/editing/Inse...
Source/core/editing/InsertListCommand.cpp:130: bool forceCreateList = false;
nit: forceListCreation?

https://codereview.chromium.org/114043002/diff/60001/Source/core/editing/Inse...
Source/core/editing/InsertListCommand.cpp:399:
document().updateLayoutIgnorePendingStylesheets();
Why?

[arpitab_, 2013-12-20 10:12:10]

Thanks for the review Levi.

On 2013/12/18 21:39:42, Levi wrote:
> > Fixing selection over list items when changing the list order type of the
> existing list items.
> Nit: Fix issue with selection being lost when InsertListCommand changes the
list
> type?
Done.
> 
> > When trying to change list the order type of an existing list item selected 
> > in its entirety, the selection is not reset properly. 
> > When the list order type is changed once, a new list with an order different
> than 
> > the existing one is created. The selection is maintained in this case. 
> > 
> > When for the same selection (the same list item) we again change the list
> order, thereby 
> > causing it to merge with the previously existing list, the start of the
> original 
> > selection is lost and needs to be restored. 
> > For achieving the same we need to store the index for start of the selection
> as we 
> > do for the end of the selection as well. 
> 
> I can't follow these paragraphs.
> 
Have tried to modify/simply the explanation.

> > So now, we simply need to maintain the indexes for the start and the end of
> the 
> > selection before we insert or merge new lists. Post this, we can reset the
> ending 
> Nit: s/Post this/Afterwards
> > selection using these start and end indexes. 
> Nit: indices.
>
Done.
 
> > However, the first call to visiblePositionForIndex(), for either obatining
the
> start 
> Nit: obtaining
Done.

> > or the end VisiblePosition, does not return the correct position based on
the
> passed 
> > index. This is because in listifyParagraph(), for the scenario when we merge
> the 
> > list into an existing list, the inlineBoxes may get destroyed. Thus a layout
> becomes 
> > necessary here in order to obtain the correct position. 
> > The same change has now been added in listifyParagraph(). Note that an
update
> layout 
> > is already being done for the scenario when a new list is inserted. 
> It seems wrong to place the layout in listifyParagraph if it's only necessary
> for this scenario. I'm also surprised we'd ever create a VisiblePosition from
an
> index without ensuring layout is up-to-date... I believe that's an invariant
> that should be enforced in that function itself.
> 
listifyParagraph() already has an updateLayout call for the case when a new list
item is created.

Since an updateLayout is also required when the selected list is merged into
another, I added another updateLayout call in listifyParagraph() for handling
this case as well.

However, as you rightly mention, we must ensure that the layout is up-to-date
before creating VisiblePositions from the indices. Perhaps we ought to have a
single updateLayout call before using the indices in ::doApply() itself.
I shall post another patch with this change.

> > The selection range for the existing test,
> insert-list-nested-with-orphaned.html changes 
> > with this fix. It now sets the selection over the entire list, as expected. 
> > The test initually sets the selection over the entire list which was later
> lost incorrectly 
> Nit: initially
Done.

> > after performing the insertOrderedList command. This is fixed now and thus
the
> selection 
> > over the contents is retained.
> Nit: this last sentence is redundant.
> 
Removed the last sentence.

>
https://codereview.chromium.org/114043002/diff/60001/Source/core/editing/Inse...
> File Source/core/editing/InsertListCommand.cpp (right):
> 
>
https://codereview.chromium.org/114043002/diff/60001/Source/core/editing/Inse...
> Source/core/editing/InsertListCommand.cpp:130: bool forceCreateList = false;
> nit: forceListCreation?
> 
Done.

>
https://codereview.chromium.org/114043002/diff/60001/Source/core/editing/Inse...
> Source/core/editing/InsertListCommand.cpp:399:
> document().updateLayoutIgnorePendingStylesheets();
> Why?
As mentioned earlier, I'll try to remove both the updateLayout calls from
listifyParagraph() and call an update layout before making use of the start and
end selection indices.

[arpitab_, 2014-01-16 07:24:42]

review pls?

[yosin_UTC9, 2014-01-16 08:19:49]

ACK

https://codereview.chromium.org/114043002/diff/80001/LayoutTests/editing/exec...
File
LayoutTests/editing/execCommand/selection-after-switch-type-of-listitem.html
(right):

https://codereview.chromium.org/114043002/diff/80001/LayoutTests/editing/exec...
LayoutTests/editing/execCommand/selection-after-switch-type-of-listitem.html:16:
Markup.description("This tests verifies that the selection is properly reset
after executing the command.");
nit: Please use single quote in JavaScript.

[arpitab_, 2014-01-16 10:40:09]

On 2014/01/16 08:19:49, Yoshi wrote:
> ACK
> 
>
https://codereview.chromium.org/114043002/diff/80001/LayoutTests/editing/exec...
> File
> LayoutTests/editing/execCommand/selection-after-switch-type-of-listitem.html
> (right):
> 
>
https://codereview.chromium.org/114043002/diff/80001/LayoutTests/editing/exec...
>
LayoutTests/editing/execCommand/selection-after-switch-type-of-listitem.html:16:
> Markup.description("This tests verifies that the selection is properly reset
> after executing the command.");
> nit: Please use single quote in JavaScript.

Thanks for the review Yoshi.
Uploaded another patch after fixing the nit.

[arpitabahuguna, 2014-06-24 14:26:04]

@leviw, @yosin, @yutak
Have uploaded an updated patch for this issue. PTAL.

[yosin_UTC9, 2014-06-27 02:15:46]

LGTM
Please wait for OWNERS review for committing.

Sorry for late response.
+tkent@ for OWNERS review.

[tkent, 2014-06-27 05:04:57]

rslgtm.

I recommend to wrap long paragraphs in the CL description.

https://codereview.chromium.org/114043002/diff/180001/Source/core/editing/Ins...
File Source/core/editing/InsertListCommand.cpp (right):

https://codereview.chromium.org/114043002/diff/180001/Source/core/editing/Ins...
Source/core/editing/InsertListCommand.cpp:142: // FIXME: This is an inefficient
way to keep selection alive because indexForVisiblePosition walks from
nit: recommend to wrap comments in 80-columns.

[arpitabahuguna, 2014-06-27 06:15:31]

Thank-you for the lgtm @yosi and @tkent! I'll upload another patch fixing the
nit.
Shall wrap the CL description as well.

https://codereview.chromium.org/114043002/diff/180001/Source/core/editing/Ins...
File Source/core/editing/InsertListCommand.cpp (right):

https://codereview.chromium.org/114043002/diff/180001/Source/core/editing/Ins...
Source/core/editing/InsertListCommand.cpp:142: // FIXME: This is an inefficient
way to keep selection alive because indexForVisiblePosition walks from
On 2014/06/27 05:04:57, tkent wrote:
> nit: recommend to wrap comments in 80-columns.

Done.

[arpitabahuguna, 2014-06-27 06:44:37]

The CQ bit was checked by a.bah@chromium.org

[commit-bot: I haz the power, 2014-06-27 06:45:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/a.bah@samsung.com/114043002/200001

[commit-bot: I haz the power, 2014-06-27 07:35:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[commit-bot: I haz the power, 2014-06-27 08:46:04]

Change committed as 177084