Bug 41423 – spelling underline gets lost on backspace

• Component: HTML Editing
• Status: REOPENED

[Ojan Vafai]

Created attachment 60134 [details]
test case

See the test case. Happens in webkit nightly and chrome dev channel.

[Hironori Bono]

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,

[Hajime Morrita]

Created attachment 61357 [details]
a preliminary patch

[Hajime Morrita]

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

[Hajime Morrita]

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

[Ojan Vafai]

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?

[Hajime Morrita]

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

[Hajime Morrita]

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.

[Justin Garcia]

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?

[Hajime Morrita]

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.

[Hajime Morrita]

Created attachment 61765 [details]
Patch

[Hajime Morrita]

Comment on attachment 61765 [details]
Patch

Posted wrong patch. I'm sorry for disturbing you...

[Hajime Morrita]

Created attachment 66691 [details]
Patch

[Hajime Morrita]

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

[Tony Chang]

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.

[Hajime Morrita]

Created attachment 66839 [details]
Patch

[Hajime Morrita]

Created attachment 66840 [details]
Patch

[Hajime Morrita]

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.

[Tony Chang]

(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?).

[Hajime Morrita]

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

[WebKit Review Bot]

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

[Hajime Morrita]

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.

[Tony Chang]

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.

[Hajime Morrita]

> 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.