Add a pref observer for accessibility.force_disabled and shutdown if it's set to 1.

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: yzen, Assigned: yzen)

Tracking

(Blocks 2 bugs, {access})

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

2 years ago
Right not accessibility service is not shutdown if the pref is set to 1. The browser needs to be restarted for the preference to take effect.

We should listen to the pref if the accessibility service is initialized and if it is set to 1, call Shutdown()
Assignee

Comment 1

2 years ago
Posted patch 1401980 patch v1 (obsolete) — Splinter Review
Attachment #8911962 - Flags: review?(surkov.alexander)
Comment on attachment 8911962 [details] [diff] [review]
1401980 patch v1

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

::: accessible/base/nsAccessibilityService.cpp
@@ +252,5 @@
>  
> +/**
> + * Cached value of the platfrom disabled preference.
> + */
> +static int32_t sPlatformDisabledState;

pls initialize it

@@ +370,5 @@
>  nsAccessibilityService::Observe(nsISupports *aSubject, const char *aTopic,
>                           const char16_t *aData)
>  {
> +  if (!strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID)) {
> +    if (!NS_strcmp(aData, u"accessibility.force_disabled")) {

it makes to assert it here
and it'd be good to have a constant variable for 'accessibility.force_disabled' string

@@ +1279,5 @@
>  
>    observerService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false);
>  
> +  // Listen to "accessibility.force_disabled"
> +  Preferences::AddWeakObserver(this, "accessibility.force_disabled");

you could assign sPlatformDisabledState variable here, and then update it on observer, in this case you wouldn't need to make it a pref cache variable, and then PlatformDisabledState() would return sPlatformDisabledState
Attachment #8911962 - Flags: review?(surkov.alexander)
Assignee

Comment 3

2 years ago
Posted patch 1401980 patch v2 (obsolete) — Splinter Review
Attachment #8911962 - Attachment is obsolete: true
Attachment #8917097 - Flags: review?(surkov.alexander)
Comment on attachment 8917097 [details] [diff] [review]
1401980 patch v2

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

::: accessible/base/nsAccessibilityService.cpp
@@ +1873,5 @@
> +  EPlatformDisabledState disabledState =
> +    (EPlatformDisabledState)sPlatformDisabledState;
> +  if (disabledState == ePlatformIsForceEnabled) {
> +    // Initialize accessibility service if necessary.
> +    GetOrCreateAccService(nsAccessibilityService::eUserPreference);

it seems you change the preference meaning here: force_enabled means we have to enable a11y ignoring all other settings, when we are asked to do so (see ShouldA11yBeEnabled), but now you attempt to start a11y whenever the pref value is changed.

so I would try to stick here with what the bug summary says: "shutdown if the pref was changed so"

@@ +1896,5 @@
> +    return;
> +  }
> +
> +  platformDisabledStateCached = true;
> +  Preferences::RegisterCallbackAndCall(PrefChanged, PREF_ACCESSIBILITY_FORCE_DISABLED);

something hard predictable may happen here: the thing can be called during a11y initialization, and if you may attempt to shutdown a11y during initialization or try to initialize it twice.

so we have to keep the first time variable initialization separate from the pref changes, it probably makes sense to have a function like ReadPlatformDisabledState or something nicer.

also, you don't need MaybeRegisterPrefCallback function, you can put its content right into PlatformDisabledState()

I sort of like the margic value of 0xff to decide whether the pref had been read or not.
Attachment #8917097 - Flags: review?(surkov.alexander)
Yura, p2 feels fair?
Priority: -- → P2
Assignee

Comment 6

2 years ago
yeah as per our discussion it's on my plate.
Assignee

Comment 7

2 years ago
Posted patch 1401980 patch v3 (obsolete) — Splinter Review
Let me know if you actually meant reading separately from adding a callback for pref. In that case we would have to make a number of changes in a number of places.
Attachment #8917097 - Attachment is obsolete: true
Attachment #8924056 - Flags: review?(surkov.alexander)
(In reply to Yura Zenevich [:yzen] from comment #7)
> Created attachment 8924056 [details] [diff] [review]
> 1401980 patch v3
> 
> Let me know if you actually meant reading separately from adding a callback
> for pref. In that case we would have to make a number of changes in a number
> of places.

yeah, that's what I meant. Reading a property shouldn't make any actions, because we read the property in order to do some action, and I'm get worried about possible conflicts, if we try to make an action during some other action.
Assignee

Comment 9

2 years ago
Posted patch 1401980 patch v4 (obsolete) — Splinter Review
Attachment #8924056 - Attachment is obsolete: true
Attachment #8924056 - Flags: review?(surkov.alexander)
Attachment #8924256 - Flags: review?(surkov.alexander)
Comment on attachment 8924256 [details] [diff] [review]
1401980 patch v4

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

we might need one more iteration, anyway cancelling review until the comments addressed

::: accessible/base/nsAccessibilityService.cpp
@@ +94,5 @@
>  using namespace mozilla::a11y;
>  using namespace mozilla::dom;
>  
> +/**
> + * Accessibility service force enable/disable preference. Default is 0.

it'd be nice to comment all supported values

@@ +254,5 @@
>    return nullptr;
>  }
>  
> +/**
> + * Cached value of the platfrom disabled preference.

Cached value of the PREF_ACCESSIBILITY_FORCE_DISABLED preference?

@@ +1874,5 @@
> +}
> +
> +void
> +PrefChanged(const char* aPref, void* aClosure)
> +{

you should update sPlatformDisabledState value, right?

::: accessible/base/nsAccessibilityService.h
@@ +351,5 @@
> +
> +/**
> + * Read and normalize PREF_ACCESSIBILITY_FORCE_DISABLED preference.
> + */
> +mozilla::a11y::EPlatformDisabledState ReadPlatformDisabledState();

I would put these under mozilla::a11y namespace:
namespace mozilla {
namespace a11y {
void PrefChanged();
EPlatformDisabledState ReadPlatformDisabledState();
}
}
or alternatively you could make those static members of nsAccessibilityService class, so would avoid of friend stuff.

::: accessible/tests/browser/browser_shutdown_pref.js
@@ +34,5 @@
> +    ok(true, "getAccesssibleFor triggers an exception as a11y service is shutdown.")
> +  }
> +  ok(!Services.appinfo.accessibilityEnabled, "Accessibility is disabled");
> +
> +  info("Reset focrce disabled preference");

focrce -> force

::: accessible/xpcom/xpcAccessibilityService.cpp
@@ +48,1 @@
>      GetOrCreateAccService(nsAccessibilityService::eXPCOM);

Shouldn't it be part of GetOrCreateAccService? I don't see any other callers of GetOrCreateAccService respecting this pref. Btw, NS_GetAccessibleService ignores this pref too [1], so your code doesn't work when the service is requested first time. Btw#2, I suppose it can be safely removed, if I don't miss anything.

[1] https://dxr.mozilla.org/mozilla-central/source/accessible/xpcom/xpcAccessibilityService.cpp#238

@@ +116,5 @@
>    }
>  
> +  nsAccessibilityService* accService = GetAccService();
> +  if (!accService) {
> +    return NS_ERROR_FAILURE;

so we create a service, which doesn't work. It'd be nicer if we failed to create an object at all, for example, throwing an exception that a11y is disabled.

@@ +274,1 @@
>    GetOrCreateAccService(nsAccessibilityService::eXPCOM);

aha, you fixed it :) which makes part of my previous comment obsolete. Do you think we need this code at all?
Attachment #8924256 - Flags: review?(surkov.alexander)
Assignee

Comment 11

2 years ago
(In reply to alexander :surkov from comment #10)
> Comment on attachment 8924256 [details] [diff] [review]
> 1401980 patch v4
> 
> Review of attachment 8924256 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +1874,5 @@
> > +}
> > +
> > +void
> > +PrefChanged(const char* aPref, void* aClosure)
> > +{
> 
> you should update sPlatformDisabledState value, right?

ReadPlatformDisabledState does that.
Assignee

Comment 12

2 years ago
(In reply to alexander :surkov from comment #10)
> Comment on attachment 8924256 [details] [diff] [review]
> 1401980 patch v4
> 
> Review of attachment 8924256 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> we might need one more iteration, anyway cancelling review until the
> comments addressed
> 
> ::: accessible/base/nsAccessibilityService.cpp
> @@ +94,5 @@
> >  using namespace mozilla::a11y;
> >  using namespace mozilla::dom;
> >  
> > +/**
> > + * Accessibility service force enable/disable preference. Default is 0.
> 
> it'd be nice to comment all supported values

done

> 
> @@ +254,5 @@
> >    return nullptr;
> >  }
> >  
> > +/**
> > + * Cached value of the platfrom disabled preference.
> 
> Cached value of the PREF_ACCESSIBILITY_FORCE_DISABLED preference?

done
> 
> ::: accessible/base/nsAccessibilityService.h
> @@ +351,5 @@
> > +
> > +/**
> > + * Read and normalize PREF_ACCESSIBILITY_FORCE_DISABLED preference.
> > + */
> > +mozilla::a11y::EPlatformDisabledState ReadPlatformDisabledState();
> 
> I would put these under mozilla::a11y namespace:
> namespace mozilla {
> namespace a11y {
> void PrefChanged();
> EPlatformDisabledState ReadPlatformDisabledState();
> }
> }
> or alternatively you could make those static members of
> nsAccessibilityService class, so would avoid of friend stuff.

done

> 
> ::: accessible/tests/browser/browser_shutdown_pref.js
> @@ +34,5 @@
> > +    ok(true, "getAccesssibleFor triggers an exception as a11y service is shutdown.")
> > +  }
> > +  ok(!Services.appinfo.accessibilityEnabled, "Accessibility is disabled");
> > +
> > +  info("Reset focrce disabled preference");
> 
> focrce -> force

done

> 
> ::: accessible/xpcom/xpcAccessibilityService.cpp
> @@ +48,1 @@
> >      GetOrCreateAccService(nsAccessibilityService::eXPCOM);
> 
> Shouldn't it be part of GetOrCreateAccService? I don't see any other callers
> of GetOrCreateAccService respecting this pref. Btw, NS_GetAccessibleService
> ignores this pref too [1], so your code doesn't work when the service is
> requested first time. Btw#2, I suppose it can be safely removed, if I don't
> miss anything.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/accessible/xpcom/
> xpcAccessibilityService.cpp#238

Wherever GetOrCreateAccService is called from outside of accessibility module, it is always done after the check of platform disabled state. However GetOrCreateAccService is also called internally AccessibleNode.cpp which can lead to unexpected results. I'm slightly weary about those.

> 
> @@ +116,5 @@
> >    }
> >  
> > +  nsAccessibilityService* accService = GetAccService();
> > +  if (!accService) {
> > +    return NS_ERROR_FAILURE;
> 
> so we create a service, which doesn't work. It'd be nicer if we failed to
> create an object at all, for example, throwing an exception that a11y is
> disabled.

Made some improvements. However there's a use case when the service is created and then the pref is set to force disabled. In that case we already have an object.

> 
> @@ +274,1 @@
> >    GetOrCreateAccService(nsAccessibilityService::eXPCOM);
> 
> aha, you fixed it :) which makes part of my previous comment obsolete. Do
> you think we need this code at all?
Assignee

Comment 13

2 years ago
Another pass, i addressed most comments except for the GetOrCreate method one (see comment above).
Attachment #8924256 - Attachment is obsolete: true
Attachment #8926428 - Flags: review?(surkov.alexander)
Comment on attachment 8926428 [details] [diff] [review]
1401980 patch v5

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

::: accessible/base/nsAccessibilityService.cpp
@@ +95,5 @@
>  using namespace mozilla::dom;
>  
> +/**
> + * Accessibility service force enable/disable preference. It can have the
> + * following values:

maybe: Supported values:

@@ +96,5 @@
>  
> +/**
> + * Accessibility service force enable/disable preference. It can have the
> + * following values:
> + *   Platform is force enabled:     -1

maybe: Platform -> accessibility

@@ +98,5 @@
> + * Accessibility service force enable/disable preference. It can have the
> + * following values:
> + *   Platform is force enabled:     -1
> + *   Platform is enabled (default):  0
> + *   Platform is force disabled:     1

it'd be also nice to comment each value shortly, i.e. Accessibility is enabled (will be started upon a request, default value)

@@ +1942,5 @@
>  PlatformDisabledState()
>  {
> +  static bool platformDisabledStateCached = false;
> +  if (platformDisabledStateCached) {
> +    return (EPlatformDisabledState)sPlatformDisabledState;

it's better to use explicit casting here, like static_cast<EPlatformDisabledState>(sPlatformDisabledState)

::: accessible/xpcom/xpcAccessibilityService.cpp
@@ +43,5 @@
>      NS_ASSERT_OWNINGTHREAD(xpcAccessibilityService);
>    nsrefcnt count = ++mRefCnt;
>    NS_LOG_ADDREF(this, count, "xpcAccessibilityService", sizeof(*this));
>  
> +  if (mRefCnt > 1 && PlatformDisabledState() != ePlatformIsDisabled) {

if you remember, could you please add a comment while you are here, why we need mRefCnt > 1 condition, i.e. why we don't want to enable accessibility in case of mRefCnt == 1
Attachment #8926428 - Flags: review?(surkov.alexander) → review+

Comment 15

2 years ago
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72f9e62daa10
shutdown accessibility service on accessibliity.force_disable pref change. r=surkov
Assignee

Updated

2 years ago
Blocks: 1416413

Comment 19

2 years ago
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcfe27603357
shutdown accessibility service on accessibliity.force_disable pref change. r=surkov

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dcfe27603357
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Blocks: 1417165
Assignee

Updated

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