Bug 218020 - Stop gap patch fix for regression in r267329.
Summary: Stop gap patch fix for regression in r267329.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-21 06:50 PDT by Megan Gardner
Modified: 2020-12-18 12:41 PST (History)
5 users (show)

See Also:


Attachments
Patch (7.64 KB, patch)
2020-10-21 06:51 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (7.88 KB, patch)
2020-10-21 11:29 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (8.35 KB, patch)
2020-10-21 12:45 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (8.38 KB, patch)
2020-10-21 12:53 PDT, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (8.39 KB, patch)
2020-10-21 14:56 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (8.52 KB, patch)
2020-10-21 15:54 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2020-10-21 06:50:41 PDT
Stop gap patch fix for regression in r267329.
Comment 1 Megan Gardner 2020-10-21 06:51:14 PDT
Created attachment 411980 [details]
Patch
Comment 2 Megan Gardner 2020-10-21 11:29:37 PDT
Created attachment 412012 [details]
Patch
Comment 3 Sam Weinig 2020-10-21 11:42:05 PDT
Comment on attachment 412012 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

What was the regression? How does this fix it? Where is the long term solution being tracked?
Comment 4 Megan Gardner 2020-10-21 12:45:19 PDT
Created attachment 412017 [details]
Patch
Comment 5 Megan Gardner 2020-10-21 12:53:30 PDT
Created attachment 412019 [details]
Patch
Comment 6 Darin Adler 2020-10-21 12:56:54 PDT
Comment on attachment 412019 [details]
Patch

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

Looks good. I really appreciate having the test, which I will keep working and/or refine as I return to the Selection API work.

> Source/WebCore/editing/VisibleSelection.cpp:390
> -    bool shouldUpdateAnchor = m_start != startBeforeAdjustments;
> -    bool shouldUpdateFocus = m_end != endBeforeAdjustments;
> +    bool shouldUpdateAnchor = false;
> +    bool shouldUpdateFocus = false;

Might be good to leave a comment behind here. The fact that these are always false makes the code peculiar for now, but makes sense if you know we intend to change it later.
Comment 7 Megan Gardner 2020-10-21 14:56:57 PDT
Created attachment 412033 [details]
Patch
Comment 8 Megan Gardner 2020-10-21 15:54:26 PDT
Created attachment 412039 [details]
Patch
Comment 9 Darin Adler 2020-10-21 16:04:39 PDT
Comment on attachment 412039 [details]
Patch

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

> Source/WebCore/editing/VisibleSelection.cpp:389
> +    bool shouldUpdateAnchor = false; // Set to false because of <rdar://problem/69542459>. Can be returned to original logic when this problem is fully fixed.

Probably more kind to open source contributors to cite https://bugs.webkit.org/show_bug.cgi?id=218020 instead of rdar, but clear enough either way. I promise to remove this comment some day either way.
Comment 10 EWS 2020-10-21 17:41:01 PDT
Committed r268847: <https://trac.webkit.org/changeset/268847>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412039 [details].
Comment 11 Radar WebKit Bug Importer 2020-10-21 17:41:18 PDT
<rdar://problem/70553518>
Comment 12 Darin Adler 2020-12-18 12:38:46 PST
Seems like the test isn’t working well to detect the problem. I rolled the change back and the test still passes!
Comment 13 Darin Adler 2020-12-18 12:41:39 PST
No, I was running the test wrong. I was confused by the ChangeLog, which lists the wrong directory for the test.