Bug 41423

Summary: spelling underline gets lost on backspace
Product: WebKit Reporter: Ojan Vafai <ojan>
Component: HTML EditingAssignee: Hajime Morrita <morrita>
Status: REOPENED ---    
Severity: Normal CC: abarth, adele, enrica, eric, hbono, jparent, justin.garcia, morrita, rniwa, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 41832    
Bug Blocks:    
Attachments:
Description Flags
test case
none
a preliminary patch
none
another repro (with automation)
none
updated. has no test yet.
none
Patch
none
Patch
none
Patch
none
Patch
none
repro for a remaining problem none

Description Ojan Vafai 2010-06-30 11:17:48 PDT
Created attachment 60134 [details]
test case

See the test case. Happens in webkit nightly and chrome dev channel.
Comment 1 Hironori Bono 2010-06-30 22:10:06 PDT
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,
Comment 2 Hajime Morrita 2010-07-13 04:58:39 PDT
Created attachment 61357 [details]
a preliminary patch
Comment 3 Hajime Morrita 2010-07-13 05:00:13 PDT
Created attachment 61358 [details]
another repro (with automation)
Comment 4 Hajime Morrita 2010-07-13 05:01:27 PDT
> a preliminary patch
This is just a preliminary version and has no test.
Comment 5 Ojan Vafai 2010-07-13 14:31:39 PDT
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?
Comment 6 Hajime Morrita 2010-07-14 01:20:49 PDT
Created attachment 61481 [details]
updated. has no test yet.
Comment 7 Hajime Morrita 2010-07-14 01:25:29 PDT
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.
Comment 8 Justin Garcia 2010-07-14 12:25:15 PDT
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?
Comment 9 Hajime Morrita 2010-07-15 20:57:50 PDT
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.
Comment 10 Hajime Morrita 2010-07-15 22:19:28 PDT
Created attachment 61765 [details]
Patch
Comment 11 Hajime Morrita 2010-07-15 22:20:58 PDT
Comment on attachment 61765 [details]
Patch

Posted wrong patch. I'm sorry for disturbing you...
Comment 12 Hajime Morrita 2010-09-07 01:36:41 PDT
Created attachment 66691 [details]
Patch
Comment 13 Hajime Morrita 2010-09-07 01:39:45 PDT
As Bug 41832 landed, the patch now has a test and ready for review.
Comment 14 Tony Chang 2010-09-07 14:35:22 PDT
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.
Comment 15 Hajime Morrita 2010-09-07 20:56:10 PDT
Created attachment 66839 [details]
Patch
Comment 16 Hajime Morrita 2010-09-07 21:04:40 PDT
Created attachment 66840 [details]
Patch
Comment 17 Hajime Morrita 2010-09-07 21:05:03 PDT
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.
Comment 18 Tony Chang 2010-09-08 09:33:49 PDT
(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?).
Comment 19 Hajime Morrita 2010-09-08 19:05:41 PDT
Committed r67049: <http://trac.webkit.org/changeset/67049>
Comment 20 WebKit Review Bot 2010-09-08 19:28:11 PDT
http://trac.webkit.org/changeset/67049 might have broken Qt Linux Release
Comment 21 Hajime Morrita 2010-09-10 00:33:51 PDT
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 22 Tony Chang 2010-09-10 09:57:37 PDT
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.
Comment 23 Hajime Morrita 2010-09-12 18:24:01 PDT
> 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.