All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: kalithae, Assigned: masayuki)

Tracking

(Depends on: 1 bug)

Trunk
mozilla53
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(platform-rel +, firefox53 fixed)

Details

(Whiteboard: btpp-followup-2016-05-27 [platform-rel-Google] [platform-rel-Gmail])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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.

Updated

3 years ago
Component: Untriaged → Serializers
Product: Firefox → Core
Michael, what do you think?
Flags: needinfo?(michael)
Whiteboard: btpp-followup-2016-05-27

Comment 2

2 years ago
(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.
Flags: needinfo?(michael)

Comment 3

2 years ago
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.
Component: Serializers → Editor
Flags: needinfo?(masayuki)

Comment 4

2 years ago
Created attachment 8754998 [details] [diff] [review]
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.
(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.
Flags: needinfo?(masayuki)
Whiteboard: btpp-followup-2016-05-27 → btpp-followup-2016-05-27 [platform-rel-Google] [platform-rel-Gmail]

Updated

2 years ago
platform-rel: --- → ?

Updated

2 years ago
platform-rel: ? → +

Updated

2 years ago
Assignee: nobody → michael
Can we boil the STR down to a test?
Depends on: 1277113

Updated

2 years ago
Rank: 2

Comment 7

2 years ago
I don't have the cycles to work on this.
Assignee: michael → nobody
Any suggestions of someone to further investigate this :mystor + :jet?

We have a line of communication with Gmail folks as well - if helpful...
Flags: needinfo?(michael)
Flags: needinfo?(bugs)

Comment 9

2 years ago
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.
Flags: needinfo?(michael) → needinfo?(masayuki)
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(masayuki)
Flags: needinfo?(bugs)
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
OS: Unspecified → All
Hardware: Unspecified → All
Version: 46 Branch → Trunk
Attachment #8754998 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 years ago
mozreview-review
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.
Attachment #8822034 - Flags: review?(masayuki) → review+

Comment 16

2 years ago
mozreview-review
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+
Attachment #8822035 - Flags: review?(bugs) → review+

Comment 17

2 years ago
mozreview-review
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."
Attachment #8822036 - Flags: review?(bugs) → review+
(Assignee)

Comment 18

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

2 years ago
Pushed by masayuki@d-toybox.com:
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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).
Attachment #8822036 - Flags: review+ → review?(bugs)

Comment 34

2 years ago
mozreview-review
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
Attachment #8822036 - Flags: review?(bugs) → review+

Comment 35

2 years ago
Pushed by masayuki@d-toybox.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/923cb5583464
https://hg.mozilla.org/mozilla-central/rev/9714d8fd2ba1
https://hg.mozilla.org/mozilla-central/rev/0cd8f95c2adc
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.