177700 - midas (designMode)

Status: VERIFIED FIXED

(Core :: DOM: Editor, enhancement)

Description

[Kathleen :Brade]

This is a placeholder bug for the work I am doing.
(I want to keep the cc list much shorter than some of the other bugs that
already exist on this type of functionality.)

Comment 1

[Kathleen :Brade]

Attachment: patch with initial framework

Comment 2

[Akkana Peck]

Comment on attachment 104774 [details] [diff] [review]
patch with initial framework

>+  boolean                   queryCommandEnabled(in DOMString commandID);
>+  boolean                   queryCommandIndeterm(in DOMString commandID);

It's too bad we don't have a good tristate so we don't have to have both of
these (or are these mirroring the MS spec?)

>+  nsAutoString tmp(aDesignMode);
>+  if (tmp.EqualsIgnoreCase("on")) {

I know it's only two characters, but doing runtime conversion every time makes
me twitch.  Turns out putting an NS_LITERAL_STRING here doesn't work (doesn't
compile).  I asked on #mozilla and was told (1) maybe something involving
creating an nsCaseInsensitiveStringComparator() but (2) it probably wouldn't
work right because there are "issues" in doing case-insensitive string
comparisons (e.g. in a turkish locale "file" and "FILE" aren't
case-insensitively equal) and (3) why weren't we using ACStrings for commands
since we know the command will be ascii?

What does the MS spec use?  Is it really unicode and case insensitive?	Should
we consider doing the conversion to ascii before we get to nsHTMLDocument.cpp? 
Simon, any thoughts?

>+    rv = editSession->MakeWindowEditable(domwindow, "html", false);

Will we ever need to pass any other type?

>+nsICommandManager *
>+nsHTMLDocument::GetMidasCommandManager()

Do we want to freeze "Midas" in the code forever?  It's not very
self-explanatory to someone reading the code ...  I'd suggest changing it to
GetEditorCommandManager or GetEmbeddedEditorCommandManager or something.

>+           attribute DOMString            designMode;

Please fix the indentation here.

Marking r=akkana, though I would be happier if you changed the "Midas" in the
code.

Comment 3

[Simon Fraser [no longer active]]

...(in DOMString commandID)...

Are you sure you want to use DOMStrings here? Does the IE spec use unicode
command names?

Was nsISelectionPrivate.idl just a convenient place to put these routines?
Should they go onto a new interface?

+NS_IMETHODIMP
+nsHTMLDocument::SetDesignMode(const nsAString & aDesignMode)

We might need to think about possible security exploits in this code. Can we
guaranetee that the window that gets made editable is always in content?

+nsICommandManager *
+nsHTMLDocument::GetMidasCommandManager()

Erm, please make this routine follow XPCOM rules (nsresult return, addreffed out
param).

+nsHTMLDocument::ConvertToMidasInternalCommand(const nsAString & inCommandID, 
+                                              const nsAString & inParam,
+                                              nsACString& outCommandID,
+                                              nsACString& outParam)

We could always just register our commands under 2 different names, to avoid
having to do all this remapping in this method.

Index: mozilla/dom/public/idl/html/nsIDOMNSHTMLDocument.idl
Is it OK to change this interface now?

Comment 4

[Akkana Peck]

Whoops: I forgot to mention that content/html/document/src/Makefile.in needs two
additions to the REQUIRES line:

Index: Makefile.in
===================================================================
RCS file: /cvsroot/mozilla/content/html/document/src/Makefile.in,v
retrieving revision 1.21
diff -r1.21 Makefile.in
58a59,60
>                 commandhandler \
>                 composer \

Comment 5

[Kathleen :Brade]

Attachment: document.designMode patch (no selection)

This patch is just for the document.  I will post a different patch for
selection when I can actually test that ;-).

Answers to previous questions / conversations:
  * Added Akkana's changes to Makefile.in
  * DOMString is used in all DOM idl files (including all of the frozen ones)
  * All of the "query*" methods are from Microsoft's MSDN website.
  * String comparisons are now done with the case insensitive comparator.
  * Some day, we may extend the current code to handle more than "html" for
editable types.
  * It's ok to use Midas as long as it isn't exposed in any idl files.
  * GetMidasCommandManager now returns nsresult and addrefs
  * a table was created to map the JS strings to our internal Cstrings.

Not addressed with this patch:
  * possibly security exploits if we don't guarantee we are in content?
  * XHTML and other formats

Comment 6

[Akkana Peck]

Comment on attachment 104868 [details] [diff] [review]
document.designMode patch (no selection)

r=akkana
I like the splitting off of the command table.
(I still don't like using "Midas". :-)
Kathy noticed that she listed bold twice.
I found the NS_IF_ADDREF of a possibly null value in GetMidasCommandManager
confusing (it does the right thing, it's just confusing to read) and Kathy has
changed that.
It does build correctly on linux, the Makefile is fine.

Comment 7

[Kathleen :Brade]

Attachment: document designMode (and friends)

This patch updates the previous patch to not do bad thing on Linux and other
Unix flavors (per jst).  It fixes the addref style (per Akkana).  It fixes an
api oddity which jfrancis noticed.

Comment 8

[Akkana Peck]

Comment on attachment 104902 [details] [diff] [review]
document designMode (and friends)

> _retval.SetLength(0);

I think I remember .Truncate() is preferred.

Otherwise r=akkana

Comment 9

[Kathleen :Brade]

The "focus" method has been removed from the idl/cpp files.
I am told that Truncate(0) merely calls SetLength(0) and that SetLength is more
readable so I'm sticking with SetLength unless everyone else disagrees.

jst--super-review?

Comment 10

[Kathleen :Brade]

Attachment: previous patch without focus but includes forced focus to prevent/reduce exploits

This is the previous patch without the "focus()" method.
Instead I am forcing a focus to happen internally.  This saves page authors the
hastle of having to do that themselves and also ensures that the commands are
dispatched to the proper (focused) document (hopefully reducing the likelihood
of an exploit)

I like the auto-focus'ing and think it makes sense:  commands triggered for a
given document should focus that document's window.

comments? reviews?

Comment 11

[Simon Fraser [no longer active]]

> I like the auto-focus'ing and think it makes sense:  commands triggered for a
> given document should focus that document's window.

This worries me, for several reasons:
1. Relying on focus for security is not sufficient. We need some hard security
   checks in there.
2. Changing focus under the user/page author like this seems bad; shouldn't we
   fix commands to be able to go to the controller for an unfocussed window?
3. IIRC, IE requires the page author to make focus() calls. Maybe we should just
   follow suit.

Comment 12

[Kathleen :Brade]

Attachment: forced focus, queryCommandValue impl for mkaply

updated patch with forced focus and an implementation of queryCommandValue for
mkaply because his example needs that

Related to security exploits, we can change the command manager api to directly
route the commands to a specific nsIDOMWindow.	If we have that, I don't think
we need any other security checks but I defer to any security reviews, etc.

Changing focus is actually a good thing, IE has a non-standards-based attribute
that page authors can add to make buttons not grab focus.  Gecko doesn't have
that.  Without similar functionality, we'd require page authors to add lots of
"focus()" calls to the editor widget.  I can only think of a few odd cases
where someone would click on a "bold" button and expect the editor to not have
focus.	I think it's very odd for a command to be triggered in editor and the
editor to not have focus.  I defer to the majority or aaronl for this specific
issue.

Comment 13

[Kathleen :Brade]

Attachment: above + fix querycommandstate for alignment case in mkaply's example

Comment 14

[Akkana Peck]

Comment on attachment 105333 [details] [diff] [review]
above  + fix querycommandstate for alignment case in mkaply's example

> +  // always set focus to this widget so the command can be executed only in
> +  // this scope (we wouldn't want to be able to execute commands
> +  nsCOMPtr<nsIDOMWindowInternal> window = do_QueryInterface(mScriptGlobalObject);

Were you interrupted while writing that comment?  Or is it complete except for
missing a close-paren?	Likewise for later in the patch where the same partial
comment is repeated.  

I wondered about the focus thing too, but after reading Kathy's explanation I
have to agree: doing the right thing by default is better than forcing authors
to call focus() everywhere, since inevitably they'll forget and then it'll be
partly our fault that some app that uses us sucks.  (Remember how XUL dialogs 
didn't use to take focus when they popped up?  Ugh.)

> +  // we need to handle alignment as a special case (possibly other commands too?)

I had to ask about why (it's because MS handles them as separate commands, but
it's one command for us).  It might be good to explain that in the comment.

> 

Do we need buy-in from anyone in content for adding these new Midas methods to
nsHTMLDocument?

You have some lines >80 columns, in case you care (that's an item in the new
moz code standards document).

r=akkana with the comment fixes, line length change is optional.

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 105333 [details] [diff] [review]
above  + fix querycommandstate for alignment case in mkaply's example

- In nsHTMLDocument.h:

+  /* Midas implementation */
+  NS_IMETHOD GetMidasCommandManager(nsICommandManager** aCommandManager);

Since this is not part of an interface you should *not* use NS_IMETHOD as the
return type here. I don't see why this would even need to be virtual, just
nsresult would be enough here.

- In nsHTMLDocument.cpp:

+NS_IMETHODIMP
+nsHTMLDocument::GetMidasCommandManager(nsICommandManager** aCmdMgr)

Same here, just nsresult in stead of NS_IMETHODIMP.

- In nsHTMLDocument::ConvertToMidasInternalCommand():

+  nsCAutoString convertedCommandID = NS_ConvertUCS2toUTF8(inCommandID);

You could make convertedCommandID and nsACString& to save a string object
construction and a string copy...

+
(convertedCommandID.EqualsIgnoreCase(gMidasCommandTable[i].incomingCommandStrin
g)) { 

and use convertedCommandID.Equals(..., nsCaseInsensitiveCStringComparator())
here.

Other than that, sr=jst

Comment 16

[Joe Francis]

can we give this a milestone, even if a fake one?  Otherwise I keep seeing it
when querying for untriaged editor core bugs.

Comment 17

[Kathleen :Brade]

jfrancis--I'll do it one better
  -->fixed

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It is NOT OK to use "Midas" in identifiers even if they are only internal. When
the Midas project is long over and done with, no-one will know what the heck it
means. It's just one more code word that people hacking on Gecko will have to
learn and remember. Would it have killed you to use "ContentEditor" or something?

Comment 19

[sairuh (rarely reading bugmail)]

midas is in. :)