660742 - accessible should use mozilla::Preferences

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 536224 [details] [diff] [review]
Patch v1.0

Review of attachment 536224 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 536224 [details] [diff] [review]
Patch v1.0

I'm _really_ not the right person for this review.  Perhaps David Bolter would be a better choice (or would re-direct it to someone more useful).

Thanks,
Blake.

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 536224 [details] [diff] [review]
Patch v1.0

r=me. nice. thanks.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/f81b4d9534f5

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 536224 [details] [diff] [review]
Patch v1.0

>diff --git a/accessible/src/base/nsAccessible.cpp b/accessible/src/base/nsAccessible.cpp
>--- a/accessible/src/base/nsAccessible.cpp
>+++ b/accessible/src/base/nsAccessible.cpp
>   // use ui.key.generalAccessKey (unless it is -1)
>-  PRInt32 accessKey;
>-  nsresult rv = prefBranch->GetIntPref("ui.key.generalAccessKey", &accessKey);
>+  PRInt32 accessKey = -1;
>+  nsresult rv = Preferences::GetInt("ui.key.generalAccessKey", accessKey);

This is wrong, isn't it? You probably wanted to pass &accessKey as that last parameter.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #5)
> Comment on attachment 536224 [details] [diff] [review] [review]
> Patch v1.0
> 
> >diff --git a/accessible/src/base/nsAccessible.cpp b/accessible/src/base/nsAccessible.cpp
> >--- a/accessible/src/base/nsAccessible.cpp
> >+++ b/accessible/src/base/nsAccessible.cpp
> >   // use ui.key.generalAccessKey (unless it is -1)
> >-  PRInt32 accessKey;
> >-  nsresult rv = prefBranch->GetIntPref("ui.key.generalAccessKey", &accessKey);
> >+  PRInt32 accessKey = -1;
> >+  nsresult rv = Preferences::GetInt("ui.key.generalAccessKey", accessKey);
> 
> This is wrong, isn't it? You probably wanted to pass &accessKey as that last
> parameter.

Wow, exactly. I'll back it out.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

backed out.
http://hg.mozilla.org/mozilla-central/rev/d011353d1719

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.1

>  GetAccessModifierMask(nsIContent* aContent)
>  {
> -  nsCOMPtr<nsIPrefBranch> prefBranch =
> -    do_GetService(NS_PREFSERVICE_CONTRACTID);
> -  if (!prefBranch)
> -    return 0;
> -
>    // use ui.key.generalAccessKey (unless it is -1)
> -  PRInt32 accessKey;
> -  nsresult rv = prefBranch->GetIntPref("ui.key.generalAccessKey", &accessKey);
> -  if (NS_SUCCEEDED(rv) && accessKey != -1) {
> -    switch (accessKey) {
> -      case nsIDOMKeyEvent::DOM_VK_SHIFT:   return NS_MODIFIER_SHIFT;
> -      case nsIDOMKeyEvent::DOM_VK_CONTROL: return NS_MODIFIER_CONTROL;
> -      case nsIDOMKeyEvent::DOM_VK_ALT:     return NS_MODIFIER_ALT;
> -      case nsIDOMKeyEvent::DOM_VK_META:    return NS_MODIFIER_META;
> -      default:                             return 0;
> -    }
> +  switch (Preferences::GetInt("ui.key.generalAccessKey", -1)) {
> +    case -1:                             break;
> +    case nsIDOMKeyEvent::DOM_VK_SHIFT:   return NS_MODIFIER_SHIFT;
> +    case nsIDOMKeyEvent::DOM_VK_CONTROL: return NS_MODIFIER_CONTROL;
> +    case nsIDOMKeyEvent::DOM_VK_ALT:     return NS_MODIFIER_ALT;
> +    case nsIDOMKeyEvent::DOM_VK_META:    return NS_MODIFIER_META;
> +    default:                             return 0;
>    }

Maybe, I broke when I changed here... Sorry for the mistake.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.2

Oops, No. This is correct.

>   // use ui.key.generalAccessKey (unless it is -1)
>   switch (Preferences::GetInt("ui.key.generalAccessKey", -1)) {
>     case -1:                             break;
>     case nsIDOMKeyEvent::DOM_VK_SHIFT:   return NS_MODIFIER_SHIFT;
>     case nsIDOMKeyEvent::DOM_VK_CONTROL: return NS_MODIFIER_CONTROL;
>     case nsIDOMKeyEvent::DOM_VK_ALT:     return NS_MODIFIER_ALT;
>     case nsIDOMKeyEvent::DOM_VK_META:    return NS_MODIFIER_META;
>     default:                             return 0;
>   }
> 
>   // get the docShell to this DOMNode, return 0 on failure
>   nsCOMPtr<nsIDocument> document = aContent->GetCurrentDoc();
>   if (!document)
>     return 0;
>   nsCOMPtr<nsISupports> container = document->GetContainer();
>   if (!container)
>     return 0;
>   nsCOMPtr<nsIDocShellTreeItem> treeItem(do_QueryInterface(container));
>   if (!treeItem)
>     return 0;
> 
>   // determine the access modifier used in this context
>   nsresult rv = NS_ERROR_FAILURE;

initial value of the rv must be error here.

I hate carried over rv!

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Note: please always get a clean try server run before landing, and/or explicitly ask me to build and test when requesting review.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #10)
> Note: please always get a clean try server run before landing, and/or
> explicitly ask me to build and test when requesting review.

I have question, how do I test the GetAccessModifierMask() manually? The mistake wasn't detected by automated tests.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, David. I succeeded to test the patch by your advice on IRC.

But I couldn't test nsAccessNodeWrap::TurnOffNewTabSwitchingForJawsAndWE() because I couldn't find a way to call it. But the change in it is very simple. And I confirmed the new APIs are work fine. So, I think they are okay.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI: I'm also testing on tryserver again.
http://tbpl.mozilla.org/?tree=Try&rev=5021b7cfeea1

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 536878 [details] [diff] [review]
Patch v1.2

Review of attachment 536878 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 536878 [details] [diff] [review]
Patch v1.2

r=me if the try run looks good. thanks.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/5b0986336b74

Comment 17

[Simona Badau, Desktop QA]

Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0

Is there any way I can verify this on the QA side? I visually inspected the code changes in the repo:
http://hg.mozilla.org/mozilla-central/file/922f27baed98

Is that enough to mark this as Verified Fixed?

Comment 18

[alexander :surkov (:asurkov)]

(In reply to Simona B [QA] from comment #17)
> Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0
> 
> Is there any way I can verify this on the QA side?

No.

> I visually inspected the
> code changes in the repo:
> http://hg.mozilla.org/mozilla-central/file/922f27baed98
> 
> Is that enough to mark this as Verified Fixed?

yes

Simona, thank you for looking at a11y bugs but curious whether QA needs to run through all fixed bugs for some release and verify them, especially those that are just code cleanings-up?

Comment 19

[alexander :surkov (:asurkov)]

(In reply to alexander surkov from comment #18)

> Simona, thank you for looking at a11y bugs but curious whether QA needs to
> run through all fixed bugs for some release and verify them, especially
> those that are just code cleanings-up?

I'm asking because if general practice is bugs verification (which is great of course) but obviously you shouldn't spend your time for some of them then maybe we have (or should have) whiteboard status (like needs-verification) to point this out to keep your work easier?

Comment 20

[u279076]

(In reply to alexander surkov from comment #18)
> (In reply to Simona B [QA] from comment #17)
> > Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0
> > 
> > Is there any way I can verify this on the QA side?
> 
> No.
> 
> > I visually inspected the
> > code changes in the repo:
> > http://hg.mozilla.org/mozilla-central/file/922f27baed98
> > 
> > Is that enough to mark this as Verified Fixed?
> 
> yes
> 
> Simona, thank you for looking at a11y bugs but curious whether QA needs to
> run through all fixed bugs for some release and verify them, especially
> those that are just code cleanings-up?

@Alex, it has been communicated to the QA teams to skip verifying fixes which are obvious code-level bugs and to only focus on broken/improved features/functional bugs. Adding a whiteboard tag would certainly make filtering easier but "most" devs are resistant to use yet another whiteboard tag. The solution here is to just educate ourselves about what NEEDS to get verified and what doesn't.

Thanks.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #20)

> @Alex, it has been communicated to the QA teams to skip verifying fixes
> which are obvious code-level bugs and to only focus on broken/improved
> features/functional bugs. Adding a whiteboard tag would certainly make
> filtering easier but "most" devs are resistant to use yet another whiteboard
> tag. The solution here is to just educate ourselves about what NEEDS to get
> verified and what doesn't.

Ok, fair enough. Either way has benefits and disadvantages. Thank you.