Isolate focusable and unavailable states from State()

RESOLVED FIXED in mozilla15

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla15
access
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

64.34 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Created attachment 625579 [details] [diff] [review]
patch

not bottleneck, just doing weird things (about .5% perf lost)
Attachment #625579 - Flags: review?(trev.saunders)
(Assignee)

Updated

5 years ago
Attachment #625579 - Attachment is obsolete: true
Attachment #625579 - Flags: review?(trev.saunders)
(Assignee)

Comment 1

5 years ago
Created attachment 628393 [details] [diff] [review]
patch

not super nice but might be a good step on states decomposition

InteractiteState is introduced which is unavailable, focusable and selectable states for now. selectable state dependency on unavailable is not consistent through the code but in general it seems we don't expose selectable if the accessible is disabled. I added more consistence on this way we had. However taking into account editable state story then we might want later to exclude this selectable state dependency.
Attachment #628393 - Flags: review?(trev.saunders)
Comment on attachment 628393 [details] [diff] [review]
patch

>+Accessible::IsNativeUnavailable() const

name seems kind of funny, my first thought is "is there's no native people around???" ;)

maybe nativelyUnAvailable() or just NativelyAvailable() return true or false in the way that makes sense for each.

>+   * Return native interactice state (unavailable, focusable or selectable).
>+   */
>+  virtual PRUint64 NativeInteractiveState() const;
>+
>+  /**
>    * Return native link states present on the accessible.
>    */
>   virtual PRUint64 NativeLinkState() const;

do you want to keep the relative order of these consistant? it doesn't look like you do that now.

>+  state |= states::FOCUSABLE; // keep in sync with NativeIteractiveState() impl

We should probably be able to replace this with NativeInteractiveState() call once we can trust compilers to devirtualize well and use final, or lto.

>+nsXFormsAccessible::IsNativeUnavailable() const
>+{
>+  nsCOMPtr<nsIDOMNode> DOMNode(do_QueryInterface(mContent));
>+
>+  bool isRelevant = false;
>+  nsresult rv = sXFormsService->IsRelevant(DOMNode, &isRelevant);
>+  NS_ENSURE_SUCCESS(rv, 0);

returning 0 as a bool is kind of funny, and what happens when it doesn't fail?

>+  if (IsNativeUnavailable()) {
>+    // Note: keep in sinc with nsXULPopupManager::IsValidMenuItem() logic.
>+    bool skipNavigatingDisabledMenuItem = true;
>+    nsMenuFrame* menuFrame = do_QueryFrame(GetFrame());
>+    if (!menuFrame->IsOnMenuBar()) {

is the reason you can't use the nsXULPopupManager method directly that you don't know if its on a menubar or popup?

>+      skipNavigatingDisabledMenuItem = LookAndFeel::
>+        GetInt(LookAndFeel::eIntID_SkipNavigatingDisabledMenuItem, 0) != 0;
>     }
>+
>+    if (skipNavigatingDisabledMenuItem)
>+      return states::UNAVAILABLE;
>+
>+    return states::UNAVAILABLE | states::FOCUSABLE | states::SELECTABLE;

you return unavailable always?

>-  nsCOMPtr<nsIContent> mSliderNode;
>+  mutable nsCOMPtr<nsIContent> mSliderNode;

cute, lets hope compilers choose reasonable behavior for this case.

> nsXULTreeItemAccessibleBase::NativeState()
> {
>   if (!mTreeView)
>     return states::DEFUNCT;

btw, shouldn't this go away? while having a tree with no view makes sense it doesn't really make sense that tree has rows or cells does it?

>--- a/accessible/tests/mochitest/states/test_controls.xul
>+++ b/accessible/tests/mochitest/states/test_controls.xul
>@@ -10,31 +10,79 @@

add links to tests?
(Assignee)

Comment 3

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> Comment on attachment 628393 [details] [diff] [review]
> patch
> 
> >+Accessible::IsNativeUnavailable() const
> 
> name seems kind of funny, my first thought is "is there's no native people
> around???" ;)
> 
> maybe nativelyUnAvailable() or just NativelyAvailable() return true or false
> in the way that makes sense for each.

I think I'm fine with NativelyUnavailable()

> do you want to keep the relative order of these consistant? it doesn't look
> like you do that now.

I think I kept them in alphabetical order. Do you like to keep nativeUnavailabe and nativeInteractive states together?

> >+  state |= states::FOCUSABLE; // keep in sync with NativeIteractiveState() impl
> 
> We should probably be able to replace this with NativeInteractiveState()
> call once we can trust compilers to devirtualize well and use final, or lto.

can you look at this please?

> >+nsXFormsAccessible::IsNativeUnavailable() const
> >+{
> 
> returning 0 as a bool is kind of funny, and what happens when it doesn't
> fail?

yep, funny :)

> >+  if (IsNativeUnavailable()) {
> >+    // Note: keep in sinc with nsXULPopupManager::IsValidMenuItem() logic.
> >+    bool skipNavigatingDisabledMenuItem = true;
> >+    nsMenuFrame* menuFrame = do_QueryFrame(GetFrame());
> >+    if (!menuFrame->IsOnMenuBar()) {
> 
> is the reason you can't use the nsXULPopupManager method directly that you
> don't know if its on a menubar or popup?

I didn't see nothing ready for us but I agree we should have code shared (I'll file a bug on this?)

> >+      skipNavigatingDisabledMenuItem = LookAndFeel::
> >+        GetInt(LookAndFeel::eIntID_SkipNavigatingDisabledMenuItem, 0) != 0;
> >     }
> >+
> >+    if (skipNavigatingDisabledMenuItem)
> >+      return states::UNAVAILABLE;
> >+
> >+    return states::UNAVAILABLE | states::FOCUSABLE | states::SELECTABLE;
> 
> you return unavailable always?

yes when it's unavailable :) this code is under IsNativeUnavailable() condition

> >-  nsCOMPtr<nsIContent> mSliderNode;
> >+  mutable nsCOMPtr<nsIContent> mSliderNode;
> 
> cute, lets hope compilers choose reasonable behavior for this case.

do you know something about backgrounds?

> > nsXULTreeItemAccessibleBase::NativeState()
> > {
> >   if (!mTreeView)
> >     return states::DEFUNCT;
> 
> btw, shouldn't this go away? while having a tree with no view makes sense it
> doesn't really make sense that tree has rows or cells does it?

yes technically IsDefunct should work here. I'll file a bug to be on safe side?

> add links to tests?

HTML:a?
(In reply to alexander :surkov from comment #3)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > Comment on attachment 628393 [details] [diff] [review]
> > patch
> > 
> > >+Accessible::IsNativeUnavailable() const
> > 
> > name seems kind of funny, my first thought is "is there's no native people
> > around???" ;)
> > 
> > maybe nativelyUnAvailable() or just NativelyAvailable() return true or false
> > in the way that makes sense for each.
> 
> I think I'm fine with NativelyUnavailable()

ok

> > do you want to keep the relative order of these consistant? it doesn't look
> > like you do that now.
> 
> I think I kept them in alphabetical order. Do you like to keep
> nativeUnavailabe and nativeInteractive states together?

doesn't really matter to me.

> > >+  state |= states::FOCUSABLE; // keep in sync with NativeIteractiveState() impl
> > 
> > We should probably be able to replace this with NativeInteractiveState()
> > call once we can trust compilers to devirtualize well and use final, or lto.
> 
> can you look at this please?

not sure what you want me to do, I'm pretty sure we  can't do that now, since we only do lto on windows, and I'm sure the mac gcc doesn't support devirtualization ever, or support final.

> > >+nsXFormsAccessible::IsNativeUnavailable() const
> > >+{
> > 
> > returning 0 as a bool is kind of funny, and what happens when it doesn't
> > fail?
> 
> yep, funny :)

I assume you'll fix it?

> > >+  if (IsNativeUnavailable()) {
> > >+    // Note: keep in sinc with nsXULPopupManager::IsValidMenuItem() logic.
> > >+    bool skipNavigatingDisabledMenuItem = true;
> > >+    nsMenuFrame* menuFrame = do_QueryFrame(GetFrame());
> > >+    if (!menuFrame->IsOnMenuBar()) {
> > 
> > is the reason you can't use the nsXULPopupManager method directly that you
> > don't know if its on a menubar or popup?
> 
> I didn't see nothing ready for us but I agree we should have code shared
> (I'll file a bug on this?)

sure

> > >-  nsCOMPtr<nsIContent> mSliderNode;
> > >+  mutable nsCOMPtr<nsIContent> mSliderNode;
> > 
> > cute, lets hope compilers choose reasonable behavior for this case.
> 
> do you know something about backgrounds?

not sure I follow

> > > nsXULTreeItemAccessibleBase::NativeState()
> > > {
> > >   if (!mTreeView)
> > >     return states::DEFUNCT;
> > 
> > btw, shouldn't this go away? while having a tree with no view makes sense it
> > doesn't really make sense that tree has rows or cells does it?
> 
> yes technically IsDefunct should work here. I'll file a bug to be on safe
> side?

sure

> > add links to tests?
> 
> HTML:a?

I meant links to the bug
(Assignee)

Comment 5

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #4)

> > can you look at this please?
> 
> not sure what you want me to do, I'm pretty sure we  can't do that now,
> since we only do lto on windows, and I'm sure the mac gcc doesn't support
> devirtualization ever, or support final.

got it

> > > returning 0 as a bool is kind of funny, and what happens when it doesn't
> > > fail?
> > 
> > yep, funny :)
> 
> I assume you'll fix it?

sure :)

> > > >-  nsCOMPtr<nsIContent> mSliderNode;
> > > >+  mutable nsCOMPtr<nsIContent> mSliderNode;
> > > 
> > > cute, lets hope compilers choose reasonable behavior for this case.
> > 
> > do you know something about backgrounds?
> 
> not sure I follow

I meant reasonable vs not reasonable behavior if you know what bad compiler can do.

> > > add links to tests?
> > 
> > HTML:a?
> 
> I meant links to the bug

ok :)
> > > > >-  nsCOMPtr<nsIContent> mSliderNode;
> > > > >+  mutable nsCOMPtr<nsIContent> mSliderNode;
> > > > 
> > > > cute, lets hope compilers choose reasonable behavior for this case.
> > > 
> > > do you know something about backgrounds?
> > 
> > not sure I follow
> 
> I meant reasonable vs not reasonable behavior if you know what bad compiler
> can do.

well, my quick read of the spec is that ever making use of mutable is undefined behavior.  However I think we're fine here, especially since all this code is single threaded.  I suppose we could get really unlucky and compute the slider node a second time, but that's it.
Attachment #628393 - Flags: review?(trev.saunders) → review+
(Assignee)

Updated

5 years ago
Summary: make nsAccessible::GetActionName faster → Isolate focusable and unavailable states from State()
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/af83ecf3ee15
Flags: in-testsuite+
Target Milestone: --- → mozilla15
(Assignee)

Comment 8

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #4)

> > > >+  if (IsNativeUnavailable()) {
> > > >+    // Note: keep in sinc with nsXULPopupManager::IsValidMenuItem() logic.
> > > >+    bool skipNavigatingDisabledMenuItem = true;
> > > >+    nsMenuFrame* menuFrame = do_QueryFrame(GetFrame());
> > > >+    if (!menuFrame->IsOnMenuBar()) {
> > > 
> > > is the reason you can't use the nsXULPopupManager method directly that you
> > > don't know if its on a menubar or popup?
> > 
> > I didn't see nothing ready for us but I agree we should have code shared
> > (I'll file a bug on this?)
> 
> sure

bug 761062

> > > > nsXULTreeItemAccessibleBase::NativeState()
> > > > {
> > > >   if (!mTreeView)
> > > >     return states::DEFUNCT;
> > > 
> > > btw, shouldn't this go away? while having a tree with no view makes sense it
> > > doesn't really make sense that tree has rows or cells does it?
> > 
> > yes technically IsDefunct should work here. I'll file a bug to be on safe
> > side?
> 
> sure

bug 761064
https://hg.mozilla.org/mozilla-central/rev/af83ecf3ee15
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 761943

Updated

5 years ago
Depends on: 763162
You need to log in before you can comment on or make changes to this bug.