Bug 190764

Summary: Spelling dots are drawn in the wrong place
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: TextAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dbates, dino, ews-watchlist, jonlee, mmaxfield, rniwa, ryanhaddad, simon.fraser, thorton, 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=190772
https://bugs.webkit.org/show_bug.cgi?id=196158
Bug Depends on:    
Bug Blocks: 190774    
Attachments:
Description Flags
Needs tests
none
Archive of layout-test-results from ews100 for mac-sierra
none
Archive of layout-test-results from ews112 for mac-sierra
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
WIP
none
Needs tests
none
Patch
none
Patch
none
Archive of layout-test-results from ews100 for mac-sierra
none
Archive of layout-test-results from ews117 for mac-sierra
none
Patch dino: review+, commit-queue: commit-queue-

Myles C. Maxfield
Reported 2018-10-19 15:15:46 PDT
Dots should not be clipped. Dots should be horizontally centered Dots should be drawn behind the text (is there standardization work here?) Distance from the baseline to the top of the dot should be 11.035% of font size Dot diameter should be 13.247% of the font size Distance between the dots (right side of the left dot to left side of the right dot) should be 9.457% of the font size The "font size" used in these calculations should be clamped so it's 10pt <= font size <= 40pt Red: #FF3B30 Orange: #FF9500 Yellow: #FFCC00 Green: #4CD964 Teal Blue: #5AC8FA Blue: #007AFF Purple: #5856D6 Pink: #FF2D55 Spelling dots should not overlap underlines, and if there would be a collision, should be moved down We'll likely need to update our repaint rects to allow for the new positioning.
Attachments
Needs tests (37.43 KB, patch)
2018-10-20 08:05 PDT, Myles C. Maxfield 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews100 for mac-sierra (2.34 MB, application/zip)
2018-10-20 09:13 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews112 for mac-sierra (3.03 MB, application/zip)
2018-10-20 10:07 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.52 MB, application/zip)
2018-10-20 10:10 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.45 MB, application/zip)
2018-10-20 12:13 PDT, EWS Watchlist 		no flags
Details
WIP (20.28 KB, patch)
2018-11-02 19:39 PDT, Myles C. Maxfield 		no flags
DetailsFormatted DiffDiff
Needs tests (19.75 KB, patch)
2018-11-02 22:39 PDT, Myles C. Maxfield 		no flags
DetailsFormatted DiffDiff
Patch (32.06 KB, patch)
2018-11-03 22:40 PDT, Myles C. Maxfield 		no flags
DetailsFormatted DiffDiff
Patch (37.76 KB, patch)
2018-11-03 22:51 PDT, Myles C. Maxfield 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews100 for mac-sierra (2.36 MB, application/zip)
2018-11-03 23:57 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews117 for mac-sierra (3.10 MB, application/zip)
2018-11-04 00:52 PDT, EWS Watchlist 		no flags
Details
Patch (49.83 KB, patch)
2018-11-04 12:22 PST, Myles C. Maxfield 		dino: review+
commit-queue: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.
Myles C. Maxfield
Comment 1 2018-10-19 15:15:54 PDT
<rdar://problem/30822494>
Simon Fraser (smfr)
Comment 2 2018-10-19 22:08:27 PDT
+ repaint should correctly repaint the dots
Myles C. Maxfield
Comment 3 2018-10-20 08:05:44 PDT
Created attachment 352854 [details] Needs tests
Myles C. Maxfield
Comment 4 2018-10-20 08:07:43 PDT
To do: 1) Make sure the repaint rects are correct 2) Move the underline down if there is underlines 3) Add tests
EWS Watchlist
Comment 5 2018-10-20 08:08:49 PDT
Attachment 352854 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
EWS Watchlist
Comment 6 2018-10-20 09:12:58 PDT
Comment on attachment 352854 [details] Needs tests Attachment 352854 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9678242 New failing tests: fast/writing-mode/english-bt-text-with-spelling-marker.html
EWS Watchlist
Comment 7 2018-10-20 09:13:00 PDT
Created attachment 352855 [details] Archive of layout-test-results from ews100 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-sierra Platform: Mac OS X 10.12.6
EWS Watchlist
Comment 8 2018-10-20 10:07:19 PDT
Comment on attachment 352854 [details] Needs tests Attachment 352854 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9678383 New failing tests: fast/writing-mode/english-bt-text-with-spelling-marker.html
EWS Watchlist
Comment 9 2018-10-20 10:07:21 PDT
Created attachment 352856 [details] Archive of layout-test-results from ews112 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
EWS Watchlist
Comment 10 2018-10-20 10:10:22 PDT
Comment on attachment 352854 [details] Needs tests Attachment 352854 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9678386 New failing tests: fast/writing-mode/english-bt-text-with-spelling-marker.html
EWS Watchlist
Comment 11 2018-10-20 10:10:24 PDT
Created attachment 352857 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
EWS Watchlist
Comment 12 2018-10-20 12:13:04 PDT
Comment on attachment 352854 [details] Needs tests Attachment 352854 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9679121 New failing tests: fast/writing-mode/english-bt-text-with-spelling-marker.html
EWS Watchlist
Comment 13 2018-10-20 12:13:05 PDT
Created attachment 352860 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Myles C. Maxfield
Comment 14 2018-11-02 19:03:58 PDT
DocumentMarkerController has a bunch of invalidation logic, we probably need to update that.
Myles C. Maxfield
Comment 15 2018-11-02 19:26:40 PDT
.... except it looks like DocumentMarkerController::renderedRectsForMarkers() is never called??? :(
Myles C. Maxfield
Comment 16 2018-11-02 19:32:29 PDT
It's not clear to me how we associate a change in the document markers with a rect that gets repainted. Perhaps the text element itself handles the invalidation? aka something like RenderStyle::changeAffectsVisualOverflow()
Myles C. Maxfield
Comment 17 2018-11-02 19:39:26 PDT
Created attachment 353758 [details] WIP
Myles C. Maxfield
Comment 18 2018-11-02 21:55:11 PDT
Comment on attachment 353758 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=353758&action=review > Source/WebCore/dom/DocumentMarkerController.cpp:168 > + // FIXME: This is incorrect. We need to get the visual overflow rect, not the selection rect. I misunderstood what this function is trying to do. Is should remove this comment.
Myles C. Maxfield
Comment 19 2018-11-02 22:23:36 PDT
Comment on attachment 353758 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=353758&action=review > Source/WebCore/rendering/InlineFlowBox.cpp:937 > + //FIXME: Add repaint rect information here. textBox.calculateUnionOfAllDocumentMarkerBounds() Not here; just above. topGlyphEdge & friends are outsets from the text box layout bounds, where positive means "outward." There is no coordinate system. Then, topGlyphOverflow & friends are the same idea, except the top and left values are flipped to represent deltas from the increasing-y-is-down coordinate system Then, logicalTopVisualOverflow & friends add the layout bounds of the text box to get points in the coordinate system that the text box is in
Myles C. Maxfield
Comment 20 2018-11-02 22:33:24 PDT
I'm going to have to move the dots down if there is underlines in a follow-up patch, because right now it's too hard to figure out where the underline position is.
Myles C. Maxfield
Comment 21 2018-11-02 22:39:54 PDT
Created attachment 353766 [details] Needs tests
Myles C. Maxfield
Comment 22 2018-11-03 22:40:02 PDT
Created attachment 353795 [details] Patch
Myles C. Maxfield
Comment 23 2018-11-03 22:51:40 PDT
Created attachment 353796 [details] Patch
EWS Watchlist
Comment 24 2018-11-03 23:57:25 PDT
Comment on attachment 353796 [details] Patch Attachment 353796 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9851633 New failing tests: fast/writing-mode/english-bt-text-with-spelling-marker.html
EWS Watchlist
Comment 25 2018-11-03 23:57:27 PDT
Created attachment 353798 [details] Archive of layout-test-results from ews100 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-sierra Platform: Mac OS X 10.12.6
EWS Watchlist
Comment 26 2018-11-04 00:52:36 PDT
Comment on attachment 353796 [details] Patch Attachment 353796 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9851710 New failing tests: fast/writing-mode/english-bt-text-with-spelling-marker.html
EWS Watchlist
Comment 27 2018-11-04 00:52:37 PDT
Created attachment 353800 [details] Archive of layout-test-results from ews117 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
Myles C. Maxfield
Comment 28 2018-11-04 12:22:02 PST
Created attachment 353807 [details] Patch
Dean Jackson
Comment 29 2018-11-05 13:25:31 PST
Comment on attachment 353807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353807&action=review > Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:224 > + > + // Toggle on before the test, and toggle off after the test. > + if (options.shouldShowSpellCheckingDots) > + [platformView toggleContinuousSpellChecking:nil]; Why not just add it to the list of incompatible options that require a new WebView to be created?
Myles C. Maxfield
Comment 30 2018-11-05 15:40:43 PST
Comment on attachment 353807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353807&action=review >> Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm:224 >> + [platformView toggleContinuousSpellChecking:nil]; > > Why not just add it to the list of incompatible options that require a new WebView to be created? It is in that list :) Once a new WebView is created, this call actually turns on the spellchecker.
WebKit Commit Bot
Comment 31 2018-11-05 15:52:27 PST
Comment on attachment 353807 [details] Patch Rejecting attachment 353807 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 353807, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Logging in as commit-queue@webkit.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=353807&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=190764&ctype=xml&excludefield=attachmentdata Processing 1 patch from 1 bug. Processing patch 353807 from bug 190764. Fetching: https://bugs.webkit.org/attachment.cgi?id=353807 Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Dean Jackson']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Parsed 29 diffs from patch file(s). patching file Source/WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebCore/platform/graphics/cocoa/GraphicsContextCocoa.mm patching file Source/WebCore/rendering/InlineFlowBox.cpp patching file Source/WebCore/rendering/InlineFlowBox.h patching file Source/WebCore/rendering/InlineTextBox.cpp patching file Source/WebCore/rendering/InlineTextBox.h patching file Source/WebCore/rendering/SimpleLineLayout.cpp patching file Source/WebCore/rendering/SimpleLineLayoutCoverage.cpp patching file Source/WebCore/rendering/SimpleLineLayoutCoverage.h patching file Tools/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Tools/WebKitTestRunner/TestController.cpp patching file Tools/WebKitTestRunner/TestController.h patching file Tools/WebKitTestRunner/TestOptions.h Hunk #2 FAILED at 103. 1 out of 2 hunks FAILED -- saving rejects to file Tools/WebKitTestRunner/TestOptions.h.rej patching file Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm patching file Tools/WebKitTestRunner/cocoa/TestRunnerWKWebView.h patching file Tools/WebKitTestRunner/ios/TestControllerIOS.mm patching file Tools/WebKitTestRunner/mac/TestControllerMac.mm patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/editing/spelling/spelling-dots-position-2-expected-mismatch.html patching file LayoutTests/editing/spelling/spelling-dots-position-2.html patching file LayoutTests/editing/spelling/spelling-dots-position-3-expected-mismatch.html patching file LayoutTests/editing/spelling/spelling-dots-position-3.html patching file LayoutTests/editing/spelling/spelling-dots-position-expected.html patching file LayoutTests/editing/spelling/spelling-dots-position.html patching file LayoutTests/editing/spelling/spelling-dots-repaint-expected.html patching file LayoutTests/editing/spelling/spelling-dots-repaint.html patching file LayoutTests/fast/writing-mode/english-bt-text-with-spelling-marker-expected.html patching file LayoutTests/fast/writing-mode/english-bt-text-with-spelling-marker.html Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Dean Jackson']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: https://webkit-queues.webkit.org/results/9871294
Myles C. Maxfield
Comment 32 2018-11-05 16:09:17 PST
Committed r237834: <https://trac.webkit.org/changeset/237834>
Ryan Haddad
Comment 33 2018-11-06 09:01:35 PST
I know that ios-sim-ews dropped the ball a bit here, but the tests added with this change are consistently crashing on the bots: Application Specific Information: CRASHING TEST: editing/spelling/spelling-dots-position-2.html *** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '-[TestRunnerWKWebView toggleContinuousSpellChecking:]: unrecognized selector sent to instance 0x7fe32c002e00' https://build.webkit.org/results/Apple%20iOS%2012%20Simulator%20Release%20WK2%20(Tests)/r237862%20(774)/results.html
Ryan Haddad
Comment 34 2018-11-06 10:23:45 PST
Reverted r237834 for reason: Tests for this change crash on iOS Simulator Committed r237870: <https://trac.webkit.org/changeset/237870>
Myles C. Maxfield
Comment 35 2018-11-06 15:06:46 PST
Committed r237893: <https://trac.webkit.org/changeset/237893>
Note You need to log in before you can comment on or make changes to this bug.