If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

accessibility module should use TextEditor and HTMLEditor instead of nsIEditor

RESOLVED FIXED in Firefox 57

Status

()

Core
Editor
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

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.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1623b7373c52fb6f91c6441bda938bb5f370f0b1
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0fdaa31af226479aba141d696dd10743b32e537
Comment hidden (mozreview-request)

Comment 4

a month 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 month 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.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c2f1fa98367f95e951916a935890b5f2723ae0f
Comment hidden (mozreview-request)

Comment 8

a month 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 month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/36c26078a53e
Status: ASSIGNED → RESOLVED
Last Resolved: a month 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.