Bug 33413 – selection.modify extends too far with 'lineboundary'

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ojan Vafai]

Created attachment 46187 [details]
test case

This was noticed in https://bugzilla.mozilla.org/show_bug.cgi?id=496275, where Gecko is adding DOMSelection.modify to match WebKit. See the test case for the bug. After calling sel.modify('extend', 'forward', 'lineboundary') once, subsequent calls should be a noop. Instead, each call extends one more line when there's line-wrapping involved.

[Darin Adler]

We need to be sure not to change the behavior of key bindings on Mac OS X in unwanted ways. The described behavior sounds like what the Mac OS X Command-Shift-Right-Arrow key does, and the AppKit moveToRightEndOfLineAndModifySelection: method. We invented selection.modify while working on the original Mac OS X version of WebKit it's highly likely the programmer who created it did intend this behavior.

[Darin Adler]

I take it back.

I tested Command-Shift-Right-Arrow in AppKit text editing, and this behavior in WebKit seems to simply be a bug.

[Darin Adler]

In fact, I suspect this is a relatively recent regression.

[Hajime Morrita]

Created attachment 50376 [details]
patch v0

[Ojan Vafai]

Comment on attachment 50376 [details]
patch v0

> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,27 @@
> [phone]  MORITA Hajime  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Selection.modify extends too far with 'lineboundary'.
> +        https://bugs.webkit.org/show_bug.cgi?id=33413
> +
> +        Selection.modify() with 'lineboundary' guranuality implies just
> +        "Go to the end of the line", unlike selection expansioin with
> +        other type of granularities. This change handled LineGranularity
> +        as special case, to look-up end of line with UPSTREAM affinity. 
> +        Doing this prevents look-up algorithm to go next line.
> +        This change also fixed getInlineBoxAndOffset() that didn't handle
> +        an edge case for for UPSTREAM.

Nit: you repeat this last line below. Only need to put it in one place. Either place is fine.

> +
> +        Re-baseline: editing/selection/extend-selection-expected.txt
> +
> +        * dom/Position.cpp:
> +        (WebCore::Position::getInlineBoxAndOffset):
> +        Handled an edge case for node lookup with UPSTREAM.
> +        * editing/SelectionController.cpp:
> +        (WebCore::SelectionController::modifyExtendingForward):
> +        Added UPSTREAM tweak for the case for LineGranularity.

Nit: This line is redundant with the change description above.

I'm definitely nit-picking here, but you only need to annotate specific files/methods if there's something extra worth noting. Or, you can leave the change description minimal and annotate the files/methods instead.

The code changes seem reasonable to me. Does modifyMovingForward also need to be fixed? What about sentenceboundary/paragraphboundary? I'm not really sure what sentenceboundary/paragraphboundary are used for, i.e. do they correspond to a user-action? If they do, it would be good to compare TextEdit and WebKit to see if this needs to be done for those cases as well.

[Alexey Proskuryakov]

Comment on attachment 50376 [details]
patch v0

+             * So we mimic UPSTREAM affinity to prevent to crossing
+             * line when the offset is at the end of line.

Two comments:
1) We use C++ style comments ("//") in C++ code.
2) I don't understand why this says "mimic". Don't we just plain use it? And would anything break if you didn't add "pos.setAffinity(m_selection.affinity());"?

> Or, you can leave the change description minimal and annotate the files/methods instead.

Yes, that's the preferred way of writing ChangeLogs.

> +        Re-baseline: editing/selection/extend-selection-expected.txt

"Baseline" is a noun, so using it as a verb is a mistake. You could just say "updated results" instead.

> I'm not really sure what sentenceboundary/paragraphboundary are used for, 
> i.e. do they correspond to a user-action?

Without actually checking the code, I suspect that you get paragraphboundary when triple-clicking and dragging, and also when triple-clicking and then extending the selection from keyboard with Cmd+Shift+Right (but when I tested, the results from the latter seemed wrong).

I think this needs another iteration, r- for now. I agree with Ojan's comments about code changes.

[Ojan Vafai]

(In reply to comment #6)
> > I'm not really sure what sentenceboundary/paragraphboundary are used for, 
> > i.e. do they correspond to a user-action?
> 
> Without actually checking the code, I suspect that you get paragraphboundary
> when triple-clicking and dragging, and also when triple-clicking and then
> extending the selection from keyboard with Cmd+Shift+Right (but when I tested,
> the results from the latter seemed wrong).

I played with this a bit more. ctrl+shift+e extends the selection to the end of the current paragraph and ctrl+shift+a will extend the selection to the beginning of the current paragraph. I tested this in WebKit and TextEdit and the behavior seems to be the same. So, I think this patch is fine as far as paragraphboundary is concerned, which makes sense since upstream/downstream is not really meaningful at a paragraph boundary.

As far as sentenceboundary, I think it probably does not need the same treatment as lineboundary, but I really don't know what the expected behavior of sentenceboundary is. I've sent and email to webkit-dev to try and understand this better.

Also, I tested the modifyMovingForward case (cmd+left and cmd+right). They work in the testcase. It does not need to be modified because VisibleSelection::setStartAndEndFromBaseAndExtentRespectingGranularity does nothing for CharacterGranularity. It's that function that causes this bug when you have the granularity set to LineGranularity.

I've answered my own questions. :) The code seems fine to me, except for ap's question about whether we really need to set the affinity back.

[Hajime Morrita]

Created attachment 50465 [details]
v1; took feedbacks

[Hajime Morrita]

Thank you for the review and further experiments! > ojan, ap
I update patch to cleanup changelog/comment mess.
Would you give the review for updated one?

>2) I don't understand why this says "mimic". Don't we just plain use it? And
>would anything break if you didn't add
>"pos.setAffinity(m_selection.affinity());"?
I agree that isn't a mimicking stuff.

At first I thought I should keep affinity as before modify() 
and swapping affinity temporally looks like "mimic" for me.
But all tests passed without that line so I removed it,
and now it looks natural for me. 

>Also, I tested the modifyMovingForward case (cmd+left and cmd+right). They work
>in the testcase.
Yes. modify('move', ... ) already works as expected.
It seems 'move' has its own logic other than 'extend' for line-boundary behavior.

[Ojan Vafai]

Comment on attachment 50465 [details]
v1; took feedbacks

> +++ b/WebCore/ChangeLog
> +        Selection.modify() with 'lineboundary' guranuality implies just
> +        "Go to the end of the line", unlike selection expansioin with

Typo: s/expansioin/expansion

> +++ b/WebCore/editing/SelectionController.cpp
> +            // LineBoundary granularity actually emulates
> +            // NSTextView#moveToRightEndOfLineAndModifySelection(),
> +            // which never expands selection across multiple lines.
> +            // So we set UPSTREAM affinity to prevent to crossing
> +            // line when the offset is at the end of line.

Grammar fix:
> +            // which never expands a selection across multiple lines.
> +            // So we set UPSTREAM affinity to prevent crossing the
> +            // line boundary when the offset is at the end of the line.

Otherwise look good to me. I'm not a reviewer though. :)

[Hajime Morrita]

Created attachment 50469 [details]
v2; spelling fix

[Hajime Morrita]

(In reply to comment #10)
> Typo: s/expansioin/expansion
> ...
> Grammar fix:

Oops, I need spell/grammar-checker for my Emacs.
Thank you for pointing them out!

[Hajime Morrita]

Created attachment 50470 [details]
v3; more misspelling fix

[Hajime Morrita]

...and ispell on Emacs found more misspelling :-(

[Enrica Casucci]

Comment on attachment 50470 [details]
v3; more misspelling fix

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index 9d47289..d30ce63 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,15 @@
> [phone]  MORITA Hajime  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Selection.modify extends too far with 'lineboundary'.
> +        https://bugs.webkit.org/show_bug.cgi?id=33413
> +
> +        * editing/selection/extend-selection-expected.txt:
> +        Updated result to correct expectation that described the wrong
> +        behaviour for the selection expansion with 'lineboundary'
> +        granularity.
> +
>  2010-03-10  Roland Steiner  <[e-mail]>
>  
>          Reviewed by David Levin.
> diff --git a/LayoutTests/editing/selection/extend-selection-expected.txt b/LayoutTests/editing/selection/extend-selection-expected.txt
> index 1617e03..0b33cd2 100644
> --- a/LayoutTests/editing/selection/extend-selection-expected.txt
> +++ b/LayoutTests/editing/selection/extend-selection-expected.txt
> @@ -514,14 +514,14 @@ Test 20, RTL:
>    Extending forward: "ABC abc DEF"[(0,0), (0,11)]
>    Extending backward:  "ABC abc DEF"[(0,11), (0,0)]
>  Test 21, LTR:
> -  Extending forward: "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,0), (0,8), (0,16), (0,26), (0,34), (0,42), (0,50), (0,58), (0,66), (0,74), (0,82), (0,90), (0,98), (0,105)]
> -  Extending backward:  "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,105), (0,0)]
> +  Extending forward: "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,0), (0,8)]
> +  Extending backward:  "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,8), (0,0)]
>  Test 21, RTL:
>    Extending forward: "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,0), (0,7)]
>    Extending backward:  "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,7), (0,0)]
>  Test 22, LTR:
> -  Extending forward: "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,0), (0,8), (0,16), (0,26), (0,34), (0,42), (0,50), (0,58), (0,66), (0,74), (0,82), (0,90), (0,98), (0,105)]
> -  Extending backward:  "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,105), (0,0)]
> +  Extending forward: "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,0), (0,8)]
> +  Extending backward:  "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,8), (0,0)]
>  Test 22, RTL:
>    Extending forward: "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,0), (0,7)]
>    Extending backward:  "abcdefg abcdefg abcdefg a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg "[(0,7), (0,0)]
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 0a812be..afda4f5 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,25 @@
> [phone]  MORITA Hajime  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Selection.modify extends too far with 'lineboundary'.
> +        https://bugs.webkit.org/show_bug.cgi?id=33413
> +
> +        Selection.modify() with 'lineboundary' granularity implies just
> +        "Go to the end of the line", unlike selection expansion with
> +        other type of granularities. This change handled LineGranularity
> +        as special case, to look-up end of line with UPSTREAM affinity. 
> +        Doing this prevents look-up algorithm to go next line.
> +
> +        Test: editing/selection/extend-selection-expected.txt
> +
> +        * dom/Position.cpp:
> +        (WebCore::Position::getInlineBoxAndOffset):
> +        Handled an edge case for node look-up with UPSTREAM.
> +        * editing/SelectionController.cpp:
> +        (WebCore::SelectionController::modifyExtendingForward):
> +        Added UPSTREAM tweak for the case for LineGranularity.
> +        
>  2010-03-10  Roland Steiner  <[e-mail]>
>  
>          Unreviewed build fix. Fix variable name change that somehow didn't
> diff --git a/WebCore/dom/Position.cpp b/WebCore/dom/Position.cpp
> index ef0b3c1..e22dc9e 100644
> --- a/WebCore/dom/Position.cpp
> +++ b/WebCore/dom/Position.cpp
> @@ -1041,6 +1041,9 @@ void Position::getInlineBoxAndOffset(EAffinity affinity, TextDirection primaryDi
>                  return;
>              }
>  
> +            if ((caretOffset == caretMaxOffset) ^ (affinity == DOWNSTREAM))
> +                break;
> +

Why this? affinity is only UPSTREAM or DOWNSTREAM.
You could change the code to be
             if (caretOffset == caretMinOffset)
                  break;
and have only one if statement instead of 2.
   
>              if ((caretOffset == caretMinOffset) ^ (affinity == UPSTREAM))
>                  break;
>  
> diff --git a/WebCore/editing/SelectionController.cpp b/WebCore/editing/SelectionController.cpp
> index c0f03de..f2859fd 100644
> --- a/WebCore/editing/SelectionController.cpp
> +++ b/WebCore/editing/SelectionController.cpp
> @@ -351,7 +351,14 @@ VisiblePosition SelectionController::modifyExtendingForward(TextGranularity gran
>              pos = endOfSentence(endForPlatform());
>              break;
>          case LineBoundary:
> -            pos = logicalEndOfLine(endForPlatform());
> +            // LineBoundary granularity actually emulates
> +            // NSTextView#moveToRightEndOfLineAndModifySelection(),
> +            // which never expands a selection across multiple lines.
> +            // So we set UPSTREAM affinity to prevent crossing the
> +            // line boundary when the offset is at the end of the line.
> +            pos = endForPlatform();
> +            pos.setAffinity(UPSTREAM);
> +            pos = logicalEndOfLine(pos);
>              break;
>          case ParagraphBoundary:
>              pos = endOfParagraph(endForPlatform());

[Ojan Vafai]

(In reply to comment #15)
> > +            if ((caretOffset == caretMaxOffset) ^ (affinity == DOWNSTREAM))
> > +                break;
> > +
> 
> Why this? affinity is only UPSTREAM or DOWNSTREAM.
> You could change the code to be
>              if (caretOffset == caretMinOffset)
>                   break;
> and have only one if statement instead of 2.

I think maybe you misread? The comparison above is to caretMaxOffset, not caretMinOffset.

> 
> >              if ((caretOffset == caretMinOffset) ^ (affinity == UPSTREAM))
> >                  break;

[Enrica Casucci]

(In reply to comment #16)
> (In reply to comment #15)
> > > +            if ((caretOffset == caretMaxOffset) ^ (affinity == DOWNSTREAM))
> > > +                break;
> > > +
> > 
> > Why this? affinity is only UPSTREAM or DOWNSTREAM.
> > You could change the code to be
> >              if (caretOffset == caretMinOffset)
> >                   break;
> > and have only one if statement instead of 2.
> 
> I think maybe you misread? The comparison above is to caretMaxOffset, not
> caretMinOffset.
> 
> > 
> > >              if ((caretOffset == caretMinOffset) ^ (affinity == UPSTREAM))
> > >                  break;

My mistake, sorry. I would still prefer to see only one if with an or rather than 2 if statements with a break. Not a big deal.

[Hajime Morrita]

Created attachment 50565 [details]
brought 2 break statements into one

[Alexey Proskuryakov]

Comment on attachment 50565 [details]
brought 2 break statements into one

Code changes look good to me. I have some nits about the comment though.

+            // LineBoundary granularity actually emulates

"Actually" doesn't mean anything here, and can be just omitted.

+            // NSTextView#moveToRightEndOfLineAndModifySelection(),

I don't see such a method in NSTextView. If it were there, it would have been written as "-[NSTextView moveToRightEndOfLineAndModifySelection:]". But the important thing is that we try to match NSTextView behavior for user actions, not the behavior of some low level method.

+            // which never expands a selection across multiple lines.
+            // So we set UPSTREAM affinity to prevent crossing the
+            // line boundary when the offset is at the end of the line.

I don't think this comment is necessary. The behavior is covered by a regression test, so it's unlikely that someone will break it, and if rationale for the change is needed in the future, svn blame can always be consulted.

These test has platform/win results, too, and they also need to be updated. I don't know why these are different, but the expected results file should be hopefully easy to patch even if you don't have a Windows development environment.

r=me if this whole comment is removed when landing, and Windows results are updated. Or you can submit an updated patch for double-checking and commit queue.

[Darin Adler]

(In reply to comment #19)
> +            // NSTextView#moveToRightEndOfLineAndModifySelection(),
> 
> I don't see such a method in NSTextView. If it were there, it would have been
> written as "-[NSTextView moveToRightEndOfLineAndModifySelection:]". But the
> important thing is that we try to match NSTextView behavior for user actions,
> not the behavior of some low level method.

Just for the record, -[NSResponder moveToRightEndOfLineAndModifySelection:] does indeed exist. See NSResponder.h.

[Alexey Proskuryakov]

I don't see it in Leopard version of NSResponder.h or in Leopard Xcode documentation.

[Darin Adler]

(In reply to comment #21)
> I don't see it in Leopard version of NSResponder.h or in Leopard Xcode
> documentation.

It’s new in Snow Leopard.

[Hajime Morrita]

Created attachment 50687 [details]
clenaup comment, update windows result

[Hajime Morrita]

Thank you for reviewing again.
I've updated the patch to remove redundant part from the comment,
updating the windows result.
Would you review it?

BTW, The behavior on windows looks a little strange.
I suspect that It is why that test on windows has different expectation.
We might need to tackle it separately.

[Alexey Proskuryakov]

Comment on attachment 50687 [details]
clenaup comment, update windows result

> +            // LineBoundary granularity emulates
> +            // [NSTextView moveToRightEndOfLineAndModifySelection:].

You didn't fully address my comments here - it's better to talk about user-visible behavior than about low level methods, and the dash sign is an integral part of method name. I still think we'd be better without any comment.

r=me

[Hajime Morrita]

Created attachment 50692 [details]
remove redundant comment

[Hajime Morrita]

Comment on attachment 50692 [details]
remove redundant comment

> You didn't fully address my comments here
Oops, I'm sorry about that. Thank you for granting.

> integral part of method name. I still think we'd be better without any comment.
Agreed. So I removed the whole comment at that part.

Would you (or someone) commit-queue:+ when bots get green ?

[WebKit Commit Bot]

Comment on attachment 50692 [details]
remove redundant comment

Clearing flags on attachment: 50692

Committed r55990: <http://trac.webkit.org/changeset/55990>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.