Bug 25757 – VisibleSelection cannot represent all selection types

• Component: HTML Editing
• Status: NEW

[Will Hirsch]

1. Go to the example URL.
2. Click somewhere at the far right of the middle wrapped line to place the caret after the space at the end of the middle wrapped line.
3. Optionally, move down one line with the down key to move the caret to the end of the third wrapped line.
4. Hold down shift, and press and hold/repeatedly press the up key.

You'll notice that you can't highlight all the way to the top. I think it's related to the algorithm which remembers your horizontal caret position when you move to a line which is so short the caret has to move to the left.

[Will Hirsch]

Still reproduces in Safari 5.0.2 (7533.18.5) and Chrome 8.0.552.11 dev

[Ryosuke Niwa]

Created attachment 72146 [details]
demo

Reproduction steps:
1. Open the attached file
2. Shift+Up Twice

Expected result:
The last two lines are selected.

Actual result:
Only the last line is selected.

[Yair Yogev]

i can reproduce the issue with this test case under Windows only if i expand the width of the text area so that there are only 3 lines

the one at Bug 41986 (which i am guessing is the same bug) reproduces the issue reliably though

[Ryosuke Niwa]

*** Bug 41986 has been marked as a duplicate of this bug. ***

[Fabrizio]

Feel free to assign to me as I've been looking into this.

[Benjamin (Ben) Kalman]

@Fabrizio you're looking into it?  Oops, I didn't notice, I've been looking at it today too.  Well it's all yours :-)

But for the record, I think the problem is with the UPSTREAM/DOWNSTREAM affinity and how it's used in VisiblePosition::getInlineBoxAndOffset.

Imagine that the text is "foo baaaaaaaa" and gets wrapped as

foo
baaaaaaaa

Placing the cursor at the end of "baaaaaaaa" and pressing shift+up moves the selection from

foo
baaaaaaaa|

... to

foo{
baaaaaaaa}

... but internally (within VisibleSelection) it's just seen as "foo {baaaaaaaa}" *, which is actually

foo
{baaaaaaaa}

... because the start of the selection ends up with DOWNSTREAM affinity**.

The exact same thing happens next time that shift+up is pressed because:
 - the same line is used (VisiblePosition::getInlineBoxAndOffset).
 - the same x offset is used due to the cached m_xPosForVerticalArrowNavigation being used.

I've verified that this is indeed the cause of the problem, by (in SelectionController::modifyExtendingBackward) changing
  VisiblePosition pos(m_selection.extent(), m_selection.affinity())
to
  VisiblePosition pos(m_selection.extent(), UPSTREAM)
but I don't think that that is necessarily the most "correct" solution.  What would be "correct" is that m_selection.affinity() somehow knows that last operation was to extend the selection backwards, and that the affinity should be UPSTREAM in this case... but that doesn't really make sense, since what does the affinity of a whole selection really mean?

So perhaps the shortest path to a fix would be to change that to UPSTREAM.  And for the record, the change doesn't cause any layout tests to fail.


* rather than "foo{ baaaaaaaa}" due to visible equivalence, I believe.
** or at least, it does when then re-queried; it seems that the start of a selection always has DOWNSTREAM, and the end always has UPSTREAM.

[Ryosuke Niwa]

(In reply to comment #6)
> ... because the start of the selection ends up with DOWNSTREAM affinity**.

If this is really the case, then this is the bug and needs to be fixed.

> I've verified that this is indeed the cause of the problem, by (in SelectionController::modifyExtendingBackward) changing
>   VisiblePosition pos(m_selection.extent(), m_selection.affinity())
> to
>   VisiblePosition pos(m_selection.extent(), UPSTREAM)
> but I don't think that that is necessarily the most "correct" solution.  What would be "correct" is that m_selection.affinity() somehow knows that last operation was to extend the selection backwards, and that the affinity should be UPSTREAM in this case... but that doesn't really make sense, since what does the affinity of a whole selection really mean?

The correct fix is to make m_selection.affinity() return UPSTREAM there.

> So perhaps the shortest path to a fix would be to change that to UPSTREAM.  And for the record, the change doesn't cause any layout tests to fail.

No. That may make sense for this specific case but it won't make sense for other cases.  I bet this is an issue with the trial SelectionController in SelectionController::modify.  http://trac.webkit.org/browser/trunk/Source/WebCore/editing/SelectionController.cpp#L699.  Levi will probably know how to fix this.

[Fabrizio]

@Benjamin no worries!  I've been stepping through the SelectionController code a few days and changing some code around, but wasn't really getting at the root of the problem.  

I'll step through it again and see if I understand what you and Ryosuke have described but, by all means, post your patch(es) if you already have the fix.   I'm interested in seeing how this is supposed to work.  I can work on an easier bug in the meantime.

[Levi Weintraub]

> > I've verified that this is indeed the cause of the problem, by (in SelectionController::modifyExtendingBackward) changing
> >   VisiblePosition pos(m_selection.extent(), m_selection.affinity())
> > to
> >   VisiblePosition pos(m_selection.extent(), UPSTREAM)
> > but I don't think that that is necessarily the most "correct" solution.  What would be "correct" is that m_selection.affinity() somehow knows that last operation was to extend the selection backwards, and that the affinity should be UPSTREAM in this case... but that doesn't really make sense, since what does the affinity of a whole selection really mean?
> 
> The correct fix is to make m_selection.affinity() return UPSTREAM there.

That is correct.

> > So perhaps the shortest path to a fix would be to change that to UPSTREAM.  And for the record, the change doesn't cause any layout tests to fail.
> 
> No. That may make sense for this specific case but it won't make sense for other cases.  I bet this is an issue with the trial SelectionController in SelectionController::modify.  http://trac.webkit.org/browser/trunk/Source/WebCore/editing/SelectionController.cpp#L699.  Levi will probably know how to fix this.

Unfortunately this isn't a trial selection controller problem, but your statement that no layout tests fail by simply changing this to UPSTREAM worries me, as this would likely cause a number of unintended consequences! 

I'll take a look at this, and hope to check in a test that will, indeed, cause such a change to fail so we don't end up making a mistake like this in the future :)

[Levi Weintraub]

Ahh, more bugs from my early days. Apologies...

The source of this bug is VisibleSelection. VisibleSelection, in an effort to avoid the overhead and relayout issues of VisiblePositions, uses a set of Positions and one affinity. The single affinity is only used for Caret selections, which does not cover all cases, this one included.

VisibleSelection needs *at least* two affinities, one each for start and end, so it can keep track of a selection that covers both a downstream and upstream Position like the one we're trying to make here.

[Benjamin (Ben) Kalman]

(In reply to comment #10)
> Ahh, more bugs from my early days. Apologies...
> 
> The source of this bug is VisibleSelection. VisibleSelection, in an effort to avoid the overhead and relayout issues of VisiblePositions, uses a set of Positions and one affinity. The single affinity is only used for Caret selections, which does not cover all cases, this one included.

This sort of thing is what I meant by "what does the affinity of a whole selection really mean".

> 
> VisibleSelection needs *at least* two affinities, one each for start and end, so it can keep track of a selection that covers both a downstream and upstream Position like the one we're trying to make here.

Yep.  When I said that the start of a selection always has DOWNSTREAM and the start UPSTREAM I was referring to the visibleStart and visibleEnd methods which return DOWNSTREAM vs UPSTREAM respectively.  Perhaps this is what should be changed.

[Levi Weintraub]

> Yep.  When I said that the start of a selection always has DOWNSTREAM and the start UPSTREAM I was referring to the visibleStart and visibleEnd methods which return DOWNSTREAM vs UPSTREAM respectively.  Perhaps this is what should be changed.

This is an attempt to ensure we use the "smallest" possible selection that represents the underlying positions for editing. If we change visibleStart to always return the upstream VisiblePosition, we'll be switching this to being greedy, which will cause unintended consequences in other editing code.

I believe VisibleSelection needs to track multiple affinities that it derives based on the VisiblePositions passed in, but I'm not sure without looking more if we need 2 or 4 to cover all cases on all platforms.

[Benjamin (Ben) Kalman]

(In reply to comment #12)
> > Yep.  When I said that the start of a selection always has DOWNSTREAM and the start UPSTREAM I was referring to the visibleStart and visibleEnd methods which return DOWNSTREAM vs UPSTREAM respectively.  Perhaps this is what should be changed.
> 
> This is an attempt to ensure we use the "smallest" possible selection that represents the underlying positions for editing. If we change visibleStart to always return the upstream VisiblePosition, we'll be switching this to being greedy, which will cause unintended consequences in other editing code.

Oh, I wasn't suggesting this.  More like what you said below, or replacing setAffinity with setStartAffinity and getEndAffinity.  From what I (eclipse) can tell, the only place it's called is SelectionController:modify anyway.

> 
> I believe VisibleSelection needs to track multiple affinities that it derives based on the VisiblePositions passed in, but I'm not sure without looking more if we need 2 or 4 to cover all cases on all platforms.

[Yair Yogev]

Created attachment 82653 [details]
RTL version of the demo

Here is an RTL demo, (hopefully) showing the same bug.

[Yair Yogev]

Created attachment 82656 [details]
RTL version of the demo

same file, without Bugzilla destroying the encoding...

[Ahmad Saleem]

I am not able to reproduce this bug in Safari Technology Preview 158 and in both attached test case, doing Shift+Arrow Up will select both lines rather than just one line.

Appreciate if someone can reconfirm my testing results, so we can mark this as "RESOLVED CONFIGURATION CHANGED". Thanks!