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.
<rdar://problem/30822494>
+ repaint should correctly repaint the dots
Created attachment 352854 [details] Needs tests
To do: 1) Make sure the repaint rects are correct 2) Move the underline down if there is underlines 3) Add tests
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.
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
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
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
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
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
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
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
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
DocumentMarkerController has a bunch of invalidation logic, we probably need to update that.
.... except it looks like DocumentMarkerController::renderedRectsForMarkers() is never called??? :(
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()
Created attachment 353758 [details] WIP
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.
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
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.
Created attachment 353766 [details] Needs tests
Created attachment 353795 [details] Patch
Created attachment 353796 [details] Patch
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
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
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
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
Created attachment 353807 [details] Patch
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?
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.
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
Committed r237834: <https://trac.webkit.org/changeset/237834>
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
Reverted r237834 for reason: Tests for this change crash on iOS Simulator Committed r237870: <https://trac.webkit.org/changeset/237870>
Committed r237893: <https://trac.webkit.org/changeset/237893>