Make Editor::revealSelectionAfterEditingOperation check document availability

Make Editor::revealSelectionAfterEditingOperation check document availability

The above mentioned function is called to reveal selection after a user
initiated editing operation. However, it shouldn't proceed if the operation
destroys the frame, which is ensured by this patch.

BUG=685347
TEST=LayoutTests/editing/inserting/insert_linebreak_remove_frame.html

Review-Url: https://codereview.chromium.org/2653063004
Cr-Commit-Position: refs/heads/master@{#446275}
Committed: https://chromium.googlesource.com/chromium/src/+/4b7ec96e0cdf6ef721b15a31ea88d4c868f38929

[Xiaocheng, 2017-01-26 04:15:51]

The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 04:16:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2017-01-26 05:14:37]

xiaochengh@chromium.org changed reviewers:
+ yosin@chromium.org

[Xiaocheng, 2017-01-26 05:14:37]

PTAL.

ps. This issue seems a recent regression, as it doesn't repro in Stable. I have
no idea what caused the regression, though...

[yosin_UTC9, 2017-01-26 05:37:58]

On 2017/01/26 at 05:14:37, xiaochengh wrote:
> PTAL.
> 
> ps. This issue seems a recent regression, as it doesn't repro in Stable. I
have no idea what caused the regression, though...

Code change itself is reasonable since content modification can destroy document
hosing selection. 
But, I'm not sure why inserting line break causes "focusout"?

[Xiaocheng, 2017-01-26 05:52:48]

On 2017/01/26 at 05:37:58, yosin wrote:
> On 2017/01/26 at 05:14:37, xiaochengh wrote:
> > PTAL.
> > 
> > ps. This issue seems a recent regression, as it doesn't repro in Stable. I
have no idea what caused the regression, though...
> 
> Code change itself is reasonable since content modification can destroy
document hosing selection. 
> But, I'm not sure why inserting line break causes "focusout"?

Focus is moved from documentElement to body.

[commit-bot: I haz the power, 2017-01-26 05:58:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 05:58:20]

Dry run: This issue passed the CQ dry run.

[yosin_UTC9, 2017-01-26 06:08:17]

On 2017/01/26 at 05:52:48, xiaochengh wrote:
> On 2017/01/26 at 05:37:58, yosin wrote:
> > On 2017/01/26 at 05:14:37, xiaochengh wrote:
> > > PTAL.
> > > 
> > > ps. This issue seems a recent regression, as it doesn't repro in Stable. I
have no idea what caused the regression, though...
> > 
> > Code change itself is reasonable since content modification can destroy
document hosing selection. 
> > But, I'm not sure why inserting line break causes "focusout"?
> 
> Focus is moved from documentElement to body.

Does http://crrev.com/2628763003 cause this?
When focus is moved from documentElement to BODY?

[Xiaocheng, 2017-01-26 07:12:30]

On 2017/01/26 at 06:08:17, yosin wrote:
> On 2017/01/26 at 05:52:48, xiaochengh wrote:
> > On 2017/01/26 at 05:37:58, yosin wrote:
> > > On 2017/01/26 at 05:14:37, xiaochengh wrote:
> > > > PTAL.
> > > > 
> > > > ps. This issue seems a recent regression, as it doesn't repro in Stable.
I have no idea what caused the regression, though...
> > > 
> > > Code change itself is reasonable since content modification can destroy
document hosing selection. 
> > > But, I'm not sure why inserting line break causes "focusout"?
> > 
> > Focus is moved from documentElement to body.
> 
> Does http://crrev.com/2628763003 cause this?
> When focus is moved from documentElement to BODY?

Not relevant... After reverting that patch, the issue still repros.

[yosin_UTC9, 2017-01-26 07:24:38]

On 2017/01/26 at 07:12:30, xiaochengh wrote:
> On 2017/01/26 at 06:08:17, yosin wrote:
> > On 2017/01/26 at 05:52:48, xiaochengh wrote:
> > > On 2017/01/26 at 05:37:58, yosin wrote:
> > > > On 2017/01/26 at 05:14:37, xiaochengh wrote:
> > > > > PTAL.
> > > > > 
> > > > > ps. This issue seems a recent regression, as it doesn't repro in
Stable. I have no idea what caused the regression, though...
> > > > 
> > > > Code change itself is reasonable since content modification can destroy
document hosing selection. 
> > > > But, I'm not sure why inserting line break causes "focusout"?
> > > 
> > > Focus is moved from documentElement to body.
> > 
> > Does http://crrev.com/2628763003 cause this?
> > When focus is moved from documentElement to BODY?
> 
> Not relevant... After reverting that patch, the issue still repros.

The patch prevents to insert type character at selection if selection doesn't
have focus.
But, it doesn't check for "Enter" key. Should we check for "Enter" key in
Editor::handleEditingKeyboardEvent()?
If focus is moved before handling key, it can fix this issue.

[Xiaocheng, 2017-01-26 07:42:45]

On 2017/01/26 at 07:24:38, yosin wrote:
> On 2017/01/26 at 07:12:30, xiaochengh wrote:
> > On 2017/01/26 at 06:08:17, yosin wrote:
> > > On 2017/01/26 at 05:52:48, xiaochengh wrote:
> > > > On 2017/01/26 at 05:37:58, yosin wrote:
> > > > > On 2017/01/26 at 05:14:37, xiaochengh wrote:
> > > > > > PTAL.
> > > > > > 
> > > > > > ps. This issue seems a recent regression, as it doesn't repro in
Stable. I have no idea what caused the regression, though...
> > > > > 
> > > > > Code change itself is reasonable since content modification can
destroy document hosing selection. 
> > > > > But, I'm not sure why inserting line break causes "focusout"?
> > > > 
> > > > Focus is moved from documentElement to body.
> > > 
> > > Does http://crrev.com/2628763003 cause this?
> > > When focus is moved from documentElement to BODY?
> > 
> > Not relevant... After reverting that patch, the issue still repros.
> 
> The patch prevents to insert type character at selection if selection doesn't
have focus.
> But, it doesn't check for "Enter" key. Should we check for "Enter" key in
Editor::handleEditingKeyboardEvent()?
> If focus is moved before handling key, it can fix this issue.

I don't get it.

In this case, the selection does have focus when "Enter" is pressed. It loses
focus only after the handling.

Adding focus check in Editor::handleEditingKeyboardEvent before handling
shouldn't change the behavior in this repro case.

[yosin_UTC9, 2017-01-26 08:10:50]

On 2017/01/26 at 07:42:45, xiaochengh wrote:
> On 2017/01/26 at 07:24:38, yosin wrote:
> > On 2017/01/26 at 07:12:30, xiaochengh wrote:
> > > On 2017/01/26 at 06:08:17, yosin wrote:
> > > > On 2017/01/26 at 05:52:48, xiaochengh wrote:
> > > > > On 2017/01/26 at 05:37:58, yosin wrote:
> > > > > > On 2017/01/26 at 05:14:37, xiaochengh wrote:
> > > > > > > PTAL.
> > > > > > > 
> > > > > > > ps. This issue seems a recent regression, as it doesn't repro in
Stable. I have no idea what caused the regression, though...
> > > > > > 
> > > > > > Code change itself is reasonable since content modification can
destroy document hosing selection. 
> > > > > > But, I'm not sure why inserting line break causes "focusout"?
> > > > > 
> > > > > Focus is moved from documentElement to body.
> > > > 
> > > > Does http://crrev.com/2628763003 cause this?
> > > > When focus is moved from documentElement to BODY?
> > > 
> > > Not relevant... After reverting that patch, the issue still repros.
> > 
> > The patch prevents to insert type character at selection if selection
doesn't have focus.
> > But, it doesn't check for "Enter" key. Should we check for "Enter" key in
Editor::handleEditingKeyboardEvent()?
> > If focus is moved before handling key, it can fix this issue.
> 
> I don't get it.
> 
> In this case, the selection does have focus when "Enter" is pressed. It loses
focus only after the handling.
> 
> Adding focus check in Editor::handleEditingKeyboardEvent before handling
shouldn't change the behavior in this repro case.

Finally, I understand.
Focus is moved to BODY, after BR, by Editor::appliedEditing() then we call
|Editor::revealSelectionAfterEditingOperation()|.
So, you change is valid.

Thanks for explanation.

lgtm

[yosin_UTC9, 2017-01-26 08:10:53]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2017-01-26 08:11:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xiaocheng, 2017-01-26 08:13:06]

On 2017/01/26 at 08:10:50, yosin wrote:
> On 2017/01/26 at 07:42:45, xiaochengh wrote:
> > On 2017/01/26 at 07:24:38, yosin wrote:
> > > On 2017/01/26 at 07:12:30, xiaochengh wrote:
> > > > On 2017/01/26 at 06:08:17, yosin wrote:
> > > > > On 2017/01/26 at 05:52:48, xiaochengh wrote:
> > > > > > On 2017/01/26 at 05:37:58, yosin wrote:
> > > > > > > On 2017/01/26 at 05:14:37, xiaochengh wrote:
> > > > > > > > PTAL.
> > > > > > > > 
> > > > > > > > ps. This issue seems a recent regression, as it doesn't repro in
Stable. I have no idea what caused the regression, though...
> > > > > > > 
> > > > > > > Code change itself is reasonable since content modification can
destroy document hosing selection. 
> > > > > > > But, I'm not sure why inserting line break causes "focusout"?
> > > > > > 
> > > > > > Focus is moved from documentElement to body.
> > > > > 
> > > > > Does http://crrev.com/2628763003 cause this?
> > > > > When focus is moved from documentElement to BODY?
> > > > 
> > > > Not relevant... After reverting that patch, the issue still repros.
> > > 
> > > The patch prevents to insert type character at selection if selection
doesn't have focus.
> > > But, it doesn't check for "Enter" key. Should we check for "Enter" key in
Editor::handleEditingKeyboardEvent()?
> > > If focus is moved before handling key, it can fix this issue.
> > 
> > I don't get it.
> > 
> > In this case, the selection does have focus when "Enter" is pressed. It
loses focus only after the handling.
> > 
> > Adding focus check in Editor::handleEditingKeyboardEvent before handling
shouldn't change the behavior in this repro case.
> 
> Finally, I understand.
> Focus is moved to BODY, after BR, by Editor::appliedEditing() then we call
|Editor::revealSelectionAfterEditingOperation()|.
> So, you change is valid.
> 
> Thanks for explanation.
> 
> lgtm

Thanks for the review!

[commit-bot: I haz the power, 2017-01-26 08:15:13]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1485418253936700, "parent_rev":
"80b5c8770dd94e9127ca7afd6cf314132ed8228e", "commit_rev":
"4b7ec96e0cdf6ef721b15a31ea88d4c868f38929"}

[commit-bot: I haz the power, 2017-01-26 08:15:48]

Description was changed from

==========
Make Editor::revealSelectionAfterEditingOperation check document availability

The above mentioned function is called to reveal selection after a user
initiated editing operation. However, it shouldn't proceed if the operation
destroys the frame, which is ensured by this patch.

BUG=685347
TEST=LayoutTests/editing/inserting/insert_linebreak_remove_frame.html
==========

to

==========
Make Editor::revealSelectionAfterEditingOperation check document availability

The above mentioned function is called to reveal selection after a user
initiated editing operation. However, it shouldn't proceed if the operation
destroys the frame, which is ensured by this patch.

BUG=685347
TEST=LayoutTests/editing/inserting/insert_linebreak_remove_frame.html

Review-Url: https://codereview.chromium.org/2653063004
Cr-Commit-Position: refs/heads/master@{#446275}
Committed:
https://chromium.googlesource.com/chromium/src/+/4b7ec96e0cdf6ef721b15a31ea88...
==========

[commit-bot: I haz the power, 2017-01-26 08:15:49]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/4b7ec96e0cdf6ef721b15a31ea88...