Bug 36256 - shift+click to extend a wordgranularity selection backwards selects the space after
Summary: shift+click to extend a wordgranularity selection backwards selects the space...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-17 18:00 PDT by Ojan Vafai
Modified: 2017-07-18 08:27 PDT (History)
7 users (show)

See Also:


Attachments
Proposed fix (6.38 KB, patch)
2011-08-23 18:21 PDT, Van Lam
rniwa: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2010-03-17 18:00:58 PDT
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.
Comment 1 Van Lam 2011-08-23 18:21:12 PDT
Created attachment 104955 [details]
Proposed fix
Comment 2 Ryosuke Niwa 2011-08-23 23:19:03 PDT
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 ?
Comment 3 Alexey Proskuryakov 2011-08-23 23:27:02 PDT
Duplicate of bug 65999?
Comment 4 Van Lam 2011-08-24 10:01:24 PDT
(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.
Comment 5 Van Lam 2011-08-24 10:14:15 PDT
(In reply to comment #3)
> Duplicate of bug 65999?

After taking a look at your patch, yes these are duplicate bugs.
Comment 6 Alexey Proskuryakov 2011-08-24 10:53:55 PDT
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.
Comment 7 Van Lam 2011-08-24 11:31:02 PDT
(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?
Comment 8 Ryosuke Niwa 2011-09-14 20:19:03 PDT
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?
Comment 9 Van Lam 2011-09-15 11:07:16 PDT
(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.
Comment 10 Ryosuke Niwa 2011-09-15 11:12:01 PDT
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?
Comment 11 Van Lam 2011-09-15 11:19:31 PDT
(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.
Comment 12 Ryosuke Niwa 2011-09-15 11:29:51 PDT
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?
Comment 13 Ryosuke Niwa 2011-09-15 11:30:53 PDT
Depending on isBaseFirst() strikes me as a bad condition.
Comment 14 Van Lam 2011-09-15 12:03:06 PDT
(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.
Comment 15 Ryosuke Niwa 2011-09-15 12:06:08 PDT
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.
Comment 16 Van Lam 2011-09-15 14:48:00 PDT
(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?
Comment 17 Ryosuke Niwa 2011-09-15 16:20:13 PDT
(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.
Comment 18 Van Lam 2011-09-15 16:53:10 PDT
(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.
Comment 19 Ryosuke Niwa 2012-04-21 18:58:05 PDT
Comment on attachment 104955 [details]
Proposed fix

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