Bug 5354 - Corner case where you can select outside the bounds of an editable block.
Summary: Corner case where you can select outside the bounds of an editable block.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Justin Garcia
URL:
Keywords:
Depends on: 4375
Blocks:
  Show dependency treegraph
 
Reported: 2005-10-12 14:46 PDT by Dan Wood
Modified: 2005-12-02 16:35 PST (History)
2 users (show)

See Also:


Attachments
Reduced & unreduced test cases showing selection problem (27.20 KB, application/octet-stream)
2005-10-12 14:48 PDT, Dan Wood
no flags Details
first cut (845 bytes, patch)
2005-11-16 20:51 PST, Justin Garcia
no flags Details | Formatted Diff | Diff
patch (6.26 KB, patch)
2005-11-17 19:27 PST, Justin Garcia
no flags Details | Formatted Diff | Diff
new patch (6.14 KB, patch)
2005-11-18 13:41 PST, Justin Garcia
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Wood 2005-10-12 14:46:46 PDT
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.
Comment 1 Dan Wood 2005-10-12 14:48:03 PDT
Created attachment 4329 [details]
Reduced & unreduced test cases showing selection problem
Comment 2 Dan Wood 2005-10-12 14:49:54 PDT
FYI It was justin garcia who fixed 4375 just a few days ago....
Comment 3 Justin Garcia 2005-11-16 20:51:48 PST
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.
Comment 4 Justin Garcia 2005-11-17 19:27:25 PST
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.
Comment 5 Justin Garcia 2005-11-18 13:41:08 PST
Created attachment 4727 [details]
new patch

Made a few small tweaks
Comment 6 Darin Adler 2005-11-18 16:04:45 PST
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.
Comment 7 Dan Wood 2005-11-19 20:56:25 PST
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.
Comment 8 Justin Garcia 2005-11-28 15:39:50 PST
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.
Comment 9 Justin Garcia 2005-11-28 15:40:12 PST
The new bug is 5856.
Comment 10 Justin Garcia 2005-11-28 15:45:47 PST
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.
Comment 11 Justin Garcia 2005-11-28 17:35:51 PST
Landed the patch.