Bug 133256 - Caret's screen position does not update during an overflow scroll
Summary: Caret's screen position does not update during an overflow scroll
Status: RESOLVED WORKSFORME
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on: 133326
Blocks:
  Show dependency treegraph
 
Reported: 2014-05-23 22:13 PDT by Myles C. Maxfield
Modified: 2017-11-14 15:36 PST (History)
7 users (show)

See Also:


Attachments
Patch (6.12 KB, patch)
2014-05-23 22:15 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (6.19 KB, patch)
2014-05-27 12:30 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (6.03 KB, patch)
2014-05-28 20:16 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2014-05-23 22:13:54 PDT
Caret's screen position does not update during a scroll
Comment 1 Myles C. Maxfield 2014-05-23 22:15:39 PDT
Created attachment 232017 [details]
Patch
Comment 2 Myles C. Maxfield 2014-05-23 22:16:32 PDT
<rdar://problem/16293602>
Comment 3 Simon Fraser (smfr) 2014-05-23 22:59:05 PDT
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?
Comment 4 Myles C. Maxfield 2014-05-24 01:08:38 PDT
setCaretRectNeedsUpdate just sets a boolean to true. Recalculating the caret's rect is guarded by this boolean.
Comment 5 Tim Horton 2014-05-24 01:12:21 PDT
(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.
Comment 6 Darin Adler 2014-05-24 20:22:29 PDT
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
Comment 7 Myles C. Maxfield 2014-05-27 12:26:53 PDT
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.
Comment 8 Myles C. Maxfield 2014-05-27 12:28:23 PDT
(In reply to comment #5)
> invalidateCaretRect *also* sets that boolean.
Yep. New patch incoming.
Comment 9 Myles C. Maxfield 2014-05-27 12:30:38 PDT
Created attachment 232144 [details]
Patch
Comment 10 Myles C. Maxfield 2014-05-27 14:15:19 PDT
http://trac.webkit.org/changeset/169399
Comment 12 WebKit Commit Bot 2014-05-27 15:53:51 PDT
Re-opened since this is blocked by bug 133326
Comment 13 Mark Lam 2014-05-27 15:58:14 PDT
(In reply to comment #12)
> Re-opened since this is blocked by bug 133326

Rolled out in r169402: <http://trac.webkit.org/changeset/169402>.
Comment 14 Myles C. Maxfield 2014-05-28 20:16:25 PDT
Created attachment 232229 [details]
Patch
Comment 15 Simon Fraser (smfr) 2014-05-28 20:45:58 PDT
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()?
Comment 16 Michael Catanzaro 2016-09-17 07:14:52 PDT
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.
Comment 17 Simon Fraser (smfr) 2017-11-14 15:36:23 PST
I can't reproduce this any more (testing in this very textarea).