Bug 218020

Summary: Stop gap patch fix for regression in r267329.
Product: WebKit Reporter: Megan Gardner <megan_gardner>
Component: New BugsAssignee: Megan Gardner <megan_gardner>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, ews-watchlist, mifenton, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=247239
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch none

Megan Gardner
Reported 2020-10-21 06:50:41 PDT
Stop gap patch fix for regression in r267329.
Attachments
Patch (7.64 KB, patch)
2020-10-21 06:51 PDT, Megan Gardner 		no flags
DetailsFormatted DiffDiff
Patch (7.88 KB, patch)
2020-10-21 11:29 PDT, Megan Gardner 		no flags
DetailsFormatted DiffDiff
Patch (8.35 KB, patch)
2020-10-21 12:45 PDT, Megan Gardner 		no flags
DetailsFormatted DiffDiff
Patch (8.38 KB, patch)
2020-10-21 12:53 PDT, Megan Gardner 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (8.39 KB, patch)
2020-10-21 14:56 PDT, Megan Gardner 		no flags
DetailsFormatted DiffDiff
Patch (8.52 KB, patch)
2020-10-21 15:54 PDT, Megan Gardner 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Megan Gardner
Comment 1 2020-10-21 06:51:14 PDT
Created attachment 411980 [details] Patch
Megan Gardner
Comment 2 2020-10-21 11:29:37 PDT
Created attachment 412012 [details] Patch
Sam Weinig
Comment 3 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?
Megan Gardner
Comment 4 2020-10-21 12:45:19 PDT
Created attachment 412017 [details] Patch
Megan Gardner
Comment 5 2020-10-21 12:53:30 PDT
Created attachment 412019 [details] Patch
Darin Adler
Comment 6 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.
Megan Gardner
Comment 7 2020-10-21 14:56:57 PDT
Created attachment 412033 [details] Patch
Megan Gardner
Comment 8 2020-10-21 15:54:26 PDT
Created attachment 412039 [details] Patch
Darin Adler
Comment 9 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.
EWS
Comment 10 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].
Radar WebKit Bug Importer
Comment 11 2020-10-21 17:41:18 PDT
<rdar://problem/70553518>
Darin Adler
Comment 12 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!
Darin Adler
Comment 13 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.
Note You need to log in before you can comment on or make changes to this bug.