Caret's screen position does not update during an overflow scroll

[Myles C. Maxfield]

Caret's screen position does not update during a scroll

[Myles C. Maxfield]

Created attachment 232017 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/16293602>

[Simon Fraser (smfr)]

Comment on attachment 232017 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Caret's screen position does not update during a scroll

This should say "overflow scroll" since this doesn't apply to document scrolling.

> Source/WebCore/rendering/RenderLayer.cpp:2305
>      frame.selection().setCaretRectNeedsUpdate();
> +    frame.selection().invalidateCaretRect();

Is setCaretRectNeedsUpdate() a no-op then? Why isn't calling it enough to get the caret rect to be updated?

[Myles C. Maxfield]

setCaretRectNeedsUpdate just sets a boolean to true. Recalculating the caret's rect is guarded by this boolean.

[Tim Horton]

(In reply to comment #4)
> setCaretRectNeedsUpdate just sets a boolean to true. Recalculating the caret's rect is guarded by this boolean.

invalidateCaretRect *also* sets that boolean.

Really I'd expect the names "invalidateX" and "setXNeedsUpdate" to be interchangeable, but I realize that's not your fault.

[Darin Adler]

Comment on attachment 232017 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayer.cpp:2305
>> +    frame.selection().invalidateCaretRect();
> 
> Is setCaretRectNeedsUpdate() a no-op then? Why isn't calling it enough to get the caret rect to be updated?

These names are admittedly unclear and confusing, but:

- setCaretRectNeedsUpdate causes the caret rect’s coordinates to be recomputed
- invalidateCaretRect causes pixels where the caret is drawn to be marked dirty so they will be repainted

[Myles C. Maxfield]

Comment on attachment 232017 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Caret's screen position does not update during a scroll
> 
> This should say "overflow scroll" since this doesn't apply to document scrolling.

Done.

[Myles C. Maxfield]

(In reply to comment #5)
> invalidateCaretRect *also* sets that boolean.
Yep. New patch incoming.

[Myles C. Maxfield]

Created attachment 232144 [details]
Patch

[Myles C. Maxfield]

http://trac.webkit.org/changeset/169399

[Alexey Proskuryakov]

This caused assertion failures on tests:

http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r169399%20(5622)/accessibility/nested-layout-crash-crash-log.txt

http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r169399%20(5622)/editing/spelling/grammar-paste-crash-log.txt

[WebKit Commit Bot]

Re-opened since this is blocked by bug 133326

[Mark Lam]

(In reply to comment #12)
> Re-opened since this is blocked by bug 133326

Rolled out in r169402: <http://trac.webkit.org/changeset/169402>.

[Myles C. Maxfield]

Created attachment 232229 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 232229 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2605
> +            m_frame.selection().invalidateCaretRect();

Hrm not sure about this. What about other kinds of scrolling (scrollbar, programmatic)? What ensures that layout is up-to-date when calling invalidateCaretRect()?

[Michael Catanzaro]

Comment on attachment 232229 [details]
Patch

Hi,

Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting.

To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.

[Simon Fraser (smfr)]

I can't reproduce this any more (testing in this very textarea).