Repository: qutebrowser/qutebrowser
Pull request: Fix dark color preference with Qt 5.15.2
QtWebEngine 5.15.2 doesn't toggle the preferred color-scheme media query
when using `--force-dark-mode`, instead a blink-specific setting needs
to be toggled.

Note: This same setting could probably be used on earlier versions as
well, but to avoid breaking anything that already works, only
QtWebEngine 5.15.2 and above will use this alternate setting.

Note 2: It's not clear that this is the correct value of
`preferredColorScheme`. According to the Chromium source, `kDark` is 0
and `kLight` is 1, but setting `preferredColorScheme` to 0 causes it to
use light mode.

Closes #5915 

Problem statement:
colors.webpage.prefers_color_scheme_dark breaks with Qt 5.15.2
```
.tox/py38-pyqt515/bin/python3 -m qutebrowser --temp-basedir -s colors.webpage.prefers_color_scheme_dark true ':later 500 jseval matchMedia("(prefers-color-scheme: dark)").matches'
```

This shows "True" on QtWebEngine 5.15.1 and below, but "False" on QtWebEngine 5.15.2.

I suspect the `--force-dark-mode` Chromium flag got removed/renamed between Chromium 80 and 83.

Hints from issue discussion:
According to `ekpneo` who originally reported this in IRC, passing `--blink-settings=preferredColorScheme=1` instead should work.

Commit message: Add invocation test for preferred colorscheme

With the addition of blink-settings for ensuring the correct color
scheme, this test makes sure the browser is obeying it via a media
match query through javascript. Any HTML page will do, so
`qute://version` is opened and tested against.

Patch to review:
diff --git a/qutebrowser/browser/webengine/darkmode.py b/qutebrowser/browser/webengine/darkmode.py
index 630a7bf9e14..5ae0793924a 100644
--- a/qutebrowser/browser/webengine/darkmode.py
+++ b/qutebrowser/browser/webengine/darkmode.py
@@ -264,6 +264,10 @@ def _variant() -> Variant:
 
 def settings() -> Iterator[Tuple[str, str]]:
     """Get necessary blink settings to configure dark mode for QtWebEngine."""
+    if (qtutils.version_check('5.15', compiled=False) and
+            config.val.colors.webpage.prefers_color_scheme_dark):
+        yield "preferredColorScheme", "1"
+
     if not config.val.colors.webpage.darkmode.enabled:
         return
 
diff --git a/qutebrowser/config/qtargs.py b/qutebrowser/config/qtargs.py
index 8ab93c904a7..663296bbcbd 100644
--- a/qutebrowser/config/qtargs.py
+++ b/qutebrowser/config/qtargs.py
@@ -203,12 +203,18 @@ def _qtwebengine_settings_args() -> Iterator[str]:
         }
     }
 
-    referrer_setting = settings['content.headers.referer']
-    if qtutils.version_check('5.14', compiled=False):
+    if (qtutils.version_check('5.14', compiled=False) and
+            not qtutils.version_check('5.15', compiled=False)):
+        # In Qt 5.14, `--force-dark-mode` is used to set the preferred
+        # colorscheme. In Qt 5.15, this is handled by a blink-setting
+        # instead.
         settings['colors.webpage.prefers_color_scheme_dark'] = {
             True: '--force-dark-mode',
             False: None,
         }
+
+    referrer_setting = settings['content.headers.referer']
+    if qtutils.version_check('5.14', compiled=False):
         # Starting with Qt 5.14, this is handled via --enable-features
         referrer_setting['same-domain'] = None
     else:
diff --git a/tests/end2end/test_invocations.py b/tests/end2end/test_invocations.py
index e34bd912d30..74288fbc3ea 100644
--- a/tests/end2end/test_invocations.py
+++ b/tests/end2end/test_invocations.py
@@ -417,3 +417,23 @@ def test_referrer(quteproc_new, server, server2, request, value, expected):
             expected = expected.replace(key, str(val))
 
     assert headers.get('Referer') == expected
+
+
+@pytest.mark.qtwebkit_skip
+@utils.qt514
+def test_preferred_colorscheme(request, quteproc_new):
+    """Make sure the the preferred colorscheme is set."""
+    args = _base_args(request.config) + [
+        '--temp-basedir',
+        '-s', 'colors.webpage.prefers_color_scheme_dark', 'true',
+    ]
+    quteproc_new.start(args)
+
+    quteproc_new.open_path("qute://version")
+    quteproc_new.send_cmd(':jseval --world main '
+                          'matchMedia("(prefers-color-scheme: dark)").matches')
+    line = quteproc_new.wait_for(message='True')
+    line.expected = True
+
+    quteproc_new.send_cmd(':quit')
+    quteproc_new.wait_for_quit()
diff --git a/tests/helpers/utils.py b/tests/helpers/utils.py
index 2c275bf157c..3b5ee0ab626 100644
--- a/tests/helpers/utils.py
+++ b/tests/helpers/utils.py
@@ -44,6 +44,9 @@
 qt514 = pytest.mark.skipif(
     not qtutils.version_check('5.14'), reason="Needs Qt 5.14 or newer")
 
+qt515 = pytest.mark.skipif(
+    not qtutils.version_check('5.15'), reason="Needs Qt 5.15 or newer")
+
 
 class PartialCompareOutcome:
 
diff --git a/tests/unit/browser/webengine/test_darkmode.py b/tests/unit/browser/webengine/test_darkmode.py
index 3e62000d20f..0c39cf95c60 100644
--- a/tests/unit/browser/webengine/test_darkmode.py
+++ b/tests/unit/browser/webengine/test_darkmode.py
@@ -32,6 +32,19 @@ def patch_backend(monkeypatch):
     monkeypatch.setattr(objects, 'backend', usertypes.Backend.QtWebEngine)
 
 
+@pytest.mark.parametrize('enabled, expected', [
+    # Disabled or nothing set
+    (False, []),
+
+    # Enabled in configuration
+    (True, [("preferredColorScheme", "1")]),
+])
+@utils.qt515
+def test_colorscheme(config_stub, monkeypatch, enabled, expected):
+    config_stub.set_obj('colors.webpage.prefers_color_scheme_dark', enabled)
+    assert list(darkmode.settings()) == expected
+
+
 @pytest.mark.parametrize('settings, expected', [
     # Disabled
     ({}, []),
diff --git a/tests/unit/config/test_qtargs.py b/tests/unit/config/test_qtargs.py
index 051956a0091..d26a8ab5069 100644
--- a/tests/unit/config/test_qtargs.py
+++ b/tests/unit/config/test_qtargs.py
@@ -289,20 +289,20 @@ def test_referer(self, config_stub, monkeypatch, parser, qt_version, referer, ar
         else:
             assert arg in args
 
-    @pytest.mark.parametrize('dark, new_qt, added', [
-        (True, True, True),
-        (True, False, False),
-        (False, True, False),
-        (False, False, False),
+    @pytest.mark.parametrize('dark, qt_version, added', [
+        (True, "5.13", False),
+        (True, "5.14", True),
+        (True, "5.15", False),
+        (False, "5.13", False),
+        (False, "5.14", False),
+        (False, "5.15", False),
     ])
     @utils.qt514
     def test_prefers_color_scheme_dark(self, config_stub, monkeypatch, parser,
-                                       dark, new_qt, added):
+                                       dark, qt_version, added):
         monkeypatch.setattr(qtargs.objects, 'backend',
                             usertypes.Backend.QtWebEngine)
-        monkeypatch.setattr(qtargs.qtutils, 'version_check',
-                            lambda version, exact=False, compiled=True:
-                            new_qt)
+        monkeypatch.setattr(qtargs.qtutils, 'qVersion', lambda: qt_version)
 
         config_stub.val.colors.webpage.prefers_color_scheme_dark = dark
 


Code review comments:
diff_hunk: @@ -264,6 +264,10 @@ def _variant() -> Variant:
 
 def settings() -> Iterator[Tuple[str, str]]:
     """Get necessary blink settings to configure dark mode for QtWebEngine."""
+    if (qtutils.version_check('5.15', compiled=False) and
path: qutebrowser/browser/webengine/darkmode.py
text: @user1:
```suggestion
    if (qtutils.version_check('5.15.2', compiled=False) and
```

diff_hunk: @@ -417,3 +417,23 @@ def test_referrer(quteproc_new, server, server2, request, value, expected):
             expected = expected.replace(key, str(val))
 
     assert headers.get('Referer') == expected
+
+
+@pytest.mark.qtwebkit_skip
+@utils.qt514
+def test_preferred_colorscheme(request, quteproc_new):
+    """Make sure the the preferred colorscheme is set."""
+    args = _base_args(request.config) + [
+        '--temp-basedir',
+        '-s', 'colors.webpage.prefers_color_scheme_dark', 'true',
+    ]
+    quteproc_new.start(args)
+
+    quteproc_new.open_path("qute://version")
+    quteproc_new.send_cmd(':jseval --world main '
+                          'matchMedia("(prefers-color-scheme: dark)").matches')
+    line = quteproc_new.wait_for(message='True')
+    line.expected = True
+
+    quteproc_new.send_cmd(':quit')
+    quteproc_new.wait_for_quit()
path: tests/end2end/test_invocations.py
text: @user1:
The `quteproc_new` [fixture](https://docs.pytest.org/en/stable/fixture.html) already takes care of cleaning up the qutebrowser process - I think (some of) the other tests doing this manually is an accident.

diff_hunk: @@ -32,6 +32,19 @@ def patch_backend(monkeypatch):
     monkeypatch.setattr(objects, 'backend', usertypes.Backend.QtWebEngine)
 
 
+@pytest.mark.parametrize('enabled, expected', [
+    # Disabled or nothing set
+    (False, []),
+
+    # Enabled in configuration
+    (True, [("preferredColorScheme", "1")]),
+])
+@utils.qt515
path: tests/unit/browser/webengine/test_darkmode.py
text: @user1:
Why not make this `qt514` and parametrize the Qt version as well, so that we can make sure it doesn't get added on Qt < 5.15.2?

@author:
Good idea. I'll do that.

diff_hunk: @@ -32,6 +32,19 @@ def patch_backend(monkeypatch):
     monkeypatch.setattr(objects, 'backend', usertypes.Backend.QtWebEngine)
 
 
+@pytest.mark.parametrize('enabled, expected', [
+    # Disabled or nothing set
+    (False, []),
+
+    # Enabled in configuration
+    (True, [("preferredColorScheme", "1")]),
+])
+@utils.qt515
+def test_colorscheme(config_stub, monkeypatch, enabled, expected):
+    config_stub.set_obj('colors.webpage.prefers_color_scheme_dark', enabled)
path: tests/unit/browser/webengine/test_darkmode.py
text: @user1:
```suggestion
    config_stub.val.colors.webpage.prefers_color_scheme_dark = enabled
```

diff_hunk: @@ -417,3 +417,23 @@ def test_referrer(quteproc_new, server, server2, request, value, expected):
             expected = expected.replace(key, str(val))
 
     assert headers.get('Referer') == expected
+
+
+@pytest.mark.qtwebkit_skip
+@utils.qt514
+def test_preferred_colorscheme(request, quteproc_new):
+    """Make sure the the preferred colorscheme is set."""
+    args = _base_args(request.config) + [
+        '--temp-basedir',
+        '-s', 'colors.webpage.prefers_color_scheme_dark', 'true',
+    ]
+    quteproc_new.start(args)
+
+    quteproc_new.open_path("qute://version")
path: tests/end2end/test_invocations.py
text: @user1:
`_base_args` above already opens `about:blank`, so I don't think this is needed.

@author:
You're right. For some reason I thought that `about:blank` had no JS context. Woops.

diff_hunk: @@ -417,3 +417,23 @@ def test_referrer(quteproc_new, server, server2, request, value, expected):
             expected = expected.replace(key, str(val))
 
     assert headers.get('Referer') == expected
+
+
+@pytest.mark.qtwebkit_skip
+@utils.qt514
+def test_preferred_colorscheme(request, quteproc_new):
+    """Make sure the the preferred colorscheme is set."""
+    args = _base_args(request.config) + [
+        '--temp-basedir',
+        '-s', 'colors.webpage.prefers_color_scheme_dark', 'true',
+    ]
+    quteproc_new.start(args)
+
+    quteproc_new.open_path("qute://version")
+    quteproc_new.send_cmd(':jseval --world main '
+                          'matchMedia("(prefers-color-scheme: dark)").matches')
+    line = quteproc_new.wait_for(message='True')
+    line.expected = True
path: tests/end2end/test_invocations.py
text: @user1:
This shouldn't be needed - the `expected` attribute is to mark error messages as expected so they don't fail the test.

diff_hunk: @@ -417,3 +417,23 @@ def test_referrer(quteproc_new, server, server2, request, value, expected):
             expected = expected.replace(key, str(val))
 
     assert headers.get('Referer') == expected
+
+
+@pytest.mark.qtwebkit_skip
+@utils.qt514
+def test_preferred_colorscheme(request, quteproc_new):
+    """Make sure the the preferred colorscheme is set."""
+    args = _base_args(request.config) + [
+        '--temp-basedir',
+        '-s', 'colors.webpage.prefers_color_scheme_dark', 'true',
+    ]
+    quteproc_new.start(args)
+
+    quteproc_new.open_path("qute://version")
+    quteproc_new.send_cmd(':jseval --world main '
path: tests/end2end/test_invocations.py
text: @user1:
Is the `--world main` here needed?

@author:
Nope. I'll remove it.

diff_hunk: @@ -203,12 +203,18 @@ def _qtwebengine_settings_args() -> Iterator[str]:
         }
     }
 
-    referrer_setting = settings['content.headers.referer']
-    if qtutils.version_check('5.14', compiled=False):
+    if (qtutils.version_check('5.14', compiled=False) and
+            not qtutils.version_check('5.15', compiled=False)):
path: qutebrowser/config/qtargs.py
text: @user1:
```suggestion
            not qtutils.version_check('5.15.2', compiled=False)):
```