458037 - Implement isContentEditable

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

See http://www.whatwg.org/specs/web-apps/current-work/#contenteditable
"The isContentEditable DOM attribute, on getting, must return true if the element is editable, and false otherwise."

Comment 1

[Jeff Muizelaar [:jrmuizel]]

The Web Compatibility Test for Mobile Browsers (http://www.w3.org/2010/01/wctmb2/) tests this attribute which causes us to fail it's contenteditable test.

Comment 2

[(no longer active)]

Attachment: Patch (v1)

Comment 3

[(no longer active)]

(In reply to comment #1)
> The Web Compatibility Test for Mobile Browsers
> (http://www.w3.org/2010/01/wctmb2/) tests this attribute which causes us to
> fail it's contenteditable test.

This patch causes the value to jump from 84% to 92%.  Not sure if that's of any value, though.

Comment 4

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

+          *aContentEditable = (value == eTrue) ? PR_TRUE : PR_FALSE;

This can just be "*aContentEditable = value == eTrue;"

You could rewrite this as something like
  for (nsIContent* node = this; node; node = node->GetParent()) {
    ContentEditableTristate value = node->GetContentEditableValue();
    if (value != inherit)
      return ...;
  }
  return false;

But don't we have code in editor already that checks whether the element is editable?

Comment 5

[(no longer active)]

Attachment: Patch (v1.1)

(In reply to comment #4)
> +          *aContentEditable = (value == eTrue) ? PR_TRUE : PR_FALSE;
> 
> This can just be "*aContentEditable = value == eTrue;"

I did that.  But out of curiosity, don't we want to avoid the automatic conversion of a bool to an int (which is what PRBool is)?

> You could rewrite this as something like
>   for (nsIContent* node = this; node; node = node->GetParent()) {
>     ContentEditableTristate value = node->GetContentEditableValue();
>     if (value != inherit)
>       return ...;
>   }
>   return false;

Not really, since GetContentEditableValue is not a member of nsIContent...

> But don't we have code in editor already that checks whether the element is
> editable?

We have nsEditor::IsEditable, but its logic is totally different to what we want here.

Comment 6

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

(In reply to comment #5)
> (In reply to comment #4)
> > +          *aContentEditable = (value == eTrue) ? PR_TRUE : PR_FALSE;
> > 
> > This can just be "*aContentEditable = value == eTrue;"
> 
> I did that.  But out of curiosity, don't we want to avoid the automatic
> conversion of a bool to an int (which is what PRBool is)?

No, why would we?

> > You could rewrite this as something like
> >   for (nsIContent* node = this; node; node = node->GetParent()) {
> >     ContentEditableTristate value = node->GetContentEditableValue();
> >     if (value != inherit)
> >       return ...;
> >   }
> >   return false;
> 
> Not really, since GetContentEditableValue is not a member of nsIContent...

Then
   for (nsIContent* node = this; node; node = node->GetParent()) {
     if (node->IsHTML()) {
       nsGenericHTMLElement* element = static_cast<nsGenericHTMLElement*>(node);
       ContentEditableTristate value = node->GetContentEditableValue();
       if (value != inherit)
         return ...;
     }
   }
   return false;

> We have nsEditor::IsEditable, but its logic is totally different to what we
> want here.

OK

Comment 7

[Boris Zbarsky [:bzbarsky]]

>       nsGenericHTMLElement* element = static_cast<nsGenericHTMLElement*>(node);

Or use nsGenericHTMLElement::FromContent.

Comment 8

[(no longer active)]

Attachment: Patch (v1.2)

Addressed review comments.

Comment 9

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

Now you've changed the behaviour so that if an HTML element has a non-HTML parent, isContentEditable will return false. I think in that case you're supposed to find the nearest HTML ancestor and keep going. You probably need a test for that :-).

Comment 10

[(no longer active)]

(In reply to comment #9)
> Now you've changed the behaviour so that if an HTML element has a non-HTML
> parent, isContentEditable will return false. I think in that case you're
> supposed to find the nearest HTML ancestor and keep going. You probably need a
> test for that :-).

Accidentally, I think the new behavior is the correct one!

Non-HTML content is by definition not-editable, so any possible children should not be editable as well, unless they're explicitly marked as editable.  Is this correct?

I'll add the test after we actually figure out which behavior we want to support.

Comment 11

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

The spec is quite clear here:

> Specifically, if an HTML element has a contenteditable attribute set to the
> true state, or it has its contenteditable attribute set to the inherit state
> and if its nearest ancestor HTML element with the contenteditable attribute set
> to a state other than the inherit state has its attribute set to the true
> state, ... then the UA must treat the element as editable (as described below).

Therefore, HTML content inside non-HTML content inside editable HTML content is definitely editable.

In practice, I think this is what we want too. We want all HTML and non-HTML content in a contenteditable container to be editable; we want to be able to edit SVG and MathML content, for example.

Comment 12

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

Right now the spec doesn't define editability for non-HTML content, or maybe it says non-HTML content is never editable, I'm not sure. But we definitely will want to support editing of HTML content, and anyway the treatment of non-HTML content isn't relative for this bug because that content doesn't have an isContentEditable attribute currently.

Comment 13

[(no longer active)]

Attachment: Patch (v1.3)

Addressed comment 11 and 12.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 431765 [details] [diff] [review]
Patch (v1.3)

>+    if (!node->IsHTML())
>+      continue;
>+
>+    nsGenericHTMLElement* element = FromContent(node);

You don't need the IsHTML() check.  FromContent will handle that for you and return null if !IsHTML().

r=me with that check removed.

Comment 15

[(no longer active)]

(In reply to comment #14)
> (From update of attachment 431765 [details] [diff] [review])
> >+    if (!node->IsHTML())
> >+      continue;
> >+
> >+    nsGenericHTMLElement* element = FromContent(node);
> 
> You don't need the IsHTML() check.  FromContent will handle that for you and
> return null if !IsHTML().

Yes, but I wanted to explicitly check for the case when the parent is not HTML, and continue upward.  FromContent can also return null when we're at the top level, right?  So, I don't have any way to differentiate between those two types of null return values.

Comment 16

[Boris Zbarsky [:bzbarsky]]

I don't follow.  You code currently has this structure (if you inline FromContent):

  if (!node->IsHTML())
    continue;

  if (!node->IsHTML())
    continue;

  nsGenericHTMLElement* element = static_cast<nsGenericHTMLElement*>(node);
  ContentEditableTristate value = ....

I'm just saying you can nix that first IsHTML check+continue.

What "two types" of null return values are you talking about in comment 15?

Comment 17

[(no longer active)]

Attachment: Patch (v1.4)

Oh, sorry, I confused it with the |node| null-check in the for loop.  Updated the patch with your comment addressed.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Yeah, that looks good.  :)

Comment 19

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/1e78e95009ca

Comment 20

[Eric Shepherd [:sheppy]]

Now documented:

https://developer.mozilla.org/en/DOM/Element.isContentEditable

While I was at it, realized that contentEditable wasn't documented, so now it is:

https://developer.mozilla.org/en/DOM/Element.contentEditable

Both have also been added to the property list on the main element documentation page.

Comment 21

[(no longer active)]

(In reply to comment #20)
> Now documented:
> 
> https://developer.mozilla.org/en/DOM/Element.isContentEditable
> 
> While I was at it, realized that contentEditable wasn't documented, so now it
> is:
> 
> https://developer.mozilla.org/en/DOM/Element.contentEditable
> 
> Both have also been added to the property list on the main element
> documentation page.

contentEditable might also return "inherit".  It actually determines the value of the contenteditable attribute set on the element.