1449665 - Show that a WebExtension is managing the document color settings

Status: NEW

(WebExtensions :: Frontend, defect, P3)

Description

[Shane Caraveo (:mixedpuppy)]

browserSettings.overrideDocumentColors is being added in bug 1417810.  about:preferences needs to be updated to show an extension is controlling this setting.

Comment 1

[Mark Striemer [:mstriemer]]

This dropdown is defined in colors.xul [1]. A message similar to the proxy.xul file will need to be added which can be seen in the diff [2]. With the message added it can be triggered similarly to the proxy.js file [3].

If someone would like to work on this and has any questions please let me know.

[1] https://searchfox.org/mozilla-central/rev/028cd8342899357d80fafba2d528a0fd5819e316/browser/components/preferences/colors.xul#94-103
[2] https://searchfox.org/mozilla-central/diff/fdb3acbc2ee5dea3253199f765a6ccd2d8e78c23/browser/components/preferences/connection.xul#40
[3] https://searchfox.org/mozilla-central/diff/fdb3acbc2ee5dea3253199f765a6ccd2d8e78c23/browser/components/preferences/connection.js#255-273

Comment 2

[Satish Pasupuleti [:satishp]]

(In reply to Mark Striemer [:mstriemer] from comment #1)
> This dropdown is defined in colors.xul [1]. A message similar to the
> proxy.xul file will need to be added which can be seen in the diff [2]. With
> the message added it can be triggered similarly to the proxy.js file [3].
> 
> If someone would like to work on this and has any questions please let me
> know.
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 028cd8342899357d80fafba2d528a0fd5819e316/browser/components/preferences/
> colors.xul#94-103
> [2]
> https://searchfox.org/mozilla-central/diff/
> fdb3acbc2ee5dea3253199f765a6ccd2d8e78c23/browser/components/preferences/
> connection.xul#40
> [3]
> https://searchfox.org/mozilla-central/diff/
> fdb3acbc2ee5dea3253199f765a6ccd2d8e78c23/browser/components/preferences/
> connection.js#255-273

Thanks for the tips, Mark! I will study this and let you know when I take this up.

Comment 3

[Satish Pasupuleti [:satishp]]

Attachment: Controlled by extension illustration

Mark, I am done with a draft for this.
I noticed that for some other preferences, the 'controlling extension' has exclusive 'write' so the setting in about:preferences gets disabled. I am not sure I would want that here - any extension can change this setting but I would also prefer to let the user change it using the menulist in the Colors dialog. We can still show the 'controlled by <abc> extension' UI to keep the user informed - just not disable the original setting UI. The attached image may illustrate this better.

Point to note is how this would be handled if there is more than one extension that is controlling this setting. My guess is, the most recent extension that changed this setting will be shown.

Let me know what you think and I can push the changes to MozReview.

Btw, please assign the bug to me, I could not figure out how to do that.

Comment 4

[Tim Nguyen :ntim]

I think it's better to disable the setting in about:preferences, because whatever the user sets will be overriden by the extension at some point.

Comment 5

[Satish Pasupuleti [:satishp]]

Attachment: Bug 1449665 - changes to indicate a WebExtension is overriding document color settings

Review commit: https://reviewboard.mozilla.org/r/235568/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235568/

Comment 6

[Satish Pasupuleti [:satishp]]

(In reply to Tim Nguyen :ntim from comment #4)
> I think it's better to disable the setting in about:preferences, because
> whatever the user sets will be overriden by the extension at some point.

Sorry, I saw your comment after I pushed my changes to MozReview.

Yes, the setting will be overridden by the extension, but what I think is - the user is already here (on the Colors dialog) and disabling the dropdown menulist is telling the user to go open that extension and do the same thing from there. Instead if we leave it enabled, user can change it right here. If an extension changed the setting, the message will inform the user that too.

So in my view, it's like having two ways to do the same thing. Disabling the setting in Colors dialog will only increase the number of clicks for the user (after all, the user navigated to the Colors dialog). Leaving it enabled causes no problems (I checked with a simple extension).

I would like to see what everyone thinks and if this aligns with the broader Firefox standard.

Comment 7

[Tim Nguyen :ntim]

(In reply to Satish Pasupuleti [:satishp] from comment #6)

> So in my view, it's like having two ways to do the same thing. Disabling the
> setting in Colors dialog will only increase the number of clicks for the
> user (after all, the user navigated to the Colors dialog). Leaving it
> enabled causes no problems (I checked with a simple extension).
>
I'm pretty sure the extension would override the setting again on the next startup, which is more confusing.

To solve the extra clicks, you could add a "Disable Extension" button in the banner that re-enables the selectbox.

Comment 8

[Satish Pasupuleti [:satishp]]

(In reply to Tim Nguyen :ntim from comment #7)

> I'm pretty sure the extension would override the setting again on the next
> startup, which is more confusing.
> 
Ohh! Firefox remembers the setting change and applies it back on startup? Or is it the extension that might do this on startup?
If it's the former, then you are right that it would confuse users.
If it's the latter then it depends on the author. For example, my simple extension doesn't do anything on load/startup. It will change the setting only when user chooses one and does not care if user changed it back using about:preferences or another extension. Ofcourse other extension authors may do this and I see your point. 
 
> To solve the extra clicks, you could add a "Disable Extension" button in the
> banner that re-enables the selectbox.
Yes, the Disable Extension button is already there (reference image in Comment #3). I did not change that part - the only part I changed is disabling the selectbox, as shown in that image.

Comment 9

[Tim Nguyen :ntim]

(In reply to Satish Pasupuleti [:satishp] from comment #8)
> (In reply to Tim Nguyen :ntim from comment #7)
> 
> > I'm pretty sure the extension would override the setting again on the next
> > startup, which is more confusing.
> > 
> Ohh! Firefox remembers the setting change and applies it back on startup? Or
> is it the extension that might do this on startup?

The extension might automatically set it on every startup :)


> > To solve the extra clicks, you could add a "Disable Extension" button in the
> > banner that re-enables the selectbox.
> Yes, the Disable Extension button is already there (reference image in
> Comment #3). I did not change that part - the only part I changed is
> disabling the selectbox, as shown in that image.

Then changing the setting is just one click away right ? If you click "Disable Extension", then that should allow you to change the setting .

Comment 10

[Mark Striemer [:mstriemer]]

The approach we've taken so far is that the user is giving up control of this setting to an extension. It is indeed more clicks to go back to the extension, but the banner may serve as a reminder that they installed an extension which actually makes this simpler (or it doesn't in which case you may want to disable it).

Since the extension can control the setting, there's nothing stopping it from changing the setting automatically as ntim pointed out. There is a situation where we allow users to change a setting without disabling the extension (search engines) but search engines can only be set on install so we don't need to worry about this case.

Comment 11

[Mark Striemer [:mstriemer]]

Meridel, can you provide some copy for this please? I think something like "An extension, <extension name>, controls when page colors are overriden with your selection above."

Comment 12

[Meridel [:meridel]]

Sure thing. I'd like to see what this situation looks like in action. What's an extension I could install to see this behavior?

Comment 13

[Mark Striemer [:mstriemer]]

Comment on attachment 8966908 [details]
Bug 1449665 - changes to indicate a WebExtension is overriding document color settings

https://reviewboard.mozilla.org/r/235568/#review241588

This is looking really good, thanks for putting it together!

::: browser/components/preferences/colors.js:22
(Diff revision 1)
>    { id: "browser.display.background_color", type: "string" },
>    { id: "browser.display.use_system_colors", type: "bool" },
>  ]);
> +
> +window.addEventListener("DOMContentLoaded", () => {
> +

nit: no need for a blank line here

::: browser/components/preferences/colors.js:27
(Diff revision 1)
> +
> +  document
> +    .getElementById("disableColorsExtension")
> +    .addEventListener(
> +      "command", makeDisableControllingExtension(
> +        PREF_SETTING_TYPE, DOC_COLORS_KEY).bind(gColorsDialog));

I don't think this bind is necessary. I'm not sure why it's in the proxy code I linked too either.

::: browser/components/preferences/colors.js:29
(Diff revision 1)
> +    .getElementById("disableColorsExtension")
> +    .addEventListener(
> +      "command", makeDisableControllingExtension(
> +        PREF_SETTING_TYPE, DOC_COLORS_KEY).bind(gColorsDialog));
> +
> +  let deferredUpdate = new DeferredTask(() => {

I should have mentioned in my comment that we don't need the `DeferredTask`. That was used because many prefs might be changed at once for proxy, this is just managing one pref though.

::: browser/components/preferences/colors.js:35
(Diff revision 1)
> +    gColorsDialog.updateColorsDialogUI();
> +  }, 10);
> +
> +  let docColorsObserver = {
> +    observe: (subject, topic, data) => {
> +      deferredUpdate.arm();

This can just be `gColorsDialog.updateColorsDialogUI()`.

::: browser/components/preferences/colors.js:45
(Diff revision 1)
> +  window.addEventListener("unload", () => {
> +    Services.prefs.removeObserver(PREF_DOC_COLOR_USE, docColorsObserver);
> +  });
> +
> +  gColorsDialog.updateColorsDialogUI();
> +}, { once: true, capture: true });

nit: The object here shouldn't have spaces inside the curlies.

`{once: true, capture: true}`

Comment 14

[Satish Pasupuleti [:satishp]]

(In reply to Meridel from comment #12)
> Sure thing. I'd like to see what this situation looks like in action. What's
> an extension I could install to see this behavior?

I put together a simple extension to test the original bug fix here - https://bugzilla.mozilla.org/show_bug.cgi?id=1417810#c53
You can install that attachment to see this in action.

Comment 15

[Mark Striemer [:mstriemer]]

Comment on attachment 8965220 [details]
Controlled by extension illustration

Is that how the dropdown normally looks on Windows? It's full width on Mac, so make sure its styling doesn't change. As noted above, we'll need to disable the dropdown when it is being controlled.

I think the controlled message should also go above the dropdown but I'll confirm with a designer today, we don't seem to be entirely consistent anymore.

Comment 16

[Meridel [:meridel]]

In the attachment 8965220 [details], why is the extension "also" controlling in this instance? Is it that the user still has control over some of the color settings but not others? If so, which of these is being controlled by user and which by extension?

If it is all-or-nothing, the string would be:

"An extension, <icon> <insert name of extension>, controls your color settings."

Comment 17

[Mark Striemer [:mstriemer]]

That image was uploaded by Satish and I think the hope was that the user would still be able to change the setting, but we have to do all-or-nothing.

This setting is just for when the color settings are used, not what the colors are, from my understanding. So that might be too generic. Maybe something like:

"An extension, <icon> <insert name of extension>, controls when your color settings are used."

Comment 18

[Satish Pasupuleti [:satishp]]

Attachment: Images showing the Colors dialog with and without extension

Attached is a zip with new message and aligning with behaviour elsewhere on Firefox - disabling the setting in about:preferences when the extension is controlling it.

1-hc-without-extn.png
2-hc-with-extn.png
These two attachments show the Colors dialog in HC mode with and without the extension actively controlling the setting.

3-never-without-extn.png
4-never-with-extn.png
These two attachments show the Colors dialog with 'Never' chosen - with and without the extension actively controlling the setting.

The earlier image with the dropdown only partly visible is a mistake - probably some part of code was missing. These new images show it right.

Mark, confirm the message and the location (if it should be above the setting) and I will push the changes to MozReview. Rest of your review comments have been handled.

Comment 19

[Meridel [:meridel]]

Hi Mark,

I'd like to change the current Preferences string - by doing so, I think the extension string will make more sense. I ran it by Michelle - she is content lead for Prefs - and she agreed. 

-Replace "Override the colors specified by the page with your selections above" with "Apply your settings:"

-When an extension is controlling when color settings are applied:

"An extension, <icon> <insert name of extension>, controls when your color settings are enabled."


Do I need to file a separate bug to get the Prefs string changed?

Comment 20

[Mark Striemer [:mstriemer]]

I just discussed this with Markus over Slack and we're going to include this (along with the possibility of more of these banners) in our Thursday meeting's discussion.

NI? on Markus and me to ensure we follow up properly.

Comment 21

[Markus Jaritz [:designakt] (UX)]

After our discussion it is clear that we can not scale the current solution for how overrides work to any possible new override that will get implemented. (As this trends towards one override message next to every pref.) 
We will look into this more and see how we can build a more scalable model. Ideas discussed for this is defining a clear hirarchy of control that has user input at the top, grouping overrides by topics (as we did with cookie overrides, and like permissions are grouped), and get clear on what preferences should be able to change, and which should not.

As for this override, I propose we wait with implementing it until we have a better model of ensuring user control.

Comment 22

[Markus Jaritz [:designakt] (UX)]

I did not intend to needinfo emanuela, just wanted to cc her. Removing her needinfo again.

Comment 23

[Meridel [:meridel]]

Hi Markus and Mark, I'm very sorry to have missed the meeting today. Are you needing content or review from me at this point?

Comment 24

[Markus Jaritz [:designakt] (UX)]

(In reply to Meridel from comment #23)
> Hi Markus and Mark, I'm very sorry to have missed the meeting today. Are you
> needing content or review from me at this point?

We could need your help on how we group/structure overrides so that they make sense to users, if we decide to go in that direction. We will continue our conversation in next Thursdays meeting to define next steps.