Bug 128806 - computeSelectionStart and computeSelectionEnd shouldn't trigger synchronous layout
Summary: computeSelectionStart and computeSelectionEnd shouldn't trigger synchronous l...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 127832
  Show dependency treegraph
 
Reported: 2014-02-14 02:28 PST by Ryosuke Niwa
Modified: 2014-02-15 15:19 PST (History)
6 users (show)

See Also:


Attachments
Fixes the bug (6.11 KB, patch)
2014-02-15 03:31 PST, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2014-02-14 02:28:53 PST
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.
Comment 1 Ryosuke Niwa 2014-02-15 03:31:04 PST
Created attachment 224295 [details]
Fixes the bug
Comment 2 Darin Adler 2014-02-15 09:37:50 PST
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.
Comment 3 Ryosuke Niwa 2014-02-15 11:56:28 PST
(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.
Comment 4 Ryosuke Niwa 2014-02-15 15:19:57 PST
Committed r164180: <http://trac.webkit.org/changeset/164180>