<rdar://problem/8546758>
Created attachment 71088 [details] Proposed patch (v1)
Comment on attachment 71088 [details] Proposed patch (v1) View in context: https://bugs.webkit.org/attachment.cgi?id=71088&action=review > WebCore/editing/Editor.cpp:-3616 > - m_frame->document()->markers()->removeMarkers(wordRange.get(), DocumentMarker::CorrectionIndicator); Why remove this line?
(In reply to comment #2) > (From update of attachment 71088 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71088&action=review > > > WebCore/editing/Editor.cpp:-3616 > > - m_frame->document()->markers()->removeMarkers(wordRange.get(), DocumentMarker::CorrectionIndicator); > > Why remove this line? This marker value doesn't get added on SnowLeopard or earlier. So, since now this couple lines of code is only for SL or earlier OS, we don't need to remove this marker here anymore.
Comment on attachment 71088 [details] Proposed patch (v1) View in context: https://bugs.webkit.org/attachment.cgi?id=71088&action=review >>> WebCore/editing/Editor.cpp:-3616 >>> - m_frame->document()->markers()->removeMarkers(wordRange.get(), DocumentMarker::CorrectionIndicator); >> >> Why remove this line? > > This marker value doesn't get added on SnowLeopard or earlier. So, since now this couple lines of code is only for SL or earlier OS, we don't need to remove this marker here anymore. Ah, ok. It looks like before recent changes, there was a call to removeMarkers for DocumentMarker::Replacement. Does that need to be re-added?
(In reply to comment #4) > (From update of attachment 71088 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71088&action=review > > >>> WebCore/editing/Editor.cpp:-3616 > >>> - m_frame->document()->markers()->removeMarkers(wordRange.get(), DocumentMarker::CorrectionIndicator); > >> > >> Why remove this line? > > > > This marker value doesn't get added on SnowLeopard or earlier. So, since now this couple lines of code is only for SL or earlier OS, we don't need to remove this marker here anymore. > > Ah, ok. It looks like before recent changes, there was a call to removeMarkers for DocumentMarker::Replacement. Does that need to be re-added? There's no need to add that. The statement for removing Replacement is also part of the patch for adding autocorrection support. But later on, I replaced "Replacement" with "CorrectionIndicator".
The commit-queue encountered the following flaky tests while processing attachment 71088 [details]: Please file bugs against the tests. The commit-queue is continuing to process your patch.
Comment on attachment 71088 [details] Proposed patch (v1) Clearing flags on attachment: 71088 Committed r70071: <http://trac.webkit.org/changeset/70071>
All reviewed patches have been landed. Closing bug.
This changed the rendering for a number of editing tests in chromium mac (on 10.5, not on 10.7 obviously): editing/deleting/delete-3800834-fix.html editing/deleting/merge-unrendered-space.html editing/deleting/pruning-after-merge-2.html editing/deleting/smart-delete-002.html editing/execCommand/4924441.html editing/execCommand/5080333-1.html editing/execCommand/5080333-2.html editing/execCommand/insert-list-and-stitch.html editing/execCommand/remove-list-item-1.html editing/inserting/insert-div-026.html editing/inserting/insert-paragraph-04.html editing/inserting/insert-text-with-newlines.html editing/inserting/return-key-with-selection-001.html editing/inserting/return-key-with-selection-002.html editing/pasteboard/5483567.html editing/pasteboard/paste-text-019.html editing/pasteboard/smart-paste-008.html editing/pasteboard/subframe-dragndrop-1.html editing/selection/4983858.html editing/selection/5057506-2.html editing/selection/move-by-character-001.html editing/style/5046875-1.html editing/style/create-block-for-style-001.html editing/style/create-block-for-style-003.html editing/style/create-block-for-style-004.html editing/style/create-block-for-style-007.html editing/style/create-block-for-style-009.html editing/style/create-block-for-style-010.html editing/style/create-block-for-style-013.html I haven't confirmed if the rendering results are different yet in safari but will check that next. The new rendering has red squigglies under many words that previously did not.
It appears this only affects chromium mac, not Safari or chromium on other platforms.
(In reply to comment #10) > It appears this only affects chromium mac, not Safari or chromium on other platforms. I think, instead of: #if PLATFORM(MAC) && (defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD) || defined(BUILDING_ON_SNOW_LEOPARD)) I should add: #if !PLATFORM(MAC) || (PLATFORM(MAC) && (defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD) || defined(BUILDING_ON_SNOW_LEOPARD))) This way, it only changes the behavior in Safari on 10.7. Shall I create a separate bug for this, or adding another path in this bug? James, what do I need to run tests for chromium mac?
I believe that would work. I'll try confirming locally.
(In reply to comment #12) > I believe that would work. I'll try confirming locally. Also, can we find out when were these tests created? <http://trac.webkit.org/changeset/66643> introduced a regression in the underline behavior, which has been corrected by this patch. So, if these tests were generated after <http://trac.webkit.org/changeset/66643>, we need to regenerate the screen shots after the fix.
(In reply to comment #11) > (In reply to comment #10) > > It appears this only affects chromium mac, not Safari or chromium on other platforms. > I think, instead of: > > #if PLATFORM(MAC) && (defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD) || defined(BUILDING_ON_SNOW_LEOPARD)) > > I should add: > > #if !PLATFORM(MAC) || (PLATFORM(MAC) && (defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD) || defined(BUILDING_ON_SNOW_LEOPARD))) > > This way, it only changes the behavior in Safari on 10.7. Shall I create a separate bug for this, or adding another path in this bug? SGTM. But did you get comment about there's a code that adds correction indicator that's outside of ifdef's? http://trac.webkit.org/browser/trunk/WebCore/editing/Editor.cpp?rev=70071#L2788 Could you verify that this is intended?
Also, I have to say that the new behavior seems better than the old behavior. In http://trac.webkit.org/browser/trunk/LayoutTests/editing/deleting/delete-3800834-fix.html for example, we have: <span id="test">Foo<BR><BLOCKQUOTE>Bar</BLOCKQUOTE></span> And we delete "\nBar" backwards. We have correction indicator (or whatever red dotted line is called) until we delete "Bar" but the indicator disappears when we delete "\n" between "Foo" and "Bar" (deleted by that moment). Is this intended?
(In reply to comment #14) > (In reply to comment #11) > > (In reply to comment #10) > > > It appears this only affects chromium mac, not Safari or chromium on other platforms. > > I think, instead of: > > > > #if PLATFORM(MAC) && (defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD) || defined(BUILDING_ON_SNOW_LEOPARD)) > > > > I should add: > > > > #if !PLATFORM(MAC) || (PLATFORM(MAC) && (defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD) || defined(BUILDING_ON_SNOW_LEOPARD))) > > > > This way, it only changes the behavior in Safari on 10.7. Shall I create a separate bug for this, or adding another path in this bug? > > SGTM. > > But did you get comment about there's a code that adds correction indicator that's outside of ifdef's? > http://trac.webkit.org/browser/trunk/WebCore/editing/Editor.cpp?rev=70071#L2788 > > Could you verify that this is intended? The CorrectionIndicator marker isn't used except in Safari on 10.7. But, for the sake of clarity, I suppose I should put that statement inside "#if PLATFORM(MAC) && !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)"
(In reply to comment #11) > (In reply to comment #10) > > It appears this only affects chromium mac, not Safari or chromium on other platforms. > I think, instead of: > > #if PLATFORM(MAC) && (defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD) || defined(BUILDING_ON_SNOW_LEOPARD)) > > I should add: > > #if !PLATFORM(MAC) || (PLATFORM(MAC) && (defined(BUILDING_ON_TIGER) || defined(BUILDING_ON_LEOPARD) || defined(BUILDING_ON_SNOW_LEOPARD))) > > This way, it only changes the behavior in Safari on 10.7. Shall I create a separate bug for this, or adding another path in this bug? > > James, what do I need to run tests for chromium mac? Confirmed that this change restores the previous Chromium Mac behavior (which matches the current Safari Mac behavior).
(In reply to comment #17) > Confirmed that this change restores the previous Chromium Mac behavior (which matches the current Safari Mac behavior). Great! Jia, I think you can commit this change as build fix since it broke chromium test bots.
(In reply to comment #15) > Also, I have to say that the new behavior seems better than the old behavior. In http://trac.webkit.org/browser/trunk/LayoutTests/editing/deleting/delete-3800834-fix.html for example, we have: > > <span id="test">Foo<BR><BLOCKQUOTE>Bar</BLOCKQUOTE></span> > > And we delete "\nBar" backwards. We have correction indicator (or whatever red dotted line is called) until we delete "Bar" but the indicator disappears when we delete "\n" between "Foo" and "Bar" (deleted by that moment). Is this intended? I'm not quite sure about the rationale behind the old behavior. So for this patch, my intention is to only change introduce the new behavior into Safari on 10.7. If the new behavior is desirable across the board, I could certainly make this change to all platforms.
Reopen the bug. The first patch broke some Chromium Mac behavior.
Created attachment 71225 [details] Additional Patch (v1) New patch to make sure there's no change of behavior on non-Mac platforms.
Comment on attachment 71225 [details] Additional Patch (v1) R=me Thanks for following up on this!
Comment on attachment 71225 [details] Additional Patch (v1) Clearing flags on attachment: 71225 Committed r70113: <http://trac.webkit.org/changeset/70113>