Closed Bug 737889 Opened 10 years ago Closed 10 years ago

Spell checking is broken when typing Facebook messages


(Core :: Spelling checker, defect)

Not set





(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)




(Keywords: testcase)


(2 files, 1 obsolete file)

This is similar to bug 734530, but is not fixed by the patch there. :(
Doesn't seem to be due to a recent regression in Firefox. I tried builds back to 2008 and none of them underlined misspelled words. Might be due to a more recent change to Facebook's website though, I don't know.

When I get the "popup" 'New Message' box that looks like its on top of the page contents when you send a message to someone you haven't sent a message to before the spelling checker does seem to work. Just when you get a reply box in an existing "conversation" is a problem for me.
OK I have a test case now.  There are actually two bugs involved here, bug 734530 and this bug.  We basically don't update the editable state of the anonymous children of the textbox when its readonly attribute is removed, and the spell checker has code to make sure it would never check non-editable content, which causes the textbox to not be spell checked.

This is indeed not a recent regression.
Attached patch Patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ehsan
Attachment #608125 - Flags: review?(roc)
Comment on attachment 608125 [details] [diff] [review]
Patch (v1)

Review of attachment 608125 [details] [diff] [review]:

::: content/html/content/src/nsHTMLTextAreaElement.cpp
@@ +1296,5 @@
> +      mState->UpdateEditableState(aNotify);
> +      nsTextEditorState *state = GetEditorState();
> +      if (state) {
> +        state->UpdateEditableState(aNotify);
> +      }

I don't understand why you need to update GetEditorState(). Isn't mState the only state we need to worry about for textareas?

(And wouldn't it make sense for GetEditorState() to actually return mState for textareas?)
Try run for 063db6b952f4 is complete.
Detailed breakdown of the results available here:
Results (out of 14 total builds):
    failure: 14
Builds (or logs if builds failed) available at:
Attached patch Patch (v1)Splinter Review
Sorry. that was the wrong patch!
Attachment #608125 - Attachment is obsolete: true
Attachment #608125 - Flags: review?(roc)
Attachment #608391 - Flags: review?(roc)
Comment on attachment 608391 [details] [diff] [review]
Patch (v1)

Review of attachment 608391 [details] [diff] [review]:

That makes more sense :-)
Attachment #608391 - Flags: review?(roc) → review+
Attached patch Part 2Splinter Review
This patch triggered a test failure which seems to be a bug that we've had for ever.  If you drop something over a non-editable content on a page with an editor, we fail to stop the event propagation, which means that the code in contentAreaDropListener.js kicks in and tries to do a link navigation (among other things).  For example, if the dropped text is "foo bar", we end up trying to navigate to http://foo%20bar which results in an error page.

This patch fixes that problem.
Attachment #608516 - Flags: review?(roc)
Try run for c223dd45b550 is complete.
Detailed breakdown of the results available here:
Results (out of 221 total builds):
    exception: 5
    success: 141
    warnings: 34
    failure: 41
Builds (or logs if builds failed) available at:
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 741216
Adding a dependency to make sure that bug 612128 does not break this.
Depends on: 612128
You need to log in before you can comment on or make changes to this bug.