Bug 5354 – Corner case where you can select outside the bounds of an editable block.

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Dan Wood]

To reproduce:

View attached page "testEditableNotReduced.html" with Safari linking to TOT (as of 2005-Oct-12).
Drag from the editable block of text downward, observe behavior.
Click on the edtable block, choose 'Select All' from menu.

Repeat with 'testEditableReduced.html' for reduced case

Expected Behavior: After the fix of bug #4375, you should not be able to drag or select-all outside of 
an editable block.

Actual behavior: More than just the editable block is selected.  On the non-reduced case, it selects 
outside in multiple directions.

Notes: The reduced case is the minimum contents I needed to reproduce.  There has to be a div below 
the editable block, with overflow:hidden; style.

[Dan Wood]

Created attachment 4329 [details]
Reduced & unreduced test cases showing selection problem

[Dan Wood]

FYI It was justin garcia who fixed 4375 just a few days ago....

[Justin Garcia]

Created attachment 4707 [details]
first cut

We added SelectionController::adjustExtentForEditableContent for 4375.	In it,
if the base is inside an editable element and the extent is not, and if the
extent comes after the base, we use the last position in the editable element
for the extent.  Here's a snippit:

// base is in an editable region, but extent is not.
if (baseRoot) {
    Position first(Position(baseRoot, 0));
    Position last(Position(baseRoot, baseRoot->maxDeepOffset()));
	
    m_extent = baseIsStart ? last : first;
...

Later, in SelectionController::validate(), VisiblePositions are created out of
base and extent.  The problem is that a VisiblePosition created with the last
position in an block doesn't always stay inside that block.  This is why the
selection "spills" out of the editable div in the test case.  

Attached is a first cut at fixing VisiblePosition::initDownstream so that it
only uses the nextVisiblePosition if that position is inside the enclosing
block of the position passed to VisiblePosition's constructor.	It changes a
few layout tests in acceptible ways except one in editing/style, which I'm
looking into.

[Justin Garcia]

Created attachment 4719 [details]
patch

We sometimes want to leave the block containing the input position.  For
example, if a script tries to set a caret selection in an empty div with
adjacent pieces of visible text, we should leave the empty div to find a valid
visible position.  We should only leave the original containing block if there
aren't any valid visible positions inside of it.  

The ChangeLog entry explains the rest of the patch.

[Justin Garcia]

Created attachment 4727 [details]
new patch

Made a few small tweaks

[Darin Adler]

Comment on attachment 4727 [details]
new patch

Looks like a really great change to me. Given our conversation about how this
affected layout tests (gave better results on 4), r=me.

[Dan Wood]

I found a problem with the existing solution!

In my reduced test case, make sure no text is selected, then double-click somewhere in the blank area to 
the right of the word "responsible."  Notice that the selection goes outside the editable box. If you type a 
letter with that selected, you've just joined with the text below.

so we're getting close, but it's still possible to do damage to your DOM by allowing the selection to go 
past the last character in the editable div.

[Justin Garcia]

The bug that Dan points out comes from the fact that SelectionController::validate does changes to the 
selection that aren't validated (changes when the granularity passed to validate does not equal 
CHARACTER).  It doesn't seem right that such changes are part of "validation" at all.

This problem is unrelated to the patch above, so I'm filing a new bug for it and landing the patch.

[Justin Garcia]

The new bug is 5856.

[Justin Garcia]

I said above that the problem was unrelated to the one addressed by the reviewed patch, but that wasn't 
quite right.  I meant to say that the cause of the problem is unrelated to the changes to visible positions 
made in the patch.  The two problems are actually closely related (a selection that is based in an editable 
div is allowed to extend into a non-editable region).  I'm pushing the new bug into a separate report to 
make it easier to track the changes necessary to fix it.

[Justin Garcia]

Landed the patch.