Closed Bug 1388006 Opened 2 years ago Closed 2 years ago

accessibility module should use TextEditor and HTMLEditor instead of nsIEditor

Categories

(Core :: Editor, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(1 file)

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 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
Attachment #8896936 - Flags: review?(surkov.alexander) → review+
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.
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/36c26078a53e
a11y module should use TextEditor and HTMLEditor instead of nsIEditor r=surkov
https://hg.mozilla.org/mozilla-central/rev/36c26078a53e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.