1486971 - 'Reduce Motion' should reflect 'prefers-reduced-motion' without restarting firefox process

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Hiroyuki Ikezoe (:hiro)]

As Jonathan mentioned in bug 1475462 comment 4, it seems that reloading content doesn't refresh prefers-reduced-motion media query.

I thought the patch in bug 1475462 comment 6 fixes the issue, but it doesn't actually (thanks arai for checking it).

There must be something different from other platforms.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Notify accessibilityDisplayShouldReduceMotion changes

FWIW, this is the patch I mentioned in comment 0.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I will check this once I get a Macbook Brian will send to me.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

This is an odd bug.  It seems that the property value, accessibilityDisplayShouldReduceMotion, has never changed since each process is created.   Other similar properties, accessibilityDisplayShouldReduceTransparency, accessibilityDisplayShouldDifferentiateWithoutColor, accessibilityDisplayShouldIncreaseContrast are not changed either.  Whereas, accessibilityDisplayShouldInvertColors and voiceOverEnabled are changed properly.  

Another odd thing is that NSSystemColorsDidChangeNotification [1] receives NSWorkspaceAccessibilityDisplayOptionsDidChangeNotification too.

I have no idea how Safari handles these oddnesses, I can't see any special handlings for accessibilityDisplayShouldReduceMotion in WebKit.  We are doing something special when we create a process on MacOSX?

[1] https://searchfox.org/mozilla-central/rev/43969de34f5d4b113133d090f024e5eed7a82af0/widget/cocoa/nsChildView.mm#3335

Comment 4

[Hiroyuki Ikezoe (:hiro)]

This is somewhat related to how we create processes on MacOSX.  The problem doesn't happen if there is no other firefox instances (releases, nightly, local builds, etc.).  For example, if there is a firefox 61 instance and a local build instance with non-E10S mode, the problem happens, but once the firefox 61 instance finished, the problem disappears.  This look pretty odd, how the firefox 61 instance affected local build?  CCing, Markus and spohl, they might know something.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

The property value, accessibilityDisplayShouldReduceMotion is properly updated in the parent processes, but not in child processes.  I can see the same symptoms for isSwipeTrackingFromScrollEventsEnabled as well, but it doesn't matter since isSwipeTrackingFromScrollEventsEnabled is only used in the parent process (in browser.xul).

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) (PTO on Aug 31) from comment #3)
> Whereas,
> accessibilityDisplayShouldInvertColors and voiceOverEnabled are changed
> properly.  

I was wrong. accessibilityDisplayShouldInvertColors isn't changed at all.  So voiceOverEnabled is the only one whose value is correctly changed in the child process.

And I did create a simple console application which queries accessibilityDisplayShouldReduceMotion, the value isn't changed in this simple application. 

So my best guess is that those values are not changed in console applications, but changed in GUI applications.  (which means our parent processes are GUI application, whereas child processes are console applications?).

Comment 7

[Hiroyuki Ikezoe (:hiro)]

I found setting dom.ipc.useNativeEventProcessing.content false solves this issue.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) (PTO on Aug 31) from comment #7)
> I found setting dom.ipc.useNativeEventProcessing.content false solves this
> issue.

Setting the pref *true*.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

I could write a fix for this.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e84e63baccbfbfb1ceb93dd8efc3b9c63169021e

Comment 11

[Hiroyuki Ikezoe (:hiro)]

I did successfully write mochitest for this.  The mochitest needs the fix for bug 1478212.

Let's see it on try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bda4f252e5f797f68ea2a10911d876902576302

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Hmm, the test works fine on MacOS10.13.6, but crashed on the try server because the server is MacOS10.10.

Using sharedWorkspace.notificationCenter to post the notification seems to be a problem, so I did use NSNotificationCenter.defaultCenter instead.  It works fine!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=407f77f0062fb3cc1d21e7cbf75c3f1b4485e737

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1486971 - Receive accessibilityDisplayShouldReduceMotion change and notify it. r=mstange

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1486971 - Query NSWorkspace.accessibilityDisplayShouldReduceMotion only if it's on parent processes or it's the initial query on child processes. r=mstange

In child processes on MacOSX we don't spin native event loop at all.
Without native event loops NSWorkspace.accessibilityDisplayShouldReduceMotion
doesn't return up-to-date value when the system setting changed for some reasons.
To workaround this we use NSWorkspace.accessibilityDisplayShouldReduceMotion
only on the parent process which spins native event loop or when it's the
initial query on the child process.  And we give the up-to-date value to the
child process via an IPC call just like other cached values do.

Depends on D5002

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1486971 - Test for dynamically change of the prefers-reduced-motion setting on MacOSX. r=mstange

The framework to simulate the setting change works as following;

 - nsIDOMWindowUtils.setPrefersReducedMotion() calls an IPC function which ends
   up calling nsChildView::SetPrefersReducedMotion() in the parent process

 - nsChildView::SetPrefersReducedMotion() sets the given value into
   nsLookAndFeel::mPrefersReducedMotionCached just like we set the value queried
   via NSWorkspace.accessibilityDisplayShouldReduceMotion in the parent process
   and send a notification which is the same notification MacOSX sends when the
   system setting changed

 - Normally the cached value is cleared before quering new values since the
   cache value is stale, but in this case the value is up-to-date one, so
   nsChildView::SetPrefersReducedMotion() tells that we don't need to clear the
   cache, and nsIDOMWindowUtils.resetPrefersReducedMotion() resets that state
   of 'we don't need to clear the cache'

There are two test cases with the framework in this commit, one is just setting
the value and checking the value queried by window.matchMedia.  The other one is
receiving 'change' event and checking the value of the event target.

Note that to make this test works the patch for bug 1478212 is necessary since
the test runs in an iframe.

Depends on D5003

Comment 16

[Alex Gaynor [:Alex_Gaynor]]

Thanks for taking this on. Sorry I didn't notice when I landed the native event loop patch.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Alex, though I actually add the bug into blocker list, as for this 'prefers-reduced-motion', the feature hasn't landed at that moment, so that's not your fault at all.  And I can imagine it's really hard to notice it, to me it's impossible, how can we know the fact that some of the system properties can not be delivered without spinning native event loops in a proprietary software without any documents for the fact?

Upgrading priority since I'd want to uplift this because users who try 'prefers-reduced-motion' will complain that the feature is totally broken.  Normally the system setting is rarely changed, but the users who want to try this new feature will change the setting many times.

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 9006437 [details]
Bug 1486971 - Query NSWorkspace.accessibilityDisplayShouldReduceMotion only if it's on parent processes or it's the initial query on child processes. r=mstange

Markus Stange [:mstange] has approved the revision.

Comment 19

[Markus Stange [:mstange]]

Comment on attachment 9006435 [details]
Bug 1486971 - Receive accessibilityDisplayShouldReduceMotion change and notify it. r=mstange

Markus Stange [:mstange] has approved the revision.

Comment 20

[Markus Stange [:mstange]]

If the system wants to update these values, it needs to run code to do that updating. It expects to be able to run this code on the main thread. But we never give it a chance to run the code because we now completely control everything that runs at all times - we never give the system an opportunity to do on demand work.
I suppose it could run its updating code synchronously whenever we query the values. But it's not designed for that, it assumes that there is a native event loop.

Comment 21

[Markus Stange [:mstange]]

Comment on attachment 9006438 [details]
Bug 1486971 - Test for dynamically change of the prefers-reduced-motion setting on MacOSX. r=mstange

Markus Stange [:mstange] has approved the revision.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9006438 [details]
Bug 1486971 - Test for dynamically change of the prefers-reduced-motion setting on MacOSX. r=mstange

Nathan, can you please check the changes in sync-messages.ini?  There are new two functions only for test.

(I couldn't find a way to add a review request in the revision on Phabricator)

Comment 23

[Nathan Froyd [:froydnj]]

Comment on attachment 9006438 [details]
Bug 1486971 - Test for dynamically change of the prefers-reduced-motion setting on MacOSX. r=mstange

Nathan Froyd [:froydnj] has approved the revision.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e92f3e990b35
Receive accessibilityDisplayShouldReduceMotion change and notify it. r=mstange
https://hg.mozilla.org/integration/autoland/rev/cea8a92452d5
Query NSWorkspace.accessibilityDisplayShouldReduceMotion only if it's on parent processes or it's the initial query on child processes. r=mstange
https://hg.mozilla.org/integration/autoland/rev/67a5acf7363d
Test for dynamically change of the prefers-reduced-motion setting on MacOSX. r=froydnj,mstange

Comment 25

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/e92f3e990b35
https://hg.mozilla.org/mozilla-central/rev/cea8a92452d5
https://hg.mozilla.org/mozilla-central/rev/67a5acf7363d

Comment 26

[Sören Hentzschel]

Tested with today's Nightly, about:buildconfig says "Built from https://hg.mozilla.org/mozilla-central/rev/73a2f427e2fdd0511198a2e308c21f3b74ca7f7d", so these patches should be included. I also tested with a current copy of mozilla-central and a local build. Firefox still requires a restart to detect a change of the option on macOS Mojave.

Comment 27

[Hiroyuki Ikezoe (:hiro)]

(In reply to Sören Hentzschel from comment #26)
> Tested with today's Nightly, about:buildconfig says "Built from
> https://hg.mozilla.org/mozilla-central/rev/
> 73a2f427e2fdd0511198a2e308c21f3b74ca7f7d", so these patches should be
> included. I also tested with a current copy of mozilla-central and a local
> build. Firefox still requires a restart to detect a change of the option on
> macOS Mojave.

Hmm it works fine on MacOSX 10.13.6.  Something has changed in Majave?  Or, I haven't checked the behavior on chrome content, so there may be other issues (only on MacOSX?).  Can you please upload your current patches in bug 1478597?

Comment 28

[Sören Hentzschel]

I tested it independent from bug 1478597 for website content with the following code:

```
<html>
<head>

</head>
<body>
<script>
  const reducedMotion = window.matchMedia("(prefers-reduced-motion)").matches;

  console.log(reducedMotion);

  window.matchMedia('(prefers-reduced-motion)').addEventListener('change', event => {
    console.log('A');
  });

  window.matchMedia('(prefers-reduced-motion: reduce)').addEventListener('change', event => {
    console.log('B');
  });

  window.matchMedia('(prefers-reduced-motion: no-preference)').addEventListener('change', event => {
    console.log('C');
  });
</script>
</body>
</html>
```

'A', 'B' and 'C' were never logged. After changing the OS setting and reloading the file the content of the first console.log didn't change. The value only changed after a restart of Firefox.

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Sören, can you please try this binary to see if it works or not?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c92696dc85007e12c24c8bee47c94432f0e3fa5f

It might be possible that on Mojave or laters `defaultCenter` doesn't receive NSWorkspaceAccessibilityDisplayOptionsDidChangeNotification.  We might need to use sharedWorkspace.notificationCenter instead there.  Just a guess though.

Comment 30

[Sören Hentzschel]

Good news: with the build from comment #29 it works on macOS Mojave!

Comment 31

[Hiroyuki Ikezoe (:hiro)]

(In reply to Sören Hentzschel (not available from 2018/09/20 until 2018/09/27) from comment #30)
> Good news: with the build from comment #29 it works on macOS Mojave!

Thanks! Filed bug 1492284.

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9006435 [details]
Bug 1486971 - Receive accessibilityDisplayShouldReduceMotion change and notify it. r=mstange

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1475462
[User impact if declined]: User will not see prefers-reduced-motion changes until the browser restarts
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: I don't think so
[List of other uplifts needed for the feature/fix]: All patches in this bug, bug 1478212 and bug 1491580
[Is the change risky?]:  Now it's low, actually this patch caused a crash on MacOSX 10.9, but it has already been fixed by bug 1491580.
[Why is the change risky/not risky?]: Basically the patch does just observe an notification event, unfortunately it caused the crash on the older version of MacOSX, but it's been already fixed.
[String changes made/needed]: None

Comment 33

[Hiroyuki Ikezoe (:hiro)]

Note that, FWIW, :Anca has been working on prefers-reduced-motion QA.

Comment 34

[Pascal Chevrel:pascalc]

Hiro, I see that this uplift request needs also uplifting patches from 2 other bugs, one of them being a P3 and the other a crash caused by the patch you want to uplift. This is not noted as a regression and according to https://developer.mozilla.org/en-US/docs/Web/CSS/@media/prefers-reduced-motion only Safari supports that feature today. Given that it already caused crashes, how important is it to have it in 63 for OSX and not let it ride the trains with 64?

Comment 35

[Hiroyuki Ikezoe (:hiro)]

I think _conscious_ web developers on MacOSX try to use the new released prefers-reduced-motion on Firefox 63, and they will think it's completely broken because the feature values don't reflect when the corresponding system setting changes, and they will think Firefox isn't a polished product.  Yes, I understand if it caused other crashes the impression will be more worse, but I believe these patches won't cause any other crashes.

Comment 36

[Markus Stange [:mstange]]

I agree with Hiro.

Comment 37

[Pascal Chevrel:pascalc]

Comment on attachment 9006435 [details]
Bug 1486971 - Receive accessibilityDisplayShouldReduceMotion change and notify it. r=mstange

Uplift approved for 63 beta 8 based on the assessment in comments #35 and #36, Thanks.

Comment 38

[Pascal Chevrel:pascalc]

Hiro, please request uplist for the patches in the 2 other bugs (bug 1478212 and bug 1491580), thanks.

Comment 39

[Hiroyuki Ikezoe (:hiro)]

Done.

Comment 40

[Dorel Luca [:dluca]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8d26699d84cf
https://hg.mozilla.org/releases/mozilla-beta/rev/78dc157290d3
https://hg.mozilla.org/releases/mozilla-beta/rev/b84a0e018ea8

Comment 41

[Anca Soncutean, Desktop QA]

I reproduced this issue on Beta 63.0b5 (20180910132416) under macOS 10.13. 
This is fixed on Beta 63.0b11 (20181001131022) and Nightly 64.0a1 (2018-10-03), under macOS 10.13 and macOS 10.12.