Bug 107850 – Make moveCaretTowardsWindowPoint not snap to the beginning/end when moved above/below editable

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Hopman]

Make moveCaretTowardsWindowPoint not snap to the beginning/end when moved above/below editable

[Chris Hopman]

Created attachment 184556 [details]
Patch

[Alexey Proskuryakov]

This doesn't have a regression test. Is there no behavior change?

[Chris Hopman]

Created attachment 184834 [details]
Patch

[Chris Hopman]

(In reply to comment #2)
> This doesn't have a regression test. Is there no behavior change?

Added a test for the changed behavior.

[Ryosuke Niwa]

Comment on attachment 184834 [details]
Patch

I don't understand what you're trying to to do at all but I don't like the code being added to VisiblePosition here.

[Ojan Vafai]

Comment on attachment 184834 [details]
Patch

Can we fix this by adding EditingAndroidBehavior and making it do the same as EditingWindowsBehavior in the right places or does that not work for some reason?

[Ryosuke Niwa]

(In reply to comment #6)
> (From update of attachment 184834 [details])
> Can we fix this by adding EditingAndroidBehavior and making it do the same as EditingWindowsBehavior in the right places or does that not work for some reason?

That seems like an acceptable solution although it means that we have to update the existing editing layout tests to test this editing behavior as well.

[Chris Hopman]

(In reply to comment #6)
> (From update of attachment 184834 [details])
> Can we fix this by adding EditingAndroidBehavior and making it do the same as EditingWindowsBehavior in the right places or does that not work for some reason?

That would be nice... I tried that first: https://bugs.webkit.org/show_bug.cgi?id=107171

Really though, moveCaret should have that behavior on all platforms that use it. Currently, only Android uses this function so adding the editing behavior would work. In the future, if another platform wanted to use the function, it would be impractical to expect that platform to change their EditingBehavior.

rniwa@ mentioned the possibility of having positionForPoint optionally take a flag for shouldMoveCaretTo[...]. Currently EditingBehavior::shouldMoveCaret[...] is used in RenderBlock::positionForPointWithInlineChildren and its setting completely changes the behavior of RenderObject::positionForPoint. It makes sense to me for that to actually be more exposed/obvious to callers of positionForPoint. What do you think?

[Ojan Vafai]

(In reply to comment #8)
> (In reply to comment #6)
> > (From update of attachment 184834 [details] [details])
> > Can we fix this by adding EditingAndroidBehavior and making it do the same as EditingWindowsBehavior in the right places or does that not work for some reason?
> 
> That would be nice... I tried that first: https://bugs.webkit.org/show_bug.cgi?id=107171

Sigh. I agree with Darin obviously (Darin and I came up with the concept EditingBehavior in the first place). Android is a different OS and has different editing behavior. I don't see what the problem is with adding a new editing behavior is. It's not like we get a new OS that often.

> Really though, moveCaret should have that behavior on all platforms that use it. 

This is not clear to me. It seems like it should just respect whatever editing behavior you pick. Why would another platform choose to use this but not want their platform's normal editing behavior?

> rniwa@ mentioned the possibility of having positionForPoint optionally take a flag for shouldMoveCaretTo[...]. Currently EditingBehavior::shouldMoveCaret[...] is used in RenderBlock::positionForPointWithInlineChildren and its setting completely changes the behavior of RenderObject::positionForPoint. It makes sense to me for that to actually be more exposed/obvious to callers of positionForPoint. What do you think?

Adding a new EditingBehavior is the clear correct approach here IMO. We should just do that as per your first patch on bug 107171. Please upload that patch + tests and I will approve it. Sorry you've gotten the runaround on this.

[Levi Weintraub]

(In reply to comment #9)
> Adding a new EditingBehavior is the clear correct approach here IMO. We should just do that as per your first patch on bug 107171. Please upload that patch + tests and I will approve it. Sorry you've gotten the runaround on this.

This was my belief from the start as well. Android has its own editing behavior and should be defined as such.

[Ryosuke Niwa]

If you're adding a new editing behavior, please be sure to update existing layout tests in LayoutTests/editing that manually set editing behavior.

[Ryosuke Niwa]

Comment on attachment 184834 [details]
Patch

I'm gonna say r-. I don't think it makes sense to add this code to VisiblePosition from the discussion we've had so far.

[Ojan Vafai]

(In reply to comment #11)
> If you're adding a new editing behavior, please be sure to update existing layout tests in LayoutTests/editing that manually set editing behavior.

I don't see why this needs to be a prerequisite to landing this patch.

[Ryosuke Niwa]

(In reply to comment #13)
> (In reply to comment #11)
> > If you're adding a new editing behavior, please be sure to update existing layout tests in LayoutTests/editing that manually set editing behavior.
> 
> I don't see why this needs to be a prerequisite to landing this patch.

It needs to be because the whole point of those tests is that they test all editing behaviors.

[Ryosuke Niwa]

There are less than 40 tests that call setEditingBehavior, of which only few of them really test all editing behaviors so I don't think this is a unfair/unreasonable requirement at all.

[Ojan Vafai]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > If you're adding a new editing behavior, please be sure to update existing layout tests in LayoutTests/editing that manually set editing behavior.
> > 
> > I don't see why this needs to be a prerequisite to landing this patch.
> 
> It needs to be because the whole point of those tests is that they test all editing behaviors.

I agree that it would be nice to do, but I don't see any benefit from coupling it with this patch.

[Ryosuke Niwa]

(In reply to comment #16)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #11)
> > > > If you're adding a new editing behavior, please be sure to update existing layout tests in LayoutTests/editing that manually set editing behavior.
> > > 
> > > I don't see why this needs to be a prerequisite to landing this patch.
> > 
> > It needs to be because the whole point of those tests is that they test all editing behaviors.
> 
> I agree that it would be nice to do, but I don't see any benefit from coupling it with this patch.

It's not nice to do. It must be done when a new editing behavior is added. That's what we did when we added 'unix' editing behavior.

[Ryosuke Niwa]

I just don't know why you're so opposed to the idea of requiring this change when we're adding new editing behavior. But whatever. I'll just write a patch to do it myself and upload it somewhere.

So please stop arguing about this.

[Chris Hopman]

Created attachment 185909 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Ryosuke Niwa]

Created attachment 185955 [details]
Updated more tests

[Chris Hopman]

Created attachment 186487 [details]
Patch

[WebKit Review Bot]

Comment on attachment 186487 [details]
Patch

Clearing flags on attachment: 186487

Committed r141837: <http://trac.webkit.org/changeset/141837>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.