No spelling items in context menu in Midas iframes

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
OS Integration
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: Stuart Morgan)

Tracking

({fixed1.8.1.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1.1

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

15.91 KB, patch
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
16.14 KB, patch
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
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.
*** Bug 357546 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 2

11 years ago
Taking. I have this hacked up and working, I just need to clean up the code.
Assignee: nobody → stuart.morgan
(Assignee)

Comment 3

11 years ago
Created attachment 246159 [details] [diff] [review]
fix
Attachment #246159 - Flags: review?

Comment 4

11 years ago
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.
Attachment #246159 - Flags: review? → review+
(Assignee)

Comment 5

11 years ago
(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
(Assignee)

Comment 6

11 years ago
Created attachment 246367 [details] [diff] [review]
with project change
Attachment #246159 - Attachment is obsolete: true
Attachment #246367 - Flags: superreview?(mikepinkerton)
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?
Attachment #246367 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 8

11 years ago
Created attachment 246889 [details] [diff] [review]
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.
Attachment #246889 - Flags: superreview?(mikepinkerton)
Comment on attachment 246889 [details] [diff] [review]
branch version

sr=pink
Attachment #246889 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 10

11 years ago
Checked in on trunk and MOZILLA_1_8_BRANCH.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.