1388006 - accessibility module should use TextEditor and HTMLEditor instead of nsIEditor

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Currently, accessibility module uses nsIEditor because:

1. HTMLTextFieldAccessible::GetEditor() uses nsIDOMNSEditableElement and it returns nsIEditor.
2. DocAccessible and HyperTextAccessible uses nsIEditingSession. However, it returns nsIEditor due to bug1388001.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1623b7373c52fb6f91c6441bda938bb5f370f0b1

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0fdaa31af226479aba141d696dd10743b32e537

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1388006 - a11y module should use TextEditor and HTMLEditor instead of nsIEditor

Accessibility module uses nsIEditor (and nsIPlaintextEditor).  However, now, it can use TextEditor and HTMLEditor.  So, it should use them.

Note that this patch makes HTMLTextFieldAccessible::GetEditor() use nsITextControlElement::GetTextEditor() instead of nsIDOMNSEditableElement::GetEditor() but this won't change actual behavior since both implementation of HTMLInputElement and HTMLTextareaElement are just call shared internal methods.

Review commit: https://reviewboard.mozilla.org/r/168226/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168226/

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 8896936 [details]
Bug 1388006 - a11y module should use TextEditor and HTMLEditor instead of nsIEditor

https://reviewboard.mozilla.org/r/168226/#review173448

::: accessible/generic/DocAccessible.cpp:362
(Diff revision 1)
>    docShell->GetEditingSession(getter_AddRefs(editingSession));
>    if (!editingSession)
>      return nullptr; // No editing session interface
>  
> -  nsCOMPtr<nsIEditor> editor;
> -  editingSession->GetEditorForWindow(mDocumentNode->GetWindow(), getter_AddRefs(editor));
> +  RefPtr<HTMLEditor> htmlEditor =
> +    editingSession->GetHTMLEditorForWindow(mDocumentNode->GetWindow());

It appears you're somewhere ahead of trunk. I'm curious if this thing can return a raw pointer. If it did then we could make DocAccessible/HyperTextAccessible::GetEditor() method to return raw pointers too. It doesn't have to be part of this bug though.

::: accessible/generic/HyperTextAccessible.cpp:396
(Diff revision 1)
> -    nsCOMPtr<nsIEditor> editor = GetEditor();
> -    if (editor) {
> +    RefPtr<TextEditor> textEditor = GetEditor();
> +    if (textEditor) {
>        bool isEmpty = false;
> -      editor->GetDocumentIsEmpty(&isEmpty);
> +      textEditor->GetDocumentIsEmpty(&isEmpty);
>        if (isEmpty) {
> -        nsCOMPtr<nsIDOMElement> editorRootElm;
> +        nsCOMPtr<nsINode> editorRoot = textEditor->GetRoot();

can you use non-refcounting version of GetRoot (https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/EditorBase.h#930)

::: accessible/generic/HyperTextAccessible.cpp:1558
(Diff revision 1)
>  
>    dom::Selection* domSel = frameSelection->GetSelection(aSelectionType);
>    if (!domSel)
>      return;
>  
>    nsCOMPtr<nsINode> startNode = GetNode();

it appears, you can use a raw pointer nsINode* here

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8896936 [details]
Bug 1388006 - a11y module should use TextEditor and HTMLEditor instead of nsIEditor

https://reviewboard.mozilla.org/r/168226/#review173448

> It appears you're somewhere ahead of trunk. I'm curious if this thing can return a raw pointer. If it did then we could make DocAccessible/HyperTextAccessible::GetEditor() method to return raw pointers too. It doesn't have to be part of this bug though.

Fortunately, GetHTMLEditorForWindow() returns raw pointer. I'll file a follow up bug.

> can you use non-refcounting version of GetRoot (https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/EditorBase.h#930)

Sure, it's safe here.

> it appears, you can use a raw pointer nsINode* here

Sure.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c2f1fa98367f95e951916a935890b5f2723ae0f

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8896936 [details]
Bug 1388006 - a11y module should use TextEditor and HTMLEditor instead of nsIEditor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168226/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36c26078a53e
a11y module should use TextEditor and HTMLEditor instead of nsIEditor r=surkov

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/36c26078a53e