Last Comment Bug 756983 - Isolate focusable and unavailable states from State()
: Isolate focusable and unavailable states from State()
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: alexander :surkov
:
Mentors:
Depends on: 761943 763162
Blocks: 732872
  Show dependency treegraph
 
Reported: 2012-05-21 01:40 PDT by alexander :surkov
Modified: 2012-06-09 01:18 PDT (History)
2 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.88 KB, patch)
2012-05-21 01:40 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch (64.34 KB, patch)
2012-05-30 11:11 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2012-05-21 01:40:55 PDT
Created attachment 625579 [details] [diff] [review]
patch

not bottleneck, just doing weird things (about .5% perf lost)
Comment 1 alexander :surkov 2012-05-30 11:11:12 PDT
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.
Comment 2 Trevor Saunders (:tbsaunde) 2012-05-31 18:24:12 PDT
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?
Comment 3 alexander :surkov 2012-05-31 20:12:34 PDT
(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?
Comment 4 Trevor Saunders (:tbsaunde) 2012-05-31 21:12:38 PDT
(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
Comment 5 alexander :surkov 2012-05-31 21:17:30 PDT
(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 :)
Comment 6 Trevor Saunders (:tbsaunde) 2012-05-31 21:31:58 PDT
> > > > >-  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.
Comment 8 alexander :surkov 2012-06-03 23:14:48 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.