accessibility module should use TextEditor and HTMLEditor instead of nsIEditor

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

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 hidden (mozreview-request)

Comment 4

a year ago
mozreview-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

::: 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+
(Assignee)

Comment 5

a year ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 8

a year ago
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

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/36c26078a53e
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.