1272623 - After pasting text to Gmail open in FF the text you change with right click - spelling suggestions changes formatting.

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Mel Ilory]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160502172042

Steps to reproduce:

Copied a piece of text from google docs opened in chrome, some of the parts were in bold. 
Pasted this into gmail, into a new message window open in firefox.
Right clicked a word underlined by spellcheck and chose the correct spelling. 


Actual results:

The word which did not have any formatting changed to bold when it was replaced.


Expected results:

The word should have changed to the correct spelling, but the formatting should have stayed the same.

Comment 1

[Andrew Overholt [:overholt]]

Michael, what do you think?

Comment 2

[Nika Layzell [:nika] (ni? for response)]

(In reply to Andrew Overholt [:overholt] from comment #1)
> Michael, what do you think?

Based on my quick investigation, this is occurring because the snippet is copied from google docs like the following:

<b style="font-weight: normal;">
  <p>
    <span style="....">Some random text</span>
    <span style="....">Some more random text</span>
    <span style="....">Some more random text</span>
  </p>
</b>

When the spellchecker replaces one of the segments, it appears as though it notices the enclosing <b> tag, as the word which is replaced is surrounded by a <b> tag. If I change that tag to a <span>, the error doesn't occur.

However, from my experiments spellchecking text not next to a boldness boundary doesn't cause the <b> tag to be inserted, while spellchecking text next to a boldness boundary does cause them to be inserted.

I'm not sure what the exact cause is in code, but I can take a look.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

I've found 2 bugs during looking into this bug, and I've determined that the problem is occurring in the Editor component, so I'm moving this bug into that component.

The first bug which I found was that when a correction is performed on a word which is alone within it's tag, the tag is destroyed. For example:

<span style="text-weight: bold;">Hello, </span><span style="text-weight:normal">Wrold</span> 

would autocorrect causing World to not be in its span. This is due to the line:

    editor->DeleteSelection(nsIEditor::eNone, nsIEditor::eStrip);

where the selection is deleted before the new replacement text is deleted, but with the eStrip flag set. This flag causes any empty tags to be deleted. The InsertText function which is called immediately after this line also deletes the selection if it is not collapsed, and passes eNoStrip, such that it performs text replacement more correctly. Removing this line should fix this bug. I'll attach a patch which does that shortly.

The other bug I've found is related to the <b> tag which appears in the copied text from google docs. In the function `nsHTMLEditor::IsTextPropertySetByContent` which is called from `nsHTMLEditRules::CacheInlineStyles`, the DOM is scanned upwards for a `b` tag, to determine if text which is inserted will need to be wrapped in a <b> tag. As a surrounding <b> tag is found, the inserted text is made bold, even though it shouldn't be bold. I'm not sure what the best way to fix this would be.

Masayuki, Andrew told me that you were interested in working on editor stuff, would you like to take over this bug? Hopefully this comment (and the patch I'll attach soon) gives you a decent starting place for looking into how to fix it.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: DRAFT: Don't destroy nodes during text replacement due to spell checking

Here's the draft patch. It adds a test for the problems I've found, and fixes one of them. It also has a simple pointer towards where I think the problem is with the other patch, which I've explained in more detail in the comment above.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Michael Layzell [:mystor] from comment #3)
> The other bug I've found is related to the <b> tag which appears in the
> copied text from google docs. In the function
> `nsHTMLEditor::IsTextPropertySetByContent` which is called from
> `nsHTMLEditRules::CacheInlineStyles`, the DOM is scanned upwards for a `b`
> tag, to determine if text which is inserted will need to be wrapped in a <b>
> tag. As a surrounding <b> tag is found, the inserted text is made bold, even
> though it shouldn't be bold. I'm not sure what the best way to fix this
> would be.

Hmm, sounds like this is edge case except occurring on a major web service...

I have a question. Why spellchecker needs to wrap the corrected word with same tag? For example, if nesting same element causes different style, that also causes odd result. And for adjusting text style around the wrong word, it's the best not to add any elements to the inserting correct word...

Cannot spellcheck just replace the text in the text node?

> Masayuki, Andrew told me that you were interested in working on editor
> stuff, would you like to take over this bug? Hopefully this comment (and the
> patch I'll attach soon) gives you a decent starting place for looking into
> how to fix it.

Yeah, but I'm not working on editor aggressively because my queue is already fill :-( So, I cannot take this at least in Q2 (perhaps, Q3 too). But you're welcome if you request reviews around editor to me.

Comment 6

[Jet Villegas (inactive)]

Can we boil the STR down to a test?

Comment 7

[Nika Layzell [:nika] (ni? for response)]

I don't have the cycles to work on this.

Comment 8

[Desigan Chinniah [:cyberdees] [:dees] [London - GMT]]

Any suggestions of someone to further investigate this :mystor + :jet?

We have a line of communication with Gmail folks as well - if helpful...

Comment 9

[Nika Layzell [:nika] (ni? for response)]

I don't know who would be good to handle this, as I am not familiar with who is familiar with Editor. Masayuki, do you know who would be able to handle this.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry for the delay to reply.

The bug which you found is reproducible with normal text input from keyboard.
https://jsfiddle.net/d_toybox/n9eaffzd/

So, looks like that we shouldn't cache the style when insertText is called with non-collapsed selection.

I'll try to fix this with your patch today because today is my last day of this year and only today isn't enough to work on my most important bug.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4c08711103b

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1272623 part.1 Don't destroy nodes during text replacement due to spell checking

Spellchecker shouldn't delete selection with nsIEditor::eStrip before calling nsIPlaintextEditor::InsertText() because it causes removing some parent nodes which may have some style rules.

Review commit: https://reviewboard.mozilla.org/r/101094/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101094/

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1272623 part.2 Add automated tests of testing cached inline style at editing

Review commit: https://reviewboard.mozilla.org/r/101096/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101096/

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1272623 part.3 HTMLEditRules::ReapplyCachedStyles() shouldn't set style to TypeInState if it's currently applied

The target node of HTMLEditRules::ReapplyCachedStyles() may be styled with its parent.  When HTMLEditRules::ReapplyCachedStyles() is called, it shouldn't restore another style cache if it's already specified in current DOM tree.

Review commit: https://reviewboard.mozilla.org/r/101098/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101098/

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8822034 [details]
Bug 1272623 part.1 Don't destroy nodes during text replacement due to spell checking

https://reviewboard.mozilla.org/r/101094/#review101602

r=masayuki for the draft patch of Michael Layzell.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8822035 [details]
Bug 1272623 part.2 Add automated tests of testing cached inline style at editing

https://reviewboard.mozilla.org/r/101096/#review101620

rs+

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8822036 [details]
Bug 1272623 part.3 HTMLEditRules::ReapplyCachedStyles() shouldn't set style to TypeInState if it's currently applied

https://reviewboard.mozilla.org/r/101098/#review101622

::: editor/libeditor/HTMLEditRules.cpp:235
(Diff revision 1)
> +    StyleCache(nsGkAtoms::backgroundColor, EmptyString(), EmptyString()));
> +  aStyleCache.AppendElement(
> +    StyleCache(nsGkAtoms::sub, EmptyString(), EmptyString()));
> +  aStyleCache.AppendElement(
> +    StyleCache(nsGkAtoms::sup, EmptyString(), EmptyString()));
>  }

Could we assert here that aStyleCache's length is SIZE_STYLE_TABLE

::: editor/libeditor/HTMLEditRules.cpp:7215
(Diff revision 1)
>                                               &bFirst, &bAny, &bAll,
>                                               &curValue, false);
>          NS_ENSURE_SUCCESS(rv, rv);
>        }
> -      // this style has disappeared through deletion.  Add to our typeinstate:
> -      if (!bAny || IsStyleCachePreservingAction(mTheAction)) {
> +      // This style has disappeared through deletion.  Let's add the styles to
> +      // mTypeInState when same style isn't applied to the node actually.

Maybe "when same style isn't applied to the node already."

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8822036 [details]
Bug 1272623 part.3 HTMLEditRules::ReapplyCachedStyles() shouldn't set style to TypeInState if it's currently applied

https://reviewboard.mozilla.org/r/101098/#review101622

> Could we assert here that aStyleCache's length is SIZE_STYLE_TABLE

Oh, thanks. And I rewrite the loop count of for loops to TArray<>.Length().

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8822036 [details]
Bug 1272623 part.3 HTMLEditRules::ReapplyCachedStyles() shouldn't set style to TypeInState if it's currently applied

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101098/diff/1-2/

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8822036 [details]
Bug 1272623 part.3 HTMLEditRules::ReapplyCachedStyles() shouldn't set style to TypeInState if it's currently applied

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101098/diff/2-3/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf6b592e0700
part.1 Don't destroy nodes during text replacement due to spell checking r=masayuki
https://hg.mozilla.org/integration/autoland/rev/e2e4ab888d78
part.2 Add automated tests of testing cached inline style at editing r=smaug
https://hg.mozilla.org/integration/autoland/rev/063782053b4a
part.3 HTMLEditRules::ReapplyCachedStyles() shouldn't set style to TypeInState if it's currently applied r=smaug

Comment 22

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/0cc483e27a8e for reports of "negative leaks found", mostly in jetpack like https://treeherder.mozilla.org/logviewer.html#?job_id=8452244&repo=autoland but also in other suites like https://treeherder.mozilla.org/logviewer.html#?job_id=8452202&repo=autoland and https://treeherder.mozilla.org/logviewer.html#?job_id=8451232&repo=autoland

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e097b743d5f3

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f75422ba9504

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d51dd46761a3

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f34fbfd8cd0f

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=434022fb6abd

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=721e104bb43e

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e13bba78f061

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8822034 [details]
Bug 1272623 part.1 Don't destroy nodes during text replacement due to spell checking

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101094/diff/1-2/

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8822035 [details]
Bug 1272623 part.2 Add automated tests of testing cached inline style at editing

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101096/diff/1-2/

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8822036 [details]
Bug 1272623 part.3 HTMLEditRules::ReapplyCachedStyles() shouldn't set style to TypeInState if it's currently applied

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101098/diff/3-4/

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8822036 [details]
Bug 1272623 part.3 HTMLEditRules::ReapplyCachedStyles() shouldn't set style to TypeInState if it's currently applied

I give up to fix the memory leak "bug" because I've not found the cause yet. Even when I clear the array when HTMLEditRules is detached from editor and/or "unlink" of cycle collector, I failed to fix the leak.

This means that we cannot allocate memory from HTMLEditRules nor TextEditRules dynamically. However, it's not worthwhile to stay this bug more days/weeks. So, I decided to use fixed length array instead of nsTArray. This is not so pretty, but not so ugly (I hope).

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8822036 [details]
Bug 1272623 part.3 HTMLEditRules::ReapplyCachedStyles() shouldn't set style to TypeInState if it's currently applied

https://reviewboard.mozilla.org/r/101098/#review103616

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/923cb5583464
part.1 Don't destroy nodes during text replacement due to spell checking r=masayuki
https://hg.mozilla.org/integration/autoland/rev/9714d8fd2ba1
part.2 Add automated tests of testing cached inline style at editing r=smaug
https://hg.mozilla.org/integration/autoland/rev/0cd8f95c2adc
part.3 HTMLEditRules::ReapplyCachedStyles() shouldn't set style to TypeInState if it's currently applied r=smaug

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/923cb5583464
https://hg.mozilla.org/mozilla-central/rev/9714d8fd2ba1
https://hg.mozilla.org/mozilla-central/rev/0cd8f95c2adc