Last Comment Bug 737889 - Spell checking is broken when typing Facebook messages
: Spell checking is broken when typing Facebook messages
Status: RESOLVED FIXED
: testcase
Product: Core
Classification: Components
Component: Spelling checker (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: :Ehsan Akhgari
:
:
Mentors:
data:text/html,<body onload="document...
Depends on: 612128 741216
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-21 08:39 PDT by :Ehsan Akhgari
Modified: 2012-05-08 10:48 PDT (History)
5 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (3.29 KB, patch)
2012-03-21 15:48 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Patch (v1) (3.69 KB, patch)
2012-03-22 10:47 PDT, :Ehsan Akhgari
roc: review+
Details | Diff | Splinter Review
Part 2 (1.32 KB, patch)
2012-03-22 16:01 PDT, :Ehsan Akhgari
roc: review+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2012-03-21 08:39:34 PDT
This is similar to bug 734530, but is not fixed by the patch there. :(
Comment 1 Timothy Nikkel (:tnikkel) 2012-03-21 13:23:05 PDT
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.
Comment 2 :Ehsan Akhgari 2012-03-21 15:06:14 PDT
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.
Comment 3 :Ehsan Akhgari 2012-03-21 15:48:31 PDT
Created attachment 608125 [details] [diff] [review]
Patch (v1)
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-03-21 16:08:16 PDT
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?)
Comment 5 Mozilla RelEng Bot 2012-03-21 17:47:24 PDT
Try run for 063db6b952f4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=063db6b952f4
Results (out of 14 total builds):
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-063db6b952f4
Comment 6 :Ehsan Akhgari 2012-03-22 10:47:00 PDT
Created attachment 608391 [details] [diff] [review]
Patch (v1)

Sorry. that was the wrong patch!
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-03-22 11:12:55 PDT
Comment on attachment 608391 [details] [diff] [review]
Patch (v1)

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

That makes more sense :-)
Comment 8 :Ehsan Akhgari 2012-03-22 16:01:53 PDT
Created attachment 608516 [details] [diff] [review]
Part 2

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.
Comment 9 Mozilla RelEng Bot 2012-03-22 16:31:51 PDT
Try run for c223dd45b550 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c223dd45b550
Results (out of 221 total builds):
    exception: 5
    success: 141
    warnings: 34
    failure: 41
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-c223dd45b550
Comment 12 :Ehsan Akhgari 2012-05-08 10:48:21 PDT
Adding a dependency to make sure that bug 612128 does not break this.

Note You need to log in before you can comment on or make changes to this bug.