Bug 41423

It seems CompositeEditCommand::moveParagraph() (called from DeleteSelectionCommand::mergeParagraphs()) does not call Document::copyMarkers() to copy the markers in the source range. (Unfortunately, I do not have any clear idea how we should fix it, though.) Regards,

Created attachment 61357 [details] a preliminary patch

Created attachment 61358 [details] another repro (with automation)

> a preliminary patch This is just a preliminary version and has no test.

Comment on attachment 61357 [details] a preliminary patch Code looks fine to me except for the comments below. Just needs a test. Not sure it's possible to test spelling markers in DRT. If not, please add a manual test. > +++ b/WebCore/editing/Editor.h > + // Invoked from CompositeEditCommand::moveParagraphs() This comment doesn't seem especially useful. > + void removeMarkersBeforeMovingParagrah(const VisibleSelection&); > + void markMisspellingsAfterMovingParagrah(const VisibleSelection&); I don't like that these names are tied to paragraph moving since the implementations themselves are generic. How about calling them clearMispellingsAndBadGrammer and markMispellingsAndBadGrammer?

Created attachment 61481 [details] updated. has no test yet.

Hi Ojan, thank you for reviewing! I updated the patch. > Code looks fine to me except for the comments below. Just needs a test. Not sure it's possible to test spelling markers in DRT. If not, please add a manual test. Though a pixel test is possible, I prefer inspecting marker from more robust way. I heard that Bono-san has code for testing marker (for Bug 40092). I'll ask him if it is available. > > + void removeMarkersBeforeMovingParagrah(const VisibleSelection&); > > + void markMisspellingsAfterMovingParagrah(const VisibleSelection&); > > I don't like that these names are tied to paragraph moving since the implementations themselves are generic. How about calling them clearMispellingsAndBadGrammer and markMispellingsAndBadGrammer? Makes sense. renamed.

Is it really necessary to remove the markers from the range that's about to be moved, they'll be removed when that range is deleted won't they?

Hi Justin, thank you for taking a look! (In reply to comment #8) > Is it really necessary to remove the markers from the range that's about to be moved, they'll be removed when that range is deleted won't they? It is a good question. Actually, no one except Editor removes the marker thus markers for removed nodes are left orphan. (It doesn't cause a memory leak because they are owned by the Document.) So not removing them likely has little harm. But I think it's more polite to remove it than left them orphan, especially inside the Editor.

Created attachment 61765 [details] Patch

Comment on attachment 61765 [details] Patch Posted wrong patch. I'm sorry for disturbing you...

Created attachment 66691 [details] Patch

As Bug 41832 landed, the patch now has a test and ready for review.

Comment on attachment 66691 [details] Patch > +document.execCommand("Delete", false); > +sel.modify("move", "right", "line"); > +shouldBe("textInputController.hasSpellingMarker(0, 6)", "1"); Can you add some more test cases? I think the current test works without this patch because the cursor is on "Secand" so the word gets checked when you move the cursor off of it. For example, you could also test ForwardDelete or test having multiple misspelled words on the second line. > diff --git a/WebCore/editing/CompositeEditCommand.cpp b/WebCore/editing/CompositeEditCommand.cpp > setEndingSelection(VisibleSelection(start, end, DOWNSTREAM)); > + document()->frame()->editor()->removeMarkersBeforeMovingParagrah(endingSelection()); > deleteSelection(false, false, false, false); Out of curiosity, what happens if you don't remove the markers? What is the benefit of removing the markers? > diff --git a/WebCore/editing/Editor.h b/WebCore/editing/Editor.h > + // Invoked from CompositeEditCommand::moveParagraphs() > + void removeMarkersBeforeMovingParagrah(const VisibleSelection&); > + void markMisspellingsAfterMovingParagrah(const VisibleSelection&); Did you mean to rename these to what Ojan suggested? r- because of this, but will be happy to r+ with the methods renamed.

Created attachment 66839 [details] Patch

Created attachment 66840 [details] Patch

Hi Tony, thank you for reviewing! > Can you add some more test cases? I think the current test works without this patch because the cursor is on "Secand" so the word gets checked when you move the cursor off of it. For example, you could also test ForwardDelete or test having multiple misspelled words on the second line. Oops. The test doesn't make sense as you mentioned. I refreshed with 2 test cases, one for "Delete" and another for "DeleteForward" Both fail without the change. > > Out of curiosity, what happens if you don't remove the markers? What is the benefit of removing the markers? There would be no observable difference. Markers are hold in the Document object using a hash map, and nobody cleanup the map entry even after the DOM node is gone. I think it is a kind of a leak. Although memory for marked-deleted node goes back to the system when once Document is deleted, it stays in WebCore during the document is alive. > > > diff --git a/WebCore/editing/Editor.h b/WebCore/editing/Editor.h > > + // Invoked from CompositeEditCommand::moveParagraphs() > > + void removeMarkersBeforeMovingParagrah(const VisibleSelection&); > > + void markMisspellingsAfterMovingParagrah(const VisibleSelection&); > > Did you mean to rename these to what Ojan suggested? r- because of this, but will be happy to r+ with the methods renamed. Fixed. I'm sorry for missing it.

(In reply to comment #17) > > Out of curiosity, what happens if you don't remove the markers? What is the benefit of removing the markers? > > There would be no observable difference. > Markers are hold in the Document object using a hash map, > and nobody cleanup the map entry even after the DOM node is gone. > I think it is a kind of a leak. > Although memory for marked-deleted node goes back to the system > when once Document is deleted, it stays in WebCore during the document is alive. That makes sense. Thanks for explaining. I agree that it is good to clear the markers rather than wait until the document is destroyed. When you commit, please watch the Snow Leopard bots. I'm not sure if the text will trigger a grammar marker or not (maybe it's not enabled by default on the bot?).

Committed r67049: <http://trac.webkit.org/changeset/67049>

http://trac.webkit.org/changeset/67049 might have broken Qt Linux Release

Created attachment 67161 [details] repro for a remaining problem Reopening this bug because the case attached is not fixed. Current codebase assumes that we need to correct single word at a time. But in this case we should.

Comment on attachment 66840 [details] Patch Removing the r+ so this doesn't show up in the commit queue. In the future, you may just want to open a new bug. It's normally easier to just have one bug per patch to be committed.

> In the future, you may just want to open a new bug. It's normally easier to just have one bug per patch to be committed. Oops. I'm sorry for that. I'll do so next time.