Bug 128806 – computeSelectionStart and computeSelectionEnd shouldn't trigger synchronous layout

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Ryosuke Niwa]

This is similar to the bug 128478.

computeSelectionStart() and computeSelectionStart() in HTMLTextFormControlElement use indexForVisiblePosition
to compute start and end indexes but we shouldn't need to since we only have br elements and text nodes inside
the shadow DOM of inputs and text area elements.

We should be able to traverse through the shadow DOM with regular Position instead of VisiblePosition.

[Ryosuke Niwa]

Created attachment 224295 [details]
Fixes the bug

[Darin Adler]

Comment on attachment 224295 [details]
Fixes the bug

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

> Source/WebCore/html/HTMLTextFormControlElement.cpp:333
> +    int index = indexForPosition(position.deepEquivalent());

Why int rather than unsigned?

> Source/WebCore/html/HTMLTextFormControlElement.cpp:601
> +                index += std::min(length, static_cast<unsigned>(passedPosition.offsetInContainerNode()));

Instead of static_cast<unsigned>, I suggest writing std::min<unsigned>.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:616
> +#ifndef NDEBUG
> +    VisiblePosition visiblePosition = passedPosition;
> +    unsigned indexComputedByVisiblePosition = 0;
> +    if (visiblePosition.isNotNull())
> +        indexComputedByVisiblePosition = WebCore::indexForVisiblePosition(innerText, visiblePosition, false /* forSelectionPreservation */);
> +    ASSERT(index == indexComputedByVisiblePosition);
> +#endif

I think an assertion like this should be written with a helper function so it doesn’t need to be inside an #if. Also can consider ASSERT_DISABLED instead of NDEBUG.

[Ryosuke Niwa]

(In reply to comment #2)
> (From update of attachment 224295 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224295&action=review
> 
> > Source/WebCore/html/HTMLTextFormControlElement.cpp:333
> > +    int index = indexForPosition(position.deepEquivalent());
> 
> Why int rather than unsigned?

Will fix.

> > Source/WebCore/html/HTMLTextFormControlElement.cpp:601
> > +                index += std::min(length, static_cast<unsigned>(passedPosition.offsetInContainerNode()));
> 
> Instead of static_cast<unsigned>, I suggest writing std::min<unsigned>.

Will fix.

> > Source/WebCore/html/HTMLTextFormControlElement.cpp:616
> > +#ifndef NDEBUG
> > +    VisiblePosition visiblePosition = passedPosition;
> > +    unsigned indexComputedByVisiblePosition = 0;
> > +    if (visiblePosition.isNotNull())
> > +        indexComputedByVisiblePosition = WebCore::indexForVisiblePosition(innerText, visiblePosition, false /* forSelectionPreservation */);
> > +    ASSERT(index == indexComputedByVisiblePosition);
> > +#endif
> 
> I think an assertion like this should be written with a helper function so it doesn’t need to be inside an #if. Also can consider ASSERT_DISABLED instead of NDEBUG.

I'd like to be able to see indexComputedByVisiblePosition and visiblePosition for debugging purposes when the assertion is hit.  If I put the computation inside a helper function, it would be tricky to get those objects/values in the debugger.  But I'd use ASSERT_DISABLED.

[Ryosuke Niwa]

Committed r164180: <http://trac.webkit.org/changeset/164180>