348695 - No spelling items in context menu in Midas iframes

Status: RESOLVED FIXED

(Camino Graveyard :: OS Integration, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Spell-checking works in Midas iframes (e.g., the demo above, and Gmail's rich-text view), but we don't include any spelling-related items in the context menu (i.e., no guesses).

We're showing the frame context menu in those cases (page cm plus frame items), whereas Firefox appears to only show the text context menu (and spelling items) on Midas iframes.

Comment 1

[Samuel Sidler (old account; do not CC)]

*** Bug 357546 has been marked as a duplicate of this bug. ***

Comment 2

[Stuart Morgan]

Taking. I have this hacked up and working, I just need to clean up the code.

Comment 3

[Stuart Morgan]

Attachment: fix

Comment 4

[Nick Kreeger]

Comment on attachment 246159 [details] [diff] [review]
fix

Looks pretty good, a couple of comments:

+    #define ENSURE_TRUE(x) if (!x) return;

Instead of redefining |ENSURE_TRUE()| twice, how about defining it once, perhaps using a different name since the |ENSURE_TRUE| macro in gecko takes two params (1. the nsresult value, 2. the return value). Also do you we care about the nsresult values of these calls?


+  if (editElement) {
     editElement->GetEditor(outEditor);
+  }

Sort of a minor nit, any reason you wanted to add the curlies here?

* This will need a project file update, we need to include the "../dist/include/composer" header path for nsIEditingSession.h

Tested and works great! Is this something we want for 1.1, I would find it very useful.

r=me with a resolution on the |ENSURE_TRUE| issue.

Comment 5

[Stuart Morgan]

(In reply to comment #4)
> Instead of redefining |ENSURE_TRUE()| twice, how about defining it once,
> perhaps using a different name since the |ENSURE_TRUE| macro in gecko takes two
> params (1. the nsresult value, 2. the return value). Also do you we care about
> the nsresult values of these calls?

It's already defined that way 3 times in BWC; I was just following the existing model. If it's a problem, we should clean it all up at once later.

> Sort of a minor nit, any reason you wanted to add the curlies here?

We generally don't seem to do
  if
    foo
  else {
    ...
  }
(i.e., not mixing-and matching curlies), so I added them to match the else.

> * This will need a project file update, we need to include the
> "../dist/include/composer" header path for nsIEditingSession.h

Thanks, I totally forgot to make the project patch in the midst of dealing with all the Midas code.

> Is this something we want for 1.1

This is for 1.1

Comment 6

[Stuart Morgan]

Attachment: with project change

Comment 7

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 246367 [details] [diff] [review]
with project change

sr=pink.

where else might we get the assumption wrong that no focused element means we have nothing to do? is there a way to abstract this so we don't have to worry about it elsewhere?

Comment 8

[Stuart Morgan]

Attachment: branch version

Unfortunately, one of the steps uses a call that's not in the 1.8 API.  This is a slightly different approach to getting the doc shell for a window (using nsIScriptGlobalObject instead of the more obviously related nsPIDOMWindow), cribbed from 1.8 core code.

Comment 9

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 246889 [details] [diff] [review]
branch version

sr=pink

Comment 10

[Stuart Morgan]

Checked in on trunk and MOZILLA_1_8_BRANCH.