Make moveCaretTowardsWindowPoint not snap to the beginning/end when moved above/below editable
Created attachment 184556 [details] Patch
This doesn't have a regression test. Is there no behavior change?
Created attachment 184834 [details] Patch
(In reply to comment #2) > This doesn't have a regression test. Is there no behavior change? Added a test for the changed behavior.
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.
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?
(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.
(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?
(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.
(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.
If you're adding a new editing behavior, please be sure to update existing layout tests in LayoutTests/editing that manually set editing behavior.
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.
(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.
(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.
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.
(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.
(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.
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.
Created attachment 185909 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Created attachment 185955 [details] Updated more tests
Created attachment 186487 [details] Patch
Comment on attachment 186487 [details] Patch Clearing flags on attachment: 186487 Committed r141837: <http://trac.webkit.org/changeset/141837>
All reviewed patches have been landed. Closing bug.