Summary: | Caret's screen position does not update during an overflow scroll | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||
Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||
Status: | RESOLVED WORKSFORME | ||||||||||
Severity: | Normal | CC: | commit-queue, dino, jonlee, mark.lam, simon.fraser, thorton, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 133326 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Myles C. Maxfield
2014-05-23 22:13:54 PDT
Created attachment 232017 [details]
Patch
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? setCaretRectNeedsUpdate just sets a boolean to true. Recalculating the caret's rect is guarded by this boolean. (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 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 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. (In reply to comment #5) > invalidateCaretRect *also* sets that boolean. Yep. New patch incoming. Created attachment 232144 [details]
Patch
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 Re-opened since this is blocked by bug 133326 (In reply to comment #12) > Re-opened since this is blocked by bug 133326 Rolled out in r169402: <http://trac.webkit.org/changeset/169402>. Created attachment 232229 [details]
Patch
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 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.
I can't reproduce this any more (testing in this very textarea). |