Closed Bug 1417810 Opened 2 years ago Closed 2 years ago

Provide API to override the document colors

Categories

(WebExtensions :: General, enhancement, P2)

57 Branch
enhancement

Tracking

(firefox61 verified)

VERIFIED FIXED
mozilla61
Tracking Status
firefox61 --- verified

People

(Reporter: satishp, Assigned: satishp, Mentored)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, good-first-bug, Whiteboard: [design-decision-approved][browserSettings])

Attachments

(3 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171112125346

Steps to reproduce:

The newer WebExtensions API does not let us change the preference 'browser.display.document_color_use' to override document colors with those chosen by user.

Background:
Firefox's preference in Options - Fonts & Colors - Colors, where user can provide his/her own colors to override those in the document. The override switch has 3 choices - Always, Only with High Contrast themes and Never.
My OS is always in High Contrast mode and hence leaving the choice at 'Only with High Contrast themes' works for most part.
However some web pages are simply useless when the colors are overridden - the text is somehow the same color as the chosen background. Also most images do not show up at all.

My extension (using legacy AddonSDK) adds a toolbar button for easy switching among the 3 choices - Always, Only with High Contrast themes and Never. So on a web page that is not displayed well enough with HC theme, I can simply go to the toolbar button and choose the option Never - this will make Firefox use the document's colors instead of my choice and thereby I can view what's missing and provide any information in the input fields, if relevant.



Actual results:

API is unavailable thus my extension is of no use.

I have to open the Options page, browse to Fonts and Colors section (or search for 'Colors', open the Colors dialog and change the choice - this is too many clicks (not considering the scrolling or search input) and inconvenient.


Expected results:

Access to the "browser.display.document_color_use" preference through WebExtensions API would let users move between the choices easily with a simple toolbar button and only 2 clicks.
Component: Untriaged → WebExtensions: General
Product: Firefox → Toolkit
Blocks: 1363856
Adding to API triage.
Priority: -- → P5
Whiteboard: [design-decision-needed][browserSettings]
Since this has to do with accessability, I'm bumping the priority to P2.
Severity: normal → enhancement
Priority: P5 → P2
Hi psatish, this has been added to the agenda for the November 28, 2017 WebExtensions APIs triage meeting. Would you be able to join us? 

Here’s a quick overview of what to expect at the triage: 

* We normally spend 5 minutes per bug
* The more information in the bug, the better
* The goal of the triage is to give a general thumbs up or thumbs down on a proposal; we won't be going deep into implementation details

Relevant Links: 

* Wiki for the meeting: https://wiki.mozilla.org/Add-ons/Contribute/Triage
* Meeting agenda: https://docs.google.com/document/d/1MduEIKmXDdj3p94PJDrPPPYvbSfEdrrMF-UhMnHPqEQ/edit#heading=h.edrw957gm8hg
* Vision doc for WebExtensions: https://wiki.mozilla.org/WebExtensions/Vision
Unfortunately no - my timezone is UTC+0530 making it 12AM and too late for me. I can provide more information when required though.

As an example, here's the link to a Amazon's product page that might help to understand the issue better - https://www.amazon.com/dp/B06XCM9LJ4

With the OS being in High Contrast mode and Firefox set to override colors only with High Contrast themes, as we scroll below, we can see dark patches where the images are present. Note that the choice of background color does not affect this at all - whichever color we choose to override the background, the images won't be visible. Toggling Colors to "Never" shows the images.

This sort of 'present-but-invisible' content is pretty common when browsing in High Contrast mode and I feel the need to frequently toggle between the Never and HC modes - hence the extension.
Just my 2 cents: if bug 1393022 was fixed a WebExtension could do its own implementation of changing the page colors, with the possibility of adding extra features beyond what this setting does.  Though adding the ability to change this would certainly make creating a simple extension very easy.
This would fit well in the browserSettings API.
Whiteboard: [design-decision-needed][browserSettings] → [design-decision-approved][browserSettings]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: good-first-bug
If this is your first contribution, please refer to https://wiki.mozilla.org/Add-ons/Contribute/Code on how to get started. 

Mentor: ntim
Mentor: ntim.bugs
Attachment #8959513 - Flags: review?(ntim.bugs)
Attachment #8959513 - Flags: review?(mixedpuppy)
Attachment #8959513 - Flags: review?(bob.silverberg)
I'm at this point torn about this setting.  There is a more advanced version in the wings, see bug 1431473.
Flags: needinfo?(mconca)
See Also: → 1431473
Comment on attachment 8959513 [details]
Bug 1417810 - Added API to override document colors

https://reviewboard.mozilla.org/r/228316/#review234358

Thanks for the patch! This is a great start, I left a few comments that I think should be addressed :)

::: toolkit/components/extensions/ext-browserSettings.js:380
(Diff revision 1)
>            }),
> +        useDocumentColors: Object.assign(
> +          getSettingsAPI(
> +            extension, "useDocumentColors",
> +            () => {
> +              return Services.prefs.getIntPref("browser.display.document_color_use");

We shouldn't return 0, 1 or 2 here, these are internal values.

How about:
"always" for 2
"never" for 1
"high-contrast-only" for 0

::: toolkit/components/extensions/ext-browserSettings.js:384
(Diff revision 1)
> +            set: details => {
> +              if (![0, 1, 2].includes(details.value)) {
> +                throw new ExtensionError(
> +                  `${details.value} is not a valid value for useDocumentColors.`);
> +              }
> +              return ExtensionPreferencesManager.setSetting(
> +                extension.id, "useDocumentColors", details.value);

Same when setting the value, we should use more descriptive values than 0, 1 or 2.

::: toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js:181
(Diff revision 1)
>      "openSearchResultsInNewTabs", true,
>      {"browser.search.openintab": true});
>    await testSetting(
>      "openSearchResultsInNewTabs", false,
>      {"browser.search.openintab": false});
> -
> +  

nit: remove leading whitespace here and below
(In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> I'm at this point torn about this setting.  There is a more advanced version
> in the wings, see bug 1431473.

This bug is simply exposing a feature that has been available since very early versions of Firefox, and available in Firefox's preferences UI. As for bug 1431473, it seems to be exposing an internal WebRender flag that's not used anywhere else in Firefox.

So I would vote for only supporting this bug only, since it also covers the a11y use case in bug 1431473, and exposes a feature that's actively maintained and tested.
Can you combine the 3 patches together ? It's easier to review this way.
I apologize for the confusing commits.
My idea was to submit to the same review - as far as I understood MozReview docs, using the same bug reference should give the same ReviewID but I found that the review IDs were new for every commit.
I am not sure of how to patch them together; is there a page with instructions? Thanks.
Go to the patch in reviewboard you want to update, you'll see something like "MozReview-Commit-ID: LfT56MLlQo9".  Add that to the commit message:

"""
Bug xxx - description

longer description

MozReview-Commit-ID: LfT56MLlQo9
"""
Attachment #8960901 - Flags: review?(psatish) → review-
(In reply to Tim Nguyen :ntim from comment #18)
> Can you combine the 3 patches together ? It's easier to review this way.

I added the Commit-ID but I am not sure I did it right. Can you confirm if you see the review request and the changes consolidated?
You need to combine all 3 commits in one, right now, there are still 3 separate commits.
Attachment #8960889 - Attachment description: Fixed issues reported in review of Bug 1417810 → Fixed issues reported in review of Bug 1417810 MozReview-Commit-ID: LfT56MLlQo9
Attachment #8960901 - Attachment description: Fixed eslint reports - Bug 1417810 → Fixed eslint reports - Bug 1417810 MozReview-Commit-ID: LfT56MLlQo9
Attachment #8960889 - Attachment is obsolete: true
Attachment #8960889 - Flags: review?(ntim.bugs)
Attachment #8960901 - Attachment is obsolete: true
Attachment #8960901 - Flags: review?(ntim.bugs)
Comment on attachment 8959513 [details]
Bug 1417810 - Added API to override document colors

https://reviewboard.mozilla.org/r/228316/#review236698

::: toolkit/components/extensions/ext-browserSettings.js:383
(Diff revision 2)
> +              } else if (prefValue === 2) {
> +                return "always";
> +              }
> +              return "high-contrast-only";
> +            }

Looking more at this, "overrideDocumentColors" would be a more appropriate name.

Since in the "never" case, the document colors are chosen, while in the "always" case, the document colors are overriden.
Attachment #8959513 - Flags: review?(ntim.bugs)
Attachment #8959513 - Flags: review?(mixedpuppy)
(In reply to Tim Nguyen :ntim from comment #26)
> Comment on attachment 8959513 [details]
> Bug 1417810 - Added API to override document colors
> 
> https://reviewboard.mozilla.org/r/228316/#review236698
> 
> ::: toolkit/components/extensions/ext-browserSettings.js:383
> (Diff revision 2)
> > +              } else if (prefValue === 2) {
> > +                return "always";
> > +              }
> > +              return "high-contrast-only";
> > +            }
> 
> Looking more at this, "overrideDocumentColors" would be a more appropriate
> name.

To be clear, this comment applies to "useDocumentColors", not to "high-contrast-only"
Comment on attachment 8959513 [details]
Bug 1417810 - Added API to override document colors

https://reviewboard.mozilla.org/r/228316/#review237638

This is looking fine.  I've kicked off a try from rb, I want to see the android tests pass.  As well, this needs a followup bug to handle the extension setting UI in about:prefs, and we should probably hold off landing this until we know the UI work can land in the same version of firefox.
Attachment #8959513 - Flags: review?(ntim.bugs)
Comment on attachment 8959513 [details]
Bug 1417810 - Added API to override document colors

getting this on my review queue.
Attachment #8959513 - Flags: review?(ntim.bugs)
Attachment #8959513 - Flags: review?(mixedpuppy)
> (In reply to Tim Nguyen :ntim from comment #11)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> > I'm at this point torn about this setting.  There is a more advanced version
> > in the wings, see bug 1431473.
> 
> This bug is simply exposing a feature that has been available since very
> early versions of Firefox, and available in Firefox's preferences UI. As for
> bug 1431473, it seems to be exposing an internal WebRender flag that's not
> used anywhere else in Firefox.
> 
> So I would vote for only supporting this bug only, since it also covers the
> a11y use case in bug 1431473, and exposes a feature that's actively
> maintained and tested.

This browserSetting API exposes a feature of the browser available to users via preferences. In the past, this type of WebExtension API has typically been approved, and that should be the case here as well. I recommend we land this patch.

As for the relation to bug 1431373, it has slightly different use cases, most related to accessibility, but not entirely. The merits of landing it will be discussed in that bug, separate from this one.
Flags: needinfo?(mconca)
The UI updates for preferences are tracked in bug 1449665. If you'd like to work on that Satish I've added some pointers on how to get started in that bug and can help answer any questions you may have.

Thank you for your contribution!
Comment on attachment 8959513 [details]
Bug 1417810 - Added API to override document colors

https://reviewboard.mozilla.org/r/228316/#review237722
Attachment #8959513 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8959513 [details]
Bug 1417810 - Added API to override document colors

https://reviewboard.mozilla.org/r/228316/#review237726

Looks good to me, thanks for contributing! mixedpuppy should give a final pass on this as he is a peer :)

Try looks green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e053ea0e97fa2a728c5e99648db2e6b6c605782
Attachment #8959513 - Flags: review?(ntim.bugs)
(In reply to Tim Nguyen :ntim from comment #34)

> mixedpuppy should give a final pass on this as he is a peer :)

I just did, I'll make rb land it.  Thanks Satish!
Comment on attachment 8959513 [details]
Bug 1417810 - Added API to override document colors

https://reviewboard.mozilla.org/r/228316/#review237728
Attachment #8959513 - Flags: review+
Assignee: nobody → psatish
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 9211a415dff9cfbf8434773a055cd50cccdcfbed -d 02213fd3a9c8: rebasing 454936:9211a415dff9 "Bug 1417810 - Added API to override document colors r=mixedpuppy" (tip)
merging toolkit/components/extensions/ext-browserSettings.js
merging toolkit/components/extensions/schemas/browser_settings.json
merging toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js
warning: conflicts while merging toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Satish, could you please update your patch on top of the latest source code ?

Let us know if you need help doing so.

Thanks!
Flags: needinfo?(psatish)
(In reply to Tim Nguyen :ntim from comment #38)
> Satish, could you please update your patch on top of the latest source code ?
> 
> Let us know if you need help doing so.
> 
> Thanks!

Yes, I think I will need help. I ran the 'hg pull' and 'hg rebase' commands and merged my changes with the latest in kDiff3. However after saving that, 'hg commit' and 'hg rebase --continue' show there's nothing to commit or no rebase in progress.

The file with unresolved conflicts is merged and resolved - how can I push this upstream?

Thanks
Does `hg log` show your commit ? If so, you can just push it to mozreview.
(In reply to Tim Nguyen :ntim from comment #40)
> Does `hg log` show your commit ? If so, you can just push it to mozreview.

No. It still shows the commit from 2 days ago.
Could you share your output of `hg log` ?
(In reply to Tim Nguyen :ntim from comment #42)
> Could you share your output of `hg log` ?

satish@win10pc /d/mozilla/source/mozilla-central
$ hg log toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js

changeset:   410511:dec291fcab76
tag:         tip
user:        Satish <psatish@live.in>
date:        Tue Mar 27 12:01:44 2018 +0530
summary:     Bug 1417810 - Added API to override document colors r?ntim

changeset:   409988:aee255d8336c
user:        Bob Silverberg <bsilverberg@mozilla.com>
date:        Fri Jan 19 12:59:53 2018 -0500
summary:     Bug 1344749 - Expose API to customize where new tabs open, r=dao

changeset:   409904:a27611cba7d7
user:        Hector Zhao <bzhao@mozilla.com>
date:        Tue Mar 06 17:11:49 2018 +0800
summary:     Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension. r=mixedpuppy

changeset:   409696:8b7876354e56
user:        Narcis Beleuzu <nbeleuzu@mozilla.com>
date:        Fri Mar 23 19:45:14 2018 +0200
summary:     Backed out 3 changesets (bug 1435142) for browser-chrome failures on browser_preloadedBrowser_zoom.js

changeset:   409668:7334da12de35
user:        Hector Zhao <bzhao@mozilla.com>
date:        Tue Mar 06 17:11:49 2018 +0800
summary:     Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension. r=mixedpuppy

changeset:   409012:c7d3eda7dedc
user:        Noemi Erli <nerli@mozilla.com>
date:        Tue Mar 20 15:38:04 2018 +0200
summary:     Backed out 3 changesets (bug 1435142) for failing in browser/base/content/test/tabs/browser_close_tab_by_dblclick.js on a CLOSED TREE

changeset:   409005:9193e64cfe29
user:        Hector Zhao <bzhao@mozilla.com>
date:        Tue Mar 06 17:11:49 2018 +0800
summary:     Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension. r=mixedpuppy

changeset:   408806:ea5c6ffc930e
user:        Dorel Luca <dluca@mozilla.com>
date:        Mon Mar 19 17:54:01 2018 +0200
summary:     Backed out 3 changesets (bug 1435142) for Mochitest failure on browser/base/content/test/tabs/browser_close_tab_by_dblclick.js

changeset:   408801:bcde232543f8
user:        Hector Zhao <bzhao@mozilla.com>
date:        Tue Mar 06 17:11:49 2018 +0800
summary:     Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension. r=mixedpuppy

changeset:   406462:e7bfe2b00511
user:        Dão Gottwald <dao@mozilla.com>
date:        Sat Mar 03 15:04:51 2018 +0100
summary:     Backed out changeset 0d138609abfd (bug 1344749)

changeset:   406139:0d138609abfd
user:        Bob Silverberg <bsilverberg@mozilla.com>
date:        Fri Jan 19 12:59:53 2018 -0500
summary:     Bug 1344749 - Expose API to customize where new tabs open, r=dao,mixedpuppy

changeset:   401543:e6a7b5e11ba8
user:        Kris Maglione <maglione.k@gmail.com>
date:        Mon Jan 29 15:20:18 2018 -0800
summary:     Bug 1431533: Part 5a - Auto-rewrite code to use ChromeUtils import methods. r=florian

changeset:   401421:c6e0fe339cb1
user:        Cosmin Sabou <csabou@mozilla.com>
date:        Tue Jan 30 07:17:48 2018 +0200
summary:     Backed out 3 changesets (bug 1431533) for Android mochitest failures on testEventDispatcher on a CLOSED TREE

changeset:   401376:34c999fa006b
user:        Kris Maglione <maglione.k@gmail.com>
date:        Mon Jan 29 15:20:18 2018 -0800
summary:     Bug 1431533: Part 5a - Auto-rewrite code to use ChromeUtils import methods. r=florian

changeset:   401362:505fc52bd608
user:        Brindusan Cristian <cbrindusan@mozilla.com>
date:        Tue Jan 30 02:32:43 2018 +0200
summary:     Backed out 2 changesets (bug 1431533) for ESlint failures on a CLOSED TREE

changeset:   401358:12fc4dee861c
user:        Kris Maglione <maglione.k@gmail.com>
date:        Mon Jan 29 15:20:18 2018 -0800
summary:     Bug 1431533: Part 5a - Auto-rewrite code to use ChromeUtils import methods. r=florian

changeset:   400770:cc87ad81ff86
user:        Kris Maglione <maglione.k@gmail.com>
date:        Wed Jan 24 22:04:59 2018 -0800
summary:     Backed out 3 changesets (bug 1431533) for Android mochitest bustage. CLOSED TREE

changeset:   400767:d4a7c018420e
user:        Kris Maglione <maglione.k@gmail.com>
date:        Wed Jan 24 15:48:47 2018 -0800
summary:     Bug 1431533: Part 5a - Auto-rewrite code to use ChromeUtils import methods. r=florian

changeset:   399804:891afc6d66f1
user:        Bob Silverberg <bsilverberg@mozilla.com>
date:        Thu Jan 11 15:22:03 2018 -0500
summary:     Bug 1425535 - Implement browserSettings.proxyConfig API, r=mixedpuppy

changeset:   397122:f418166c70b8
user:        Florian Quèze <florian@queze.net>
date:        Thu Dec 21 11:10:23 2017 +0100
summary:     Bug 1421992 - script-generated patch to replace do_execute_soon, do_print and do_register_cleanup with executeSoon, info and registerCleanupFunction, rs=Gijs.

changeset:   395878:0f8c30988da2
user:        Kris Maglione <maglione.k@gmail.com>
date:        Tue Nov 28 14:13:59 2017 -0800
summary:     Bug 1421459: Update to ESLint 4 "indent" rule. r=aswan

changeset:   394264:77229a598e9f
user:        Bob Silverberg <bsilverberg@mozilla.com>
date:        Wed Nov 29 09:49:46 2017 -0500
summary:     Bug 1420969 - API to open search results in new tab, r=mixedpuppy

changeset:   394263:5a46e18c726d
user:        Bob Silverberg <bsilverberg@mozilla.com>
date:        Tue Nov 28 09:20:31 2017 -0500
summary:     Bug 1420974 - API to open bookmarks in new tab, r=mixedpuppy

changeset:   393486:d4779e0fdbbb
user:        Bob Silverberg <bsilverberg@mozilla.com>
date:        Tue Nov 21 12:33:15 2017 -0500
summary:     Bug 1419426 - Implement browserSettings.contextMenuShowEvent, r=kmag

changeset:   393369:2da6fb2d843a
user:        Brindusan Cristian <cbrindusan@mozilla.com>
date:        Thu Nov 23 16:19:37 2017 +0200
summary:     Backed out changeset e8d475fed206 (bug 1419426) for opt-test-verify failures on extensions/test/xpcshell/test_ext_browserSettings.js. r=backout a=backout on a CLOSED TREE

changeset:   393356:e8d475fed206
user:        Bob Silverberg <bsilverberg@mozilla.com>
date:        Tue Nov 21 12:33:15 2017 -0500
summary:     Bug 1419426 - Implement browserSettings.contextMenuShowEvent, r=kmag

changeset:   388699:2ac09a374b69
user:        Bob Silverberg <bsilverberg@mozilla.com>
date:        Wed Oct 18 17:46:38 2017 -0400
summary:     Bug 1364942 - Allow WebExtensions to disable Web API notifications, r=mixedpuppy

changeset:   379741:39a2a87fc100
user:        Bob Silverberg <bsilverberg@mozilla.com>
date:        Wed Sep 06 16:46:38 2017 -0400
summary:     Bug 1364972 - Allow WebExtensions to disable animated images, r=mixedpuppy

changeset:   379515:9047e2077ec7
user:        Wes Kocher <wkocher@mozilla.com>
date:        Thu Sep 07 12:31:11 2017 -0700
summary:     Backed out changeset 674775dda8ad (bug 1364972) for eslint failures a=backout

changeset:   379514:674775dda8ad
user:        Bob Silverberg <bsilverberg@mozilla.com>
date:        Wed Sep 06 16:46:38 2017 -0400
summary:     Bug 1364972 - Allow WebExtensions to disable animated images, r=mixedpuppy

changeset:   372334:b89bf45144d8
user:        Bob Silverberg <bsilverberg@mozilla.com>
date:        Mon Jul 31 16:46:36 2017 -0400
summary:     Bug 1339550 - Implement browser.settings.allowPopupsForUserEvents, r=aswan

changeset:   364774:4383e27b5216
user:        Bob Silverberg <bsilverberg@mozilla.com>
date:        Wed May 24 13:17:42 2017 -0400
summary:     Bug 1364936 - Allow WebExtensions to disable the browser cache, r=aswan
That looks fine to me, you should just be able to push what you have to mozreview.
Done. Thanks, looks like it worked!!
Flags: needinfo?(psatish)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/430a1c231025
Added API to override document colors r=mixedpuppy,ntim
Congrats on your first contribution! It should be in Nightly some time tomorrow :)
https://hg.mozilla.org/mozilla-central/rev/430a1c231025
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Yay! Congrats on landing this, Satish! This has been added to our add-ons recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition#March_2018

Would you be interested in creating a profile on mozillians.org? I'd love to vouch for you!
Thank you everyone for the kind words. Tbh, though, this was quite simple. All I did was copy-paste few lines :-)
Keywords: dev-doc-needed
Could you please provide a link or xpi for your extension in order to test it?
Flags: needinfo?(psatish)
Attached a very simple (and crude) extension that uses this new setting to toggle the document colors.
Flags: needinfo?(psatish)
Verified as fixed in Latest Nightly using Windows 10x64 and I will attach a postfix screenshot. Also, I can confirm that the selector is not working in FF 59.02.
Status: RESOLVED → VERIFIED
Attached image Postfix video
Docs -> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserSettings/overrideDocumentColors

Please let me know if this covers it.
Flags: needinfo?(bob.silverberg)
LGTM, thanks Will.
Flags: needinfo?(bob.silverberg)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.