733335 - dexpcom nsIEditableTextAccessible::GetAssociatedEditor

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Review of attachment 603210 [details] [diff] [review]:
-----------------------------------------------------------------

r=me looks fine. (I didn't build/test)

Comment 3

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52ffdbb7c699

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/52ffdbb7c699

Comment 5

[Trevor Saunders (:tbsaunde)]

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

>-  /**
>-   * Returns an editor associated with the accessible.
>-   */
>-  [noscript] readonly attribute nsIEditor associatedEditor;

do we need dev-doc-needed even if it  isn't scriptable?

>+  bool isEditable = false;
>   editor->GetIsDocumentEditable(&isEditable);
>-  if (isEditable) {
>-    NS_ADDREF(*aEditor = editor);
>-  }
>-  return NS_OK;
>+  if (isEditable)
>+    return editor.forget();
>+
>+  return nsnull;

return isEditable ? editor.forget() : nsnull; ?

>   // nsGenericHTMLElement::GetEditor has a security check.
>   // Make sure we're not restricted by the permissions of
>   // whatever script is currently running.
>   nsCOMPtr<nsIJSContextStack> stack =
>     do_GetService("@mozilla.org/js/xpc/ContextStack;1");
>   bool pushed = stack && NS_SUCCEEDED(stack->Push(nsnull));
> 
>   nsCOMPtr<nsIEditor> editor;
>-  nsresult rv = editableElt->GetEditor(aEditor);
>+  editableElt->GetEditor(getter_AddRefs(editor));

it seems like you could use nsGenericHTMLElement::FromContent() and then GetEditorInternal() to save going through this dance.

>     editor->GetFlags(&flags);
>     if (0 == (flags & nsIPlaintextEditor::eEditorReadonlyMask)) {

btw I seem to remember nsIEditor having an attribute just for this

>       states |= states::EDITABLE;
>     }
>   } else if (mContent->Tag() == nsGkAtoms::article) {
>     // We want <article> to behave like a document in terms of readonly state.
>@@ -705,18 +704,17 @@ nsHyperTextAccessible::HypertextOffsetsT
>   *aEndNode = nsnull;
> 
>   NS_ENSURE_ARG_POINTER(aEndOffset);
>   *aEndOffset = -1;
> 
>   // If the given offsets are 0 and associated editor is empty then return
>   // collapsed range with editor root element as range container.
>   if (aStartHTOffset == 0 && aEndHTOffset == 0) {
>-    nsCOMPtr<nsIEditor> editor;
>-    GetAssociatedEditor(getter_AddRefs(editor));
>+    nsCOMPtr<nsIEditor> editor = GetEditor();
>     if (editor) {
>       bool isEmpty = false;
>       editor->GetDocumentIsEmpty(&isEmpty);
>       if (isEmpty) {
>         nsCOMPtr<nsIDOMElement> editorRootElm;
>         editor->GetRootElement(getter_AddRefs(editorRootElm));
> 
>         nsCOMPtr<nsIDOMNode> editorRoot(do_QueryInterface(editorRootElm));
>@@ -1449,118 +1447,121 @@ NS_IMETHODIMP nsHyperTextAccessible::Set
>     return InsertText(aText, 0);
>   }
>   return NS_ERROR_FAILURE;
> }
> 
> NS_IMETHODIMP
> nsHyperTextAccessible::InsertText(const nsAString &aText, PRInt32 aPosition)
> {
>-  nsCOMPtr<nsIEditor> editor;
>-  GetAssociatedEditor(getter_AddRefs(editor));
>+  if (IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  nsCOMPtr<nsIEditor> editor = GetEditor();
> 
>   nsCOMPtr<nsIPlaintextEditor> peditor(do_QueryInterface(editor));
>   NS_ENSURE_STATE(peditor);
> 
>   nsresult rv = SetSelectionRange(aPosition, aPosition);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   return peditor->InsertText(aText);
> }
> 
> NS_IMETHODIMP
> nsHyperTextAccessible::CopyText(PRInt32 aStartPos, PRInt32 aEndPos)
> {
>-  nsCOMPtr<nsIEditor> editor;
>-  GetAssociatedEditor(getter_AddRefs(editor));
>+  if (IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  nsCOMPtr<nsIEditor> editor = GetEditor();
>   NS_ENSURE_STATE(editor);
> 
>   nsresult rv = SetSelectionRange(aStartPos, aEndPos);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   return editor->Copy();
> }
> 
> NS_IMETHODIMP
> nsHyperTextAccessible::CutText(PRInt32 aStartPos, PRInt32 aEndPos)
> {
>-  nsCOMPtr<nsIEditor> editor;
>-  GetAssociatedEditor(getter_AddRefs(editor));
>+  if (IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  nsCOMPtr<nsIEditor> editor = GetEditor();
>   NS_ENSURE_STATE(editor);
> 
>   nsresult rv = SetSelectionRange(aStartPos, aEndPos);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   return editor->Cut();
> }
> 
> NS_IMETHODIMP
> nsHyperTextAccessible::DeleteText(PRInt32 aStartPos, PRInt32 aEndPos)
> {
>-  nsCOMPtr<nsIEditor> editor;
>-  GetAssociatedEditor(getter_AddRefs(editor));
>+  if (IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  nsCOMPtr<nsIEditor> editor = GetEditor();
>   NS_ENSURE_STATE(editor);
> 
>   nsresult rv = SetSelectionRange(aStartPos, aEndPos);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   return editor->DeleteSelection(nsIEditor::eNone);
> }
> 
> NS_IMETHODIMP
> nsHyperTextAccessible::PasteText(PRInt32 aPosition)
> {
>-  nsCOMPtr<nsIEditor> editor;
>-  GetAssociatedEditor(getter_AddRefs(editor));
>+  if (IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  nsCOMPtr<nsIEditor> editor = GetEditor();
>   NS_ENSURE_STATE(editor);
> 
>   nsresult rv = SetSelectionRange(aPosition, aPosition);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   return editor->Paste(nsIClipboard::kGlobalClipboard);
> }
> 
>-NS_IMETHODIMP
>-nsHyperTextAccessible::GetAssociatedEditor(nsIEditor **aEditor)
>+already_AddRefed<nsIEditor>
>+nsHyperTextAccessible::GetEditor() const
> {
>-  NS_ENSURE_ARG_POINTER(aEditor);
>-  *aEditor = nsnull;
>-
>-  if (IsDefunct())
>-    return NS_ERROR_FAILURE;
>-
>   if (!mContent->HasFlag(NODE_IS_EDITABLE)) {
>     // If we're inside an editable container, then return that container's editor
>-    nsCOMPtr<nsIAccessible> ancestor, current = this;
>-    while (NS_SUCCEEDED(current->GetParent(getter_AddRefs(ancestor))) && ancestor) {
>-      nsRefPtr<nsHyperTextAccessible> ancestorTextAccessible;
>-      ancestor->QueryInterface(NS_GET_IID(nsHyperTextAccessible),
>-                               getter_AddRefs(ancestorTextAccessible));
>-      if (ancestorTextAccessible) {
>+    nsAccessible* ancestor = Parent();
>+    while (ancestor) {
>+      nsHyperTextAccessible* hyperText = ancestor->AsHyperText();
>+      if (hyperText) {
>         // Recursion will stop at container doc because it has its own impl
>-        // of GetAssociatedEditor()
>-        return ancestorTextAccessible->GetAssociatedEditor(aEditor);
>+        // of GetEditor()
>+        return hyperText->GetEditor();
>       }

move comment before if while your here?

>+   * Return the editor associated with the accessible.
>+   */
>+  virtual already_AddRefed<nsIEditor> GetEditor() const;

any reason not to name it Editor()?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)

> >-  /**
> >-   * Returns an editor associated with the accessible.
> >-   */
> >-  [noscript] readonly attribute nsIEditor associatedEditor;
> 
> do we need dev-doc-needed even if it  isn't scriptable?

we need, https://developer.mozilla.org/en/nsIAccessibleEditableText

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)

> >+  if (isEditable)
> >+    return editor.forget();
> >+
> >+  return nsnull;
> 
> return isEditable ? editor.forget() : nsnull; ?

that'd be nice, want me to follow up?

> >   // nsGenericHTMLElement::GetEditor has a security check.
> >   // Make sure we're not restricted by the permissions of
> >   // whatever script is currently running.
> >   nsCOMPtr<nsIJSContextStack> stack =
> >     do_GetService("@mozilla.org/js/xpc/ContextStack;1");
> >   bool pushed = stack && NS_SUCCEEDED(stack->Push(nsnull));
> > 
> >   nsCOMPtr<nsIEditor> editor;
> >-  nsresult rv = editableElt->GetEditor(aEditor);
> >+  editableElt->GetEditor(getter_AddRefs(editor));
> 
> it seems like you could use nsGenericHTMLElement::FromContent() and then
> GetEditorInternal() to save going through this dance.

I think I'm going to tweak GetEditor() implementation in nearest future so I didn't touch that this time.

> >     editor->GetFlags(&flags);
> >     if (0 == (flags & nsIPlaintextEditor::eEditorReadonlyMask)) {
> 
> btw I seem to remember nsIEditor having an attribute just for this

anyway this code is going away. and if it has then I think it's just a shortcut of this property.

> >+    while (ancestor) {
> >+      nsHyperTextAccessible* hyperText = ancestor->AsHyperText();
> >+      if (hyperText) {
> >         // Recursion will stop at container doc because it has its own impl
> >-        // of GetAssociatedEditor()
> >-        return ancestorTextAccessible->GetAssociatedEditor(aEditor);
> >+        // of GetEditor()
> >+        return hyperText->GetEditor();
> >       }
> 
> move comment before if while your here?

I can do follow up if you like. And I would love if you commented before this patch was landed :)

> >+   * Return the editor associated with the accessible.
> >+   */
> >+  virtual already_AddRefed<nsIEditor> GetEditor() const;
> 
> any reason not to name it Editor()?

well, nothing specific but few things. 1) I'm still feeling pressure from that Gecko rule, add Get() for stuffs that may return null. 2) It sounds like util method, not a property of accessible object - it's applicable to some accessibles and it doesn't return anything valid for most of them.

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> 
> > >+  if (isEditable)
> > >+    return editor.forget();
> > >+
> > >+  return nsnull;
> > 
> > return isEditable ? editor.forget() : nsnull; ?
> 
> that'd be nice, want me to follow up?

up to you.

> 
> > >   // nsGenericHTMLElement::GetEditor has a security check.
> > >   // Make sure we're not restricted by the permissions of
> > >   // whatever script is currently running.
> > >   nsCOMPtr<nsIJSContextStack> stack =
> > >     do_GetService("@mozilla.org/js/xpc/ContextStack;1");
> > >   bool pushed = stack && NS_SUCCEEDED(stack->Push(nsnull));
> > > 
> > >   nsCOMPtr<nsIEditor> editor;
> > >-  nsresult rv = editableElt->GetEditor(aEditor);
> > >+  editableElt->GetEditor(getter_AddRefs(editor));
> > 
> > it seems like you could use nsGenericHTMLElement::FromContent() and then
> > GetEditorInternal() to save going through this dance.
> 
> I think I'm going to tweak GetEditor() implementation in nearest future so I
> didn't touch that this time.

ok, no reason to for what this patch fixes just seemed like an idea woth mentioning.

> 
> > >     editor->GetFlags(&flags);
> > >     if (0 == (flags & nsIPlaintextEditor::eEditorReadonlyMask)) {
> > 
> > btw I seem to remember nsIEditor having an attribute just for this
> 
> anyway this code is going away. and if it has then I think it's just a
> shortcut of this property.

makes sense just haven't gotten to that bug yet.

> > >+    while (ancestor) {
> > >+      nsHyperTextAccessible* hyperText = ancestor->AsHyperText();
> > >+      if (hyperText) {
> > >         // Recursion will stop at container doc because it has its own impl
> > >-        // of GetAssociatedEditor()
> > >-        return ancestorTextAccessible->GetAssociatedEditor(aEditor);
> > >+        // of GetEditor()
> > >+        return hyperText->GetEditor();
> > >       }
> > 
> > move comment before if while your here?
> 
> I can do follow up if you like. And I would love if you commented before
> this patch was landed :)

I'd have loved to have had the time ;)

> > >+   * Return the editor associated with the accessible.
> > >+   */
> > >+  virtual already_AddRefed<nsIEditor> GetEditor() const;
> > 
> > any reason not to name it Editor()?
> 
> well, nothing specific but few things. 1) I'm still feeling pressure from
> that Gecko rule, add Get() for stuffs that may return null. 2) It sounds
> like util method, not a property of accessible object - it's applicable to
> some accessibles and it doesn't return anything valid for most of them.

fair enough