303727 - Supply a supported way to access the nsIEditor of HTML input/textarea

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Brian Ryner (not reading)]

Currently, there's no supported way to access the inner <div> for HTML
input/textarea elements.  There are some interesting use cases for doing this
(see the spellcheck feature of Google's Firefox toolbar), but right now it
requires writing native code that depends on unfrozen interfaces.  I'm proposing
we add a new interface which is supported by input and textarea elements and
provides access to the inner div element.  Because of the possibility of using
this to trick the user, the functionality would only be made available to chrome.

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 191832 [details] [diff] [review]
patch

+/**
+ * This interface is implemented by elements which have inner editable
content,
+ * such as HTML input and textarea. 
+ *
+ * @status FROZEN
+*/
+
+[scriptable, uuid(c4a71f8e-82ba-49d7-94f9-beb359361072)]
+interface nsIDOMNSEditableElement : nsISupports
+{
+  readonly attribute nsIDOMElement innerDiv;
+};

Exposing internals like the fact that we've got a div inside our editable
elements in a frozen interface seems like something that could come back and
haunt us later on... What's the actual need for this? Who would need this inner
div (and what if there are more than one?), wouldn't it make more sense to
return the editor for the element instead? And what if there's more than one,
like with some of the whatwg widgets I could see us having more than one editor
in a single HTML widget...

- In content/base/public/nsContentUtils.h

+  /**
+   * Get the first native anonymous child element for a node.
+   */
+  static already_AddRefed<nsIDOMElement>
+  GetAnonymousChildElement(nsIContent *aContent);

Given the current use of this, this would IMO fit better into
nsGenericHTMLElement, or one of the related classes.

I don't see anything in this diff that prevents content code from getting at
the inner div if exposed this way, content code can QI just as easily as chrome
code can. If you want this to be callable from chrome only, you'd need some
nsContentUtils::IsCallerChrome() checks somewhere...

Comment 3

[Brian Ryner (not reading)]

(In reply to comment #2)
> haunt us later on... What's the actual need for this? Who would need this 
inner
> div (and what if there are more than one?), wouldn't it make more sense to
> return the editor for the element instead? And what if there's more than one,
> like with some of the whatwg widgets I could see us having more than one 
editor
> in a single HTML widget...

I gave a use case in my initial comment... basically it's useful for any kind 
of annotation that someone could want to do to our text inputs.


> return the editor for the element instead? And what if there's more than one,
> like with some of the whatwg widgets I could see us having more than one 
editor
> in a single HTML widget...

Example?

> Given the current use of this, this would IMO fit better into
> nsGenericHTMLElement, or one of the related classes.

Sure, that's fine.

> I don't see anything in this diff that prevents content code from getting at
> the inner div if exposed this way, content code can QI just as easily as 
chrome
> code can. If you want this to be callable from chrome only, you'd need some
> nsContentUtils::IsCallerChrome() checks somewhere...

If I try this, I get:

Error: uncaught exception: Permission denied to get property 
HTMLDivElement.style

I'm pretty sure native-anonymous content is already protected from 
modification by untrusted content.

Comment 4

[Brian Ryner (not reading)]

Attachment: another try

It's not great, because nsIEditor isn't frozen, but it's better than where
we're at now.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 194617 [details] [diff] [review]
another try

Yeah, that's IMO much better, even if we're exposing non-frozen interfaces to
scripts that really care. r+sr=jst

Comment 6

[Brian Ryner (not reading)]

Checked in on the trunk.

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 194617 [details] [diff] [review]
another try

I'd really like to get this in for 1.5, even though it's clearly an
enhancement.  Here's my thinking:

1. It does not affect existing code paths
2. The new functionality is only available to chrome, so it shouldn't be a
security problem.
3. It provides a much-needed piece of functionality for extension authors.

Comment 8

[Asa Dotzler [:asa]]

Comment on attachment 194617 [details] [diff] [review]
another try

It's late in the game and we have a lot more to be concerned with right now. We
realize you were looking for an exception to the "it's too late for features"
and this got a lot of discussion before I minused it. Hopefully Google can do
something with native code.

Comment 9

[[e-mail]]

Since nsIDOMNSHTMLInputElement and nsIDOMNSHTMLTextAreaElement are logically
identical is there a case for moving everything into nsIDOMNSEditableElement?

Comment 10

[Darin Fisher]

> It's late in the game and we have a lot more to be concerned with right now. 
> We realize you were looking for an exception to the "it's too late for 
> features" and this got a lot of discussion before I minused it. Hopefully 
> Google can do something with native code.

Asa: I think we should take this patch for Firefox 1.5.  It provides a good,
stable API that has very little associated risk.  It is not affecting existing
codepaths.  It simply exposes a property that is otherwise difficult to access.  

This patch specifically would enable a certain toolbar extension to avoid having
to code to a bunch of unfrozen interfaces (e.g., nsIDocShell, nsIPresShell, and
nsIContent).  Moreover, those are interfaces that we are very likely to want to
change as we apply other patches to Gecko during the run-up to Firefox 1.5 final
(e.g., see the proposed patches to improve the memory utilization of fastback).

I think it would be silly for us to not accept this patch on the grounds that we
are done taking features.  This is not just a feature, it's an API that makes it
easier for extensions to do cool stuff with less risk of being broken by core
changes to Gecko.  We like APIs that are easy to support in the future! ;-)

Comment 11

[Darin Fisher]

Comment on attachment 194617 [details] [diff] [review]
another try

Please see my previous comment in this bug.  I think we should include this
patch for Firefox 1.5b2.

Comment 12

[Darin Fisher]

So Brendan said that he was in favor of this patch on the 1.8 branch provided
that we hide the ".editor" property from content.  From the patch it appears
that we're just making the property throw if called by content.  So, how easy
would it be to hide it altogether?

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'm not sure there's an obvious way to hide it, given XPConnect's interface
flattening.  Once someone QIs to this interface on an actual content node, the
property will be on the wrapper...

That said, should <xul:textbox> implement this interface, perhaps?

And should code that has UniversalXPConnect but isn't chrome be able to access this?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Ah, brendan was suggesting hiding via classinfo...

That could maybe be done.  You'd have to only resolve and get the prop if the
caller has the right privileges, and make sure that it's using JSPROP_SHARED so
it doesn't get a slot on the object.  And of course that way it'll only be
accessible from JS, not from C++ or python or whatever..

Comment 15

[Johnny Stenback (:jst)]

(In reply to comment #14)
> Ah, brendan was suggesting hiding via classinfo...
> 
> That could maybe be done.  You'd have to only resolve and get the prop if the
> caller has the right privileges, and make sure that it's using JSPROP_SHARED so
> it doesn't get a slot on the object.  And of course that way it'll only be
> accessible from JS, not from C++ or python or whatever..

The way to hide this would IMO be to make the new interface a standalone
interface that inherits nothing and nothing inherits from. That means one more
vtable where we implement this, but it also means that this property is hidden
until someone QI's to this new interface, which would only be done by extensions
that use this feature... Sure, this could still break some random pages out
there, *if* an extension is installed that QI's all textareas and inputs on page
load...

That would IMO be acceptable for the 1.8 branch.

Comment 16

[Mike Schroepfer]

If you implement as comment 15 suggests - i.e. hidding the element from content
we'll approve for 1.8b5.  

Comment 17

[Brian Ryner (not reading)]

Attachment: trunk patch to require explicit QI to nsIDOMNSEditableElement

Comment 18

[[e-mail]]

Comment on attachment 196883 [details] [diff] [review]
trunk patch to require explicit QI to nsIDOMNSEditableElement

>-[scriptable, uuid(8aa7dadc-73f1-416e-9d1e-15bc63226938)]
>-interface nsIDOMNSHTMLTextAreaElement : nsIDOMNSEditableElement
>+[scriptable, uuid(ca066b44-9ddf-11d3-bccc-0060b0fc76bd)]
>+interface nsIDOMNSHTMLTextAreaElement : nsISupports
Shouldn't this be a backout rather than a new uuid?

Comment 19

[Brian Ryner (not reading)]

It is.  This patch restores the original uuids.

(In reply to comment #18)
> (From update of attachment 196883 [details] [diff] [review] [edit])
> >-[scriptable, uuid(8aa7dadc-73f1-416e-9d1e-15bc63226938)]
> >-interface nsIDOMNSHTMLTextAreaElement : nsIDOMNSEditableElement
> >+[scriptable, uuid(ca066b44-9ddf-11d3-bccc-0060b0fc76bd)]
> >+interface nsIDOMNSHTMLTextAreaElement : nsISupports
> Shouldn't this be a backout rather than a new uuid?
> 

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 196883 [details] [diff] [review]
trunk patch to require explicit QI to nsIDOMNSEditableElement

r+sr=jst, please put a branch diff in the bug before checking this in on the
branch (since that diff will be the sum of both these diffs).

Comment 21

[Brian Ryner (not reading)]

Attachment: cumulative branch patch

Comment 22

[[e-mail]]

(In reply to comment #19)
>It is.  This patch restores the original uuids.
Sorry, it would have helped if I had looked at the right .idl :-[

Comment 23

[Alex Vincent [:WeirdAl]]

This is pretty cool!

editor = (xulTextbox.inputField instanceof
Components.interfaces.nsIDOMNSEditableElement) ? xulTextbox.inputField.editor : null

Comment 24

[[e-mail]]

Going to add that to the XBL?

Comment 25

[Alex Vincent [:WeirdAl]]

Yes.  Bug 312867.