Bug 36256 – shift+click to extend a wordgranularity selection backwards selects the space after

• Component: HTML Editing
• Status: NEW

[Ojan Vafai]

Given "foo bar baz" double-click on bar, then shift+click on foo. You should see "foo bar" selected. Instead "foo bar " is selected. The space after bar gets selected and shouldn't.

[Van Lam]

Created attachment 104955 [details]
Proposed fix

[Ryosuke Niwa]

Comment on attachment 104955 [details]
Proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=104955&action=review

> Source/WebCore/editing/VisibleSelection.cpp:300
> +            if (isEndOfDocument(originalEnd) || (isEndOfLine(originalEnd) && !isStartOfLine(originalEnd) && !isEndOfParagraph(originalEnd)) || (isEndOfWord(originalEnd) && start != originalEnd && m_extent != originalEnd))

I'd split it into two lines since this is really long line.  ap & mitz should probably look at this change.

> Source/WebCore/editing/visible_units.cpp:288
> +    if (c.isNull())
> +        return false;
> +    
> +    VisiblePosition prev = previousWordPosition(c);
> +    return prev.isNotNull() && endOfWord(prev, RightWordIfOnBoundary) == c;

So we can't just do endOfWord(c, RightWordIfOnBoundary) == c ?

[Alexey Proskuryakov]

Duplicate of bug 65999?

[Van Lam]

(In reply to comment #2)
> (From update of attachment 104955 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104955&action=review
> 
> > Source/WebCore/editing/VisibleSelection.cpp:300
> > +            if (isEndOfDocument(originalEnd) || (isEndOfLine(originalEnd) && !isStartOfLine(originalEnd) && !isEndOfParagraph(originalEnd)) || (isEndOfWord(originalEnd) && start != originalEnd && m_extent != originalEnd))
> 
> I'd split it into two lines since this is really long line.  ap & mitz should probably look at this change.
> 
> > Source/WebCore/editing/visible_units.cpp:288
> > +    if (c.isNull())
> > +        return false;
> > +    
> > +    VisiblePosition prev = previousWordPosition(c);
> > +    return prev.isNotNull() && endOfWord(prev, RightWordIfOnBoundary) == c;
> 
> So we can't just do endOfWord(c, RightWordIfOnBoundary) == c ?

No, if c is at the end of a word, endOfWord(c, RightWordIfOnBoundary) returns the end of the following word.

[Van Lam]

(In reply to comment #3)
> Duplicate of bug 65999?

After taking a look at your patch, yes these are duplicate bugs.

[Alexey Proskuryakov]

It's possible that I went overboard trying to fix everything at once in bug 65999, but as you can see from discussion there, the situation is fairly complicated. We don't even quite know what the desired behavior is.

[Van Lam]

(In reply to comment #6)
> It's possible that I went overboard trying to fix everything at once in bug 65999, but as you can see from discussion there, the situation is fairly complicated. We don't even quite know what the desired behavior is.

Definitely; I ran into several of the same problems involving what the expected behavior is when attempting to fix this bug. But do you think the behavior mentioned in the title of this bug is not expected and should be fixed for now?

[Ryosuke Niwa]

Comment on attachment 104955 [details]
Proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=104955&action=review

>>> Source/WebCore/editing/VisibleSelection.cpp:300

>> 
>> I'd split it into two lines since this is really long line.  ap & mitz should probably look at this change.
> 
> No, if c is at the end of a word, endOfWord(c, RightWordIfOnBoundary) returns the end of the following word.

Why do we have to compare m_extent with originalEnd?

[Van Lam]

(In reply to comment #8)
> (From update of attachment 104955 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104955&action=review
> 
> >>> Source/WebCore/editing/VisibleSelection.cpp:300
> 
> >> 
> >> I'd split it into two lines since this is really long line.  ap & mitz should probably look at this change.
> > 
> > No, if c is at the end of a word, endOfWord(c, RightWordIfOnBoundary) returns the end of the following word.
> 
> Why do we have to compare m_extent with originalEnd?

Given that I'm checking isEndOfWord(originalEnd) && start != originalEnd, without doing the additional m_extent != originalEnd check, word-granularity mouse selections don't work correctly. What happens is given:

foo bar baz

If you double click in 'bar' and drag the mouse to the right to the position after the r in 'bar, it should grow the selection to include the space after 'bar' (in general the next word); it doesn't grow the selection if I omit the check in question.

I do admit it's not very clear what this check is doing; I'll try to find a more clear way to do this check. I initially saw that the first check (if originalEnd is at the end of a word) would fix the problem and added the other two checks to keep other behavior the same.

[Ryosuke Niwa]

Comment on attachment 104955 [details]
Proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=104955&action=review

>>>>> Source/WebCore/editing/VisibleSelection.cpp:300
>>>>> +            if (isEndOfDocument(originalEnd) || (isEndOfLine(originalEnd) && !isStartOfLine(originalEnd) && !isEndOfParagraph(originalEnd)) || (isEndOfWord(originalEnd) && start != originalEnd && m_extent != originalEnd))
>>>> 
>>>> I'd split it into two lines since this is really long line.  ap & mitz should probably look at this change.
>>> 
>>> No, if c is at the end of a word, endOfWord(c, RightWordIfOnBoundary) returns the end of the following word.
>> 
>> 
> 
> Given that I'm checking isEndOfWord(originalEnd) && start != originalEnd, without doing the additional m_extent != originalEnd check, word-granularity mouse selections don't work correctly. What happens is given:
> 
> foo bar baz
> 
> If you double click in 'bar' and drag the mouse to the right to the position after the r in 'bar, it should grow the selection to include the space after 'bar' (in general the next word); it doesn't grow the selection if I omit the check in question.
> 
> I do admit it's not very clear what this check is doing; I'll try to find a more clear way to do this check. I initially saw that the first check (if originalEnd is at the end of a word) would fix the problem and added the other two checks to keep other behavior the same.

Shouldn't we be comparing it with m_end then?

[Van Lam]

(In reply to comment #10)
> (From update of attachment 104955 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104955&action=review
> 
> >>>>> Source/WebCore/editing/VisibleSelection.cpp:300
> >>>>> +            if (isEndOfDocument(originalEnd) || (isEndOfLine(originalEnd) && !isStartOfLine(originalEnd) && !isEndOfParagraph(originalEnd)) || (isEndOfWord(originalEnd) && start != originalEnd && m_extent != originalEnd))
> >>>> 
> >>>> I'd split it into two lines since this is really long line.  ap & mitz should probably look at this change.
> >>> 
> >>> No, if c is at the end of a word, endOfWord(c, RightWordIfOnBoundary) returns the end of the following word.
> >> 
> >> 
> > 
> > Given that I'm checking isEndOfWord(originalEnd) && start != originalEnd, without doing the additional m_extent != originalEnd check, word-granularity mouse selections don't work correctly. What happens is given:
> > 
> > foo bar baz
> > 
> > If you double click in 'bar' and drag the mouse to the right to the position after the r in 'bar, it should grow the selection to include the space after 'bar' (in general the next word); it doesn't grow the selection if I omit the check in question.
> > 
> > I do admit it's not very clear what this check is doing; I'll try to find a more clear way to do this check. I initially saw that the first check (if originalEnd is at the end of a word) would fix the problem and added the other two checks to keep other behavior the same.
> 
> Shouldn't we be comparing it with m_end then?

originalEnd is declared:

VisiblePosition originalEnd(m_end, m_affinity); and is not modified until my check, so there shouldn't be a difference.

[Ryosuke Niwa]

Comment on attachment 104955 [details]
Proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=104955&action=review

>>>>>>> Source/WebCore/editing/VisibleSelection.cpp:300
>>>>>>> +            if (isEndOfDocument(originalEnd) || (isEndOfLine(originalEnd) && !isStartOfLine(originalEnd) && !isEndOfParagraph(originalEnd)) || (isEndOfWord(originalEnd) && start != originalEnd && m_extent != originalEnd))
>>>>>> 
>>>>>> I'd split it into two lines since this is really long line.  ap & mitz should probably look at this change.
>>>>> 
>>>>> No, if c is at the end of a word, endOfWord(c, RightWordIfOnBoundary) returns the end of the following word.
>>>> 
>>>> 
>>> 
>>> Given that I'm checking isEndOfWord(originalEnd) && start != originalEnd, without doing the additional m_extent != originalEnd check, word-granularity mouse selections don't work correctly. What happens is given:
>>> 
>>> foo bar baz
>>> 
>>> If you double click in 'bar' and drag the mouse to the right to the position after the r in 'bar, it should grow the selection to include the space after 'bar' (in general the next word); it doesn't grow the selection if I omit the check in question.
>>> 
>>> I do admit it's not very clear what this check is doing; I'll try to find a more clear way to do this check. I initially saw that the first check (if originalEnd is at the end of a word) would fix the problem and added the other two checks to keep other behavior the same.
>> 
>> Shouldn't we be comparing it with m_end then?
> 
> originalEnd is declared:
> 
> VisiblePosition originalEnd(m_end, m_affinity); and is not modified until my check, so there shouldn't be a difference.

But you see
    if (m_baseIsFirst) {
        m_start = m_base;
        m_end = m_extent;
    } else {
        m_start = m_extent;
        m_end = m_base;
    }
above, right?

[Ryosuke Niwa]

Depending on isBaseFirst() strikes me as a bad condition.

[Van Lam]

(In reply to comment #12)
> (From update of attachment 104955 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104955&action=review
> 
> >>>>>>> Source/WebCore/editing/VisibleSelection.cpp:300
> >>>>>>> +            if (isEndOfDocument(originalEnd) || (isEndOfLine(originalEnd) && !isStartOfLine(originalEnd) && !isEndOfParagraph(originalEnd)) || (isEndOfWord(originalEnd) && start != originalEnd && m_extent != originalEnd))
> >>>>>> 
> >>>>>> I'd split it into two lines since this is really long line.  ap & mitz should probably look at this change.
> >>>>> 
> >>>>> No, if c is at the end of a word, endOfWord(c, RightWordIfOnBoundary) returns the end of the following word.
> >>>> 
> >>>> 
> >>> 
> >>> Given that I'm checking isEndOfWord(originalEnd) && start != originalEnd, without doing the additional m_extent != originalEnd check, word-granularity mouse selections don't work correctly. What happens is given:
> >>> 
> >>> foo bar baz
> >>> 
> >>> If you double click in 'bar' and drag the mouse to the right to the position after the r in 'bar, it should grow the selection to include the space after 'bar' (in general the next word); it doesn't grow the selection if I omit the check in question.
> >>> 
> >>> I do admit it's not very clear what this check is doing; I'll try to find a more clear way to do this check. I initially saw that the first check (if originalEnd is at the end of a word) would fix the problem and added the other two checks to keep other behavior the same.
> >> 
> >> Shouldn't we be comparing it with m_end then?
> > 
> > originalEnd is declared:
> > 
> > VisiblePosition originalEnd(m_end, m_affinity); and is not modified until my check, so there shouldn't be a difference.
> 
> But you see
>     if (m_baseIsFirst) {
>         m_start = m_base;
>         m_end = m_extent;
>     } else {
>         m_start = m_extent;
>         m_end = m_base;
>     }
> above, right?

Wouldn't we need to depend on whether or not the base is first? Because the correct behavior from what I understand is that when dragging by word-granularity to the end of a word, the next word gets selected; on the other hand, when dragging by word-granularity to the start of a word, the previous word doesn't get selected. Let me know if I misunderstood your point.

But I don't see a difference between comparing m_extent to m_end and comparing m_extent to originalEnd in this context.

[Ryosuke Niwa]

Comment on attachment 104955 [details]
Proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=104955&action=review

>>>>>>>>> Source/WebCore/editing/VisibleSelection.cpp:300
>>>>>>>>> +            if (isEndOfDocument(originalEnd) || (isEndOfLine(originalEnd) && !isStartOfLine(originalEnd) && !isEndOfParagraph(originalEnd)) || (isEndOfWord(originalEnd) && start != originalEnd && m_extent != originalEnd))
>>>>>>>> 
>>>>>>>> I'd split it into two lines since this is really long line.  ap & mitz should probably look at this change.
>>>>>>> 
>>>>>>> No, if c is at the end of a word, endOfWord(c, RightWordIfOnBoundary) returns the end of the following word.
>>>>>> 
>>>>>> 
>>>>> 
>>>>> Given that I'm checking isEndOfWord(originalEnd) && start != originalEnd, without doing the additional m_extent != originalEnd check, word-granularity mouse selections don't work correctly. What happens is given:
>>>>> 
>>>>> foo bar baz
>>>>> 
>>>>> If you double click in 'bar' and drag the mouse to the right to the position after the r in 'bar, it should grow the selection to include the space after 'bar' (in general the next word); it doesn't grow the selection if I omit the check in question.
>>>>> 
>>>>> I do admit it's not very clear what this check is doing; I'll try to find a more clear way to do this check. I initially saw that the first check (if originalEnd is at the end of a word) would fix the problem and added the other two checks to keep other behavior the same.
>>>> 
>>>> Shouldn't we be comparing it with m_end then?
>>> 
>>> originalEnd is declared:
>>> 
>>> VisiblePosition originalEnd(m_end, m_affinity); and is not modified until my check, so there shouldn't be a difference.
>> 
>> But you see
>>     if (m_baseIsFirst) {
>>         m_start = m_base;
>>         m_end = m_extent;
>>     } else {
>>         m_start = m_extent;
>>         m_end = m_base;
>>     }
>> above, right?
> 
> Wouldn't we need to depend on whether or not the base is first? Because the correct behavior from what I understand is that when dragging by word-granularity to the end of a word, the next word gets selected; on the other hand, when dragging by word-granularity to the start of a word, the previous word doesn't get selected. Let me know if I misunderstood your point.
> 
> But I don't see a difference between comparing m_extent to m_end and comparing m_extent to originalEnd in this context.

It seems like your assumption breaks down on Mac since selection is directionless on Mac.

[Van Lam]

(In reply to comment #15)
> (From update of attachment 104955 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104955&action=review
> 
> >>>>>>>>> Source/WebCore/editing/VisibleSelection.cpp:300
> >>>>>>>>> +            if (isEndOfDocument(originalEnd) || (isEndOfLine(originalEnd) && !isStartOfLine(originalEnd) && !isEndOfParagraph(originalEnd)) || (isEndOfWord(originalEnd) && start != originalEnd && m_extent != originalEnd))
> >>>>>>>> 
> >>>>>>>> I'd split it into two lines since this is really long line.  ap & mitz should probably look at this change.
> >>>>>>> 
> >>>>>>> No, if c is at the end of a word, endOfWord(c, RightWordIfOnBoundary) returns the end of the following word.
> >>>>>> 
> >>>>>> 
> >>>>> 
> >>>>> Given that I'm checking isEndOfWord(originalEnd) && start != originalEnd, without doing the additional m_extent != originalEnd check, word-granularity mouse selections don't work correctly. What happens is given:
> >>>>> 
> >>>>> foo bar baz
> >>>>> 
> >>>>> If you double click in 'bar' and drag the mouse to the right to the position after the r in 'bar, it should grow the selection to include the space after 'bar' (in general the next word); it doesn't grow the selection if I omit the check in question.
> >>>>> 
> >>>>> I do admit it's not very clear what this check is doing; I'll try to find a more clear way to do this check. I initially saw that the first check (if originalEnd is at the end of a word) would fix the problem and added the other two checks to keep other behavior the same.
> >>>> 
> >>>> Shouldn't we be comparing it with m_end then?
> >>> 
> >>> originalEnd is declared:
> >>> 
> >>> VisiblePosition originalEnd(m_end, m_affinity); and is not modified until my check, so there shouldn't be a difference.
> >> 
> >> But you see
> >>     if (m_baseIsFirst) {
> >>         m_start = m_base;
> >>         m_end = m_extent;
> >>     } else {
> >>         m_start = m_extent;
> >>         m_end = m_base;
> >>     }
> >> above, right?
> > 
> > Wouldn't we need to depend on whether or not the base is first? Because the correct behavior from what I understand is that when dragging by word-granularity to the end of a word, the next word gets selected; on the other hand, when dragging by word-granularity to the start of a word, the previous word doesn't get selected. Let me know if I misunderstood your point.
> > 
> > But I don't see a difference between comparing m_extent to m_end and comparing m_extent to originalEnd in this context.
> 
> It seems like your assumption breaks down on Mac since selection is directionless on Mac.

I'm not sure how that breaks down my assumption (what is my assumption exactly?). Mac selections while directionless are still implemented using base and extent such that the greater of the two is used to determine the end of the selection.

But understanding that this proposed fix produces the correct behavior while keeping other behavior the same (no test failures in editing/selection/), do you have any suggestions on how it can be further improved?

[Ryosuke Niwa]

(In reply to comment #16)
> I'm not sure how that breaks down my assumption (what is my assumption exactly?). Mac selections while directionless are still implemented using base and extent such that the greater of the two is used to determine the end of the selection.
> 
> But understanding that this proposed fix produces the correct behavior while keeping other behavior the same (no test failures in editing/selection/), do you have any suggestions on how it can be further improved?

This area is very under-tested so the fact your patch doesn't break any tests isn't a good indication that your change is correct. In fact, we should never assume the correctness of code purely based on tests pass.

Having said that, m_extent can be either at the beginning or at the end of selection depending on the value isBaseFirst and from what you're saying, it seems like you need to compare originalEnd with whichever selection endpoint that appears later in the document order.

[Van Lam]

(In reply to comment #17)
> (In reply to comment #16)
> > I'm not sure how that breaks down my assumption (what is my assumption exactly?). Mac selections while directionless are still implemented using base and extent such that the greater of the two is used to determine the end of the selection.
> > 
> > But understanding that this proposed fix produces the correct behavior while keeping other behavior the same (no test failures in editing/selection/), do you have any suggestions on how it can be further improved?
> 
> This area is very under-tested so the fact your patch doesn't break any tests isn't a good indication that your change is correct. In fact, we should never assume the correctness of code purely based on tests pass.
> 
> Having said that, m_extent can be either at the beginning or at the end of selection depending on the value isBaseFirst and from what you're saying, it seems like you need to compare originalEnd with whichever selection endpoint that appears later in the document order.

The check in question (m_extent != originalEnd) is introduced only to handle the case when the user double clicks on a word and drags (with the cursor position corresponding to the extent of the selection) forward towards the end of the word; the context is determining whether the later(/greater offset) endpoint of the selection should be increased so that the selection includes the next word. This is why I compared m_extent to originalEnd.

[Ryosuke Niwa]

Comment on attachment 104955 [details]
Proposed fix

I don't think we want to land this patch as is.