Remove Accessible::DOMNode

RESOLVED FIXED in Firefox 61

Status

()

P5
enhancement
RESOLVED FIXED
6 years ago
9 months ago

People

(Reporter: ayg, Assigned: dora.tokio, Mentored)

Tracking

(Blocks: 1 bug, {good-first-bug})

Trunk
mozilla61
good-first-bug
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

iirc we wanted to replace it on raw pointer version (I guess via nsINode::AsDOMNode())
I think there's only two or so uses, so it's probably easier to deal in the callers.
Will we still have the ability to reference the original DOM node from the accessible? I believe some of the JSAT folder uses, or new patches plan to use, this to gain access to the original node coming from the accessible object.
GetNode().
jsat is xpcom based, this bug is about internal method
(In reply to :Ms2ger from comment #2)
> I think there's only two or so uses, so it's probably easier to deal in the
> callers.

I don't mind
Mentor: surkov.alexander
Keywords: good-first-bug
Priority: -- → P5

Comment 7

9 months ago
Can anyone please explain this bug. I would like to work on it.
(In reply to Videet Singhai from comment #7)
> Can anyone please explain this bug. I would like to work on it.

technically, you should replace Accessible:DOMNode() instances on more appropriate calls like GetContent or GetNode().

https://searchfox.org/mozilla-central/source/accessible/generic/Accessible.h#169
(Assignee)

Comment 9

9 months ago
Is nobody assigned to this bug yet?

Comment 10

9 months ago
(In reply to dora.tokio from comment #9)
> Is nobody assigned to this bug yet?

yup,nobady mean that no body is assigned
yep, feel free to claim it if you are keen to work on it
(Assignee)

Comment 12

9 months ago
Thanks, I'd like to work on it.

Accessible::DOMNode() is called only twice, in a similar situation. So, I rewrite XULTabAccessible.cpp as follows, is this correct?

Before:
>  nsCOMPtr<nsIDOMNode> domNode(DOMNode());
>  nsCOMPtr<nsIDOMNode> tabpanelNode;
>  tabsElm->GetRelatedElement(domNode, getter_AddRefs(tabpanelNode));

After:
>  nsCOMPtr<nsIDOMNode> domNode(do_QueryInterface(GetNode()));
>  nsCOMPtr<nsIDOMNode> tabpanelNode;
>  tabsElm->GetRelatedElement(domNode, getter_AddRefs(tabpanelNode));
yep, sounds good
Assignee: nobody → dora.tokio
(Assignee)

Comment 14

9 months ago
Created attachment 8961301 [details] [diff] [review]
Remove Accessible::DOMNode

replace Accessible::DOMNode with Accessible::GetNode version

MozReview-Commit-ID: 9p74ISHnWzF
(Assignee)

Comment 15

9 months ago
I'm sorry but some extra file was included in the patch. I'll try it again.
(Assignee)

Comment 16

9 months ago
Created attachment 8961304 [details] [diff] [review]
Remove Accessible::DOMNode

replace Accessible::DOMNode with Accessible::GetNode version
(Assignee)

Comment 17

9 months ago
Oh, I'm so sorry again, an extra character 'S' was included.
(Assignee)

Comment 18

9 months ago
Created attachment 8961444 [details] [diff] [review]
Remove Accessible::DOMNode

replace Accessible::DOMNode with Accessible::GetNode version
Attachment #8961304 - Attachment is obsolete: true
Attachment #8961301 - Attachment is obsolete: true
Keywords: checkin-needed

Comment 19

9 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5a9d5bca98b
Remove Accessible::DOMNode. r=surkov
Keywords: checkin-needed

Comment 20

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a5a9d5bca98b
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.