The default bug view has changed. See this FAQ.

accessible should use mozilla::Preferences

RESOLVED FIXED in mozilla7

Status

()

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

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

Trunk
mozilla7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 536224 [details] [diff] [review]
Patch v1.0
Attachment #536224 - Flags: review?(roc)
Comment on attachment 536224 [details] [diff] [review]
Patch v1.0

Review of attachment 536224 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #536224 - Flags: review?(roc) → review+
(Assignee)

Updated

6 years ago
Attachment #536224 - Flags: review?(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.
Attachment #536224 - Flags: review?(bwinton) → review?(bolterbugz)
Comment on attachment 536224 [details] [diff] [review]
Patch v1.0

r=me. nice. thanks.
Attachment #536224 - Flags: review?(bolterbugz) → review+
(Assignee)

Comment 4

6 years ago
http://hg.mozilla.org/mozilla-central/rev/f81b4d9534f5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
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.
(Assignee)

Comment 6

6 years ago
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 7

6 years ago
backed out.
http://hg.mozilla.org/mozilla-central/rev/d011353d1719
(Assignee)

Comment 8

6 years ago
Created attachment 536877 [details] [diff] [review]
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.
Attachment #536224 - Attachment is obsolete: true
Attachment #536877 - Flags: review?(roc)
(Assignee)

Comment 9

6 years ago
Created attachment 536878 [details] [diff] [review]
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!
Attachment #536877 - Attachment is obsolete: true
Attachment #536877 - Flags: review?(roc)
Attachment #536878 - Flags: review?(roc)
Note: please always get a clean try server run before landing, and/or explicitly ask me to build and test when requesting review.
(Assignee)

Comment 11

6 years ago
(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.
(Assignee)

Comment 12

6 years ago
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.
(Assignee)

Comment 13

6 years ago
FYI: I'm also testing on tryserver again.
http://tbpl.mozilla.org/?tree=Try&rev=5021b7cfeea1
Comment on attachment 536878 [details] [diff] [review]
Patch v1.2

Review of attachment 536878 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #536878 - Flags: review?(roc) → review+
(Assignee)

Updated

6 years ago
Attachment #536878 - Flags: review?(bolterbugz)
Comment on attachment 536878 [details] [diff] [review]
Patch v1.2

r=me if the try run looks good. thanks.
Attachment #536878 - Flags: review?(bolterbugz) → review+
(Assignee)

Comment 16

6 years ago
http://hg.mozilla.org/mozilla-central/rev/5b0986336b74
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
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

6 years ago
(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

6 years ago
(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?
(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

6 years ago
(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.
You need to log in before you can comment on or make changes to this bug.