Closed Bug 1450114 Opened 6 years ago Closed 5 years ago

Allow theming the selection background and text

Categories

(WebExtensions :: Themes, defect, P3)

Product:
WebExtensions
Component:
Themes
Type:
defect

Tracking

(firefox67 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- verified

People

(Reporter: ntim, Assigned: edward.i.wu, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, good-first-bug)

Attachments

(3 files, 2 obsolete files)

selection_text.png
31.36 KB, image/png
Details
Bug 1450114 -Update browser themes to allow customization of selection background and text r=Jaws
47 bytes, text/x-phabricator-request
Details | Review
manifest.json
390 bytes, application/json
Details
Attached image selection_text.png — 
      No description provided.
Priority: -- → P3
not getting the starting point. Can you guide me?
Thanks
Flags: needinfo?(ntim.bugs)
Assignee: nobody → 1991manish.kumar
This bug is about allowing WebExtension themes to style the selected text color and background color.

In case you're not familiar with WebExtension themes, here are some docs: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/theme

You don't need a strong knowledge of the document, you simply need to know what WebExtension themes are, and how they can be used.

Here are some steps to get started:
1) Implement the CSS for text selection: in browser/themes/shared/browser.inc.css

:root:-moz-lwtheme::-moz-selection {
  background-color: var(--lwt-selection-background, Highlight);
  color: var(--lwt-selection-text-color, HighlightText);
}


2) Implement the WebExtension properties: "selection" and "selection_text"

You can look at the patch in bug 1434476, which implements "tab_selected". Based on the same model, you can try to implement "selection" and "selection_text".

3) Add automated test

Let me know if you need help with that part, but it should be pretty similar to the test in bug 1434476.

You can run a test using the `./mach test path/to/test` command, to check if your test works properly.


Let me know if you have questions.
Flags: needinfo?(ntim.bugs)
The add-ons contest reached voting stage, but this issue is not fixed. Selection text is unreadable on dark themes.
Mac https://bug1454971.bmoattachments.org/attachment.cgi?id=8969129
Windows https://bug1454971.bmoattachments.org/attachment.cgi?id=8968907
Product: Toolkit → WebExtensions
Manish, I'm freeing this bug for other contributors to work on. Please let me know if you'd like other bug suggestions.
Assignee: 1991manish.kumar → nobody
Mentor: ntim.bugs
Keywords: good-first-bug
(In reply to shutovby from comment #4)
> The add-ons contest reached voting stage, but this issue is not fixed.
> Selection text is unreadable on dark themes.
> Mac https://bug1454971.bmoattachments.org/attachment.cgi?id=8969129
> Windows https://bug1454971.bmoattachments.org/attachment.cgi?id=8968907

Could this bug be fixed may making text turn black when highlighted?
(In reply to dsemel from comment #6)
> (In reply to shutovby from comment #4)
> > The add-ons contest reached voting stage, but this issue is not fixed.
> > Selection text is unreadable on dark themes.
> > Mac https://bug1454971.bmoattachments.org/attachment.cgi?id=8969129
> > Windows https://bug1454971.bmoattachments.org/attachment.cgi?id=8968907
> 
> Could this bug be fixed may making text turn black when highlighted?

This bug makes the selection color customizable so themes can change it themselves.
(In reply to Tim Nguyen :ntim from comment #7)
> (In reply to dsemel from comment #6)
> > (In reply to shutovby from comment #4)
> > > The add-ons contest reached voting stage, but this issue is not fixed.
> > > Selection text is unreadable on dark themes.
> > > Mac https://bug1454971.bmoattachments.org/attachment.cgi?id=8969129
> > > Windows https://bug1454971.bmoattachments.org/attachment.cgi?id=8968907
> > 
> > Could this bug be fixed may making text turn black when highlighted?
> 
> This bug makes the selection color customizable so themes can change it
> themselves.

I've been reviewing bug 1434476 and I would like to make an attempt at fixing this bug. This would be my first time contributing. I have completed the firefox build on my laptop.
(In reply to dsemel from comment #8)
> (In reply to Tim Nguyen :ntim from comment #7)
> > (In reply to dsemel from comment #6)
> > > (In reply to shutovby from comment #4)
> > > > The add-ons contest reached voting stage, but this issue is not fixed.
> > > > Selection text is unreadable on dark themes.
> > > > Mac https://bug1454971.bmoattachments.org/attachment.cgi?id=8969129
> > > > Windows https://bug1454971.bmoattachments.org/attachment.cgi?id=8968907
> > > 
> > > Could this bug be fixed may making text turn black when highlighted?
> > 
> > This bug makes the selection color customizable so themes can change it
> > themselves.
> 
> I've been reviewing bug 1434476 and I would like to make an attempt at
> fixing this bug. This would be my first time contributing. I have completed
> the firefox build on my laptop.


Sure, assigned the bug to you, please let me know if you have any questions.
Assignee: nobody → dsemel
(In reply to Tim Nguyen :ntim from comment #9)
> (In reply to dsemel from comment #8)
> > (In reply to Tim Nguyen :ntim from comment #7)
> > > (In reply to dsemel from comment #6)
> > > > (In reply to shutovby from comment #4)
> > > > > The add-ons contest reached voting stage, but this issue is not fixed.
> > > > > Selection text is unreadable on dark themes.
> > > > > Mac https://bug1454971.bmoattachments.org/attachment.cgi?id=8969129
> > > > > Windows https://bug1454971.bmoattachments.org/attachment.cgi?id=8968907
> > > > 
> > > > Could this bug be fixed may making text turn black when highlighted?
> > > 
> > > This bug makes the selection color customizable so themes can change it
> > > themselves.
> > 
> > I've been reviewing bug 1434476 and I would like to make an attempt at
> > fixing this bug. This would be my first time contributing. I have completed
> > the firefox build on my laptop.
> 
> 
> Sure, assigned the bug to you, please let me know if you have any questions.

in 'toolkit/componenets/extensions/schemas/theme.json', I added 'selection_text' and 'selection' properties.

in 'toolkit/componenets/extensions/parent/ext-theme.js' in 'load colors' added case statement for 'selection_text' and 'selection'.

in 'toolkit/modules/LightWeightThemeConsumer.jsm' added to 'consttoolkitVariableMap' lwtProperty for 'selected-text' and lwtProperty for 'selection'.

in browser/themes/shared/browser.inc.css, I added:

:root:-moz-lwtheme::-moz-selection {
  background-color: var(--lwt-selection-background, Highlight);
  color: var(--lwt-selection-text-color, HighlightText);
}


I also ran an automated test `./mach test  toolkit/components/extensions/test/browser/` and result came back ok.

I will begin my attempt and submitting a patch.
(In reply to Tim Nguyen :ntim from comment #9)
> (In reply to dsemel from comment #8)
> > (In reply to Tim Nguyen :ntim from comment #7)
> > > (In reply to dsemel from comment #6)
> > > > (In reply to shutovby from comment #4)
> > > > > The add-ons contest reached voting stage, but this issue is not fixed.
> > > > > Selection text is unreadable on dark themes.
> > > > > Mac https://bug1454971.bmoattachments.org/attachment.cgi?id=8969129
> > > > > Windows https://bug1454971.bmoattachments.org/attachment.cgi?id=8968907
> > > > 
> > > > Could this bug be fixed may making text turn black when highlighted?
> > > 
> > > This bug makes the selection color customizable so themes can change it
> > > themselves.
> > 
> > I've been reviewing bug 1434476 and I would like to make an attempt at
> > fixing this bug. This would be my first time contributing. I have completed
> > the firefox build on my laptop.
> 
> 
> Sure, assigned the bug to you, please let me know if you have any questions.

Hi, I'm trying to upload my files to submit for review and I'm having trouble. I installed arcanist and I'm having trouble finding clear instructions to commit my changes. I have a Mac.
Have you done `hg commit -m "..."` yet ? If so, `arc diff` should submit the patch for review.
Flags: needinfo?(ntim.bugs) → needinfo?(dsemel)
(In reply to Tim Nguyen :ntim from comment #13)
> Have you done `hg commit -m "..."` yet ? If so, `arc diff` should submit the
> patch for review.

Arcanist no longer seems to be working on my terminal. 'arc' is not recognized as a command any more. It was recognized a few hours ago.

I did an hg commit -m for the theme.json file.
Flags: needinfo?(dsemel)
(In reply to Tim Nguyen :ntim from comment #13)
> Have you done `hg commit -m "..."` yet ? If so, `arc diff` should submit the
> patch for review.


I have done hg commit -m for the following files:

in 'toolkit/componenets/extensions/schemas/theme.json', I added 'selection_text' and 'selection' properties.

in 'toolkit/componenets/extensions/parent/ext-theme.js' in 'load colors' added case statement for 'selection_text' and 'selection'.

in 'toolkit/modules/LightWeightThemeConsumer.jsm' added to 'consttoolkitVariableMap' lwtProperty for 'selected-text' and lwtProperty for 'selection'.

in browser/themes/shared/browser.inc.css, I added:

:root:-moz-lwtheme::-moz-selection {
  background-color: var(--lwt-selection-background, Highlight);
  color: var(--lwt-selection-text-color, HighlightText);
}
a(In reply to Tim Nguyen :ntim from comment #13)
> Have you done `hg commit -m "..."` yet ? If so, `arc diff` should submit the
> patch for review.

Arcanist was installed a second time. Now working.
/Users/danielsemel/src/mozilla-unified/toolkit/components/extensions/parent/ext-theme.js
(In reply to Tim Nguyen :ntim from comment #13)
> Have you done `hg commit -m "..."` yet ? If so, `arc diff` should submit the
> patch for review.

Okay, hg commit -m and arc diff completed for changed files listed above.
Attachment #9005792 - Attachment description: /Users/danielsemel/src/mozilla-unified/toolkit/components/extensions/schemas/theme.json → Bug 1450114 - Allow theming the selection background and text
Thanks! I've added some comments on the Phabricator revision.
(In reply to Tim Nguyen :ntim from comment #19)
> Thanks! I've added some comments on the Phabricator revision.

Corrections made. :)
(In reply to dsemel from comment #20)
> (In reply to Tim Nguyen :ntim from comment #19)
> > Thanks! I've added some comments on the Phabricator revision.
> 
> Corrections made. :)

Reviewed again, guetting closer, thanks! Btw, do you have time to work on adding a new automated test ? Totally fine if you don't, but here's some guidance if you do:

You can get inspired from: https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/browser/browser_ext_themes_tab_line.js

You can call it something like browser_ext_themes_selection.js.

To get the computed style of the selection, you can use getComputedStyle(elementName, "::selection");

Then your new test needs to be added in  toolkit/components/extensions/test/browser/browser.ini.


Let me know if you have any questions
(In reply to Tim Nguyen :ntim from comment #21)
> (In reply to dsemel from comment #20)
> > (In reply to Tim Nguyen :ntim from comment #19)
> > > Thanks! I've added some comments on the Phabricator revision.
> > 
> > Corrections made. :)
> 
> Reviewed again, guetting closer, thanks! Btw, do you have time to work on
> adding a new automated test ? Totally fine if you don't, but here's some
> guidance if you do:
> 
> You can get inspired from:
> https://searchfox.org/mozilla-central/source/toolkit/components/extensions/
> test/browser/browser_ext_themes_tab_line.js
> 
> You can call it something like browser_ext_themes_selection.js.
> 
> To get the computed style of the selection, you can use
> getComputedStyle(elementName, "::selection");
> 
> Then your new test needs to be added in 
> toolkit/components/extensions/test/browser/browser.ini.
> 
> 
> Let me know if you have any questions

I restored the space before the hashtag in the browser.css file. I did an hg commit -m and arc diff and now the file no longer appears on phabricator.
(In reply to Tim Nguyen :ntim from comment #21)
> (In reply to dsemel from comment #20)
> > (In reply to Tim Nguyen :ntim from comment #19)
> > > Thanks! I've added some comments on the Phabricator revision.
> > 
> > Corrections made. :)
> 
> Reviewed again, guetting closer, thanks! Btw, do you have time to work on
> adding a new automated test ? Totally fine if you don't, but here's some
> guidance if you do:
> 
> You can get inspired from:
> https://searchfox.org/mozilla-central/source/toolkit/components/extensions/
> test/browser/browser_ext_themes_tab_line.js
> 
> You can call it something like browser_ext_themes_selection.js.
> 
> To get the computed style of the selection, you can use
> getComputedStyle(elementName, "::selection");
> 
> Then your new test needs to be added in 
> toolkit/components/extensions/test/browser/browser.ini.
> 
> 
> Let me know if you have any questions

I will work on adding the automated test. I tried to correct spacing and indentations and submitted new commits, I'm using Sublime Text which helps with formatting, but I have trouble seeing the differences in spacing.
(In reply to dsemel from comment #23)

> I will work on adding the automated test. I tried to correct spacing and
> indentations and submitted new commits, I'm using Sublime Text which helps
> with formatting, but I have trouble seeing the differences in spacing.

To check formatting, I recommend looking at the changes on Phabricator, which should be enough to check if the formatting is correct.

You can also use ./mach lint toolkit/components/extensions --fix for JavaScript formatting.

I'll do a full review once the test is added, please let me know if you have any questions about how it works.
(In reply to Tim Nguyen :ntim from comment #21)
> (In reply to dsemel from comment #20)
> > (In reply to Tim Nguyen :ntim from comment #19)
> > > Thanks! I've added some comments on the Phabricator revision.
> > 
> > Corrections made. :)
> 
> Reviewed again, guetting closer, thanks! Btw, do you have time to work on
> adding a new automated test ? Totally fine if you don't, but here's some
> guidance if you do:
> 
> You can get inspired from:
> https://searchfox.org/mozilla-central/source/toolkit/components/extensions/
> test/browser/browser_ext_themes_tab_line.js
> 
> You can call it something like browser_ext_themes_selection.js.
> 
> To get the computed style of the selection, you can use
> getComputedStyle(elementName, "::selection");
> 
> Then your new test needs to be added in 
> toolkit/components/extensions/test/browser/browser.ini.
> 
> 
> Let me know if you have any questions

Hi Tim,

I have some questions about writing the test. Should the document.querySelector get object names from the LightWeightThemeConsumer file? Anyway, the code I've cobbled together is below:

    add_task(async function test_theme_text_background_selection(elementName) {
  
  let extension = ExtensionTestUtils.loadExtension({
    manifest: {
      "theme": {
        "colors": {
          "selection": SELECTION_COLOR,
          "selection_text": SELECTION_TEXT_COLOR,
          
        },
      },
    },
  });

  await extension.startup();

  info("Checking selection tab color and text color");
  let highlight = document.querySelector("selection");
  let line = document.getAnonymousElementByAttribute(highlight, "selection", "selection-text");
  Assert.equal(window.getComputedStyle(elementName, "::selection").backgroundColor,
               `rgb(${hexToRGB(SELECTION_COLOR).join(", ")})`,
               "Selection should have theme color");

  await extension.unload();
});
(In reply to dsemel from comment #25)
> (In reply to Tim Nguyen :ntim from comment #21)
> > (In reply to dsemel from comment #20)
> > > (In reply to Tim Nguyen :ntim from comment #19)
> > > > Thanks! I've added some comments on the Phabricator revision.
> > > 
> > > Corrections made. :)
> > 
> > Reviewed again, guetting closer, thanks! Btw, do you have time to work on
> > adding a new automated test ? Totally fine if you don't, but here's some
> > guidance if you do:
> > 
> > You can get inspired from:
> > https://searchfox.org/mozilla-central/source/toolkit/components/extensions/
> > test/browser/browser_ext_themes_tab_line.js
> > 
> > You can call it something like browser_ext_themes_selection.js.
> > 
> > To get the computed style of the selection, you can use
> > getComputedStyle(elementName, "::selection");
> > 
> > Then your new test needs to be added in 
> > toolkit/components/extensions/test/browser/browser.ini.
> > 
> > 
> > Let me know if you have any questions
> 
> Hi Tim,
> 
> I have some questions about writing the test. Should the
> document.querySelector get object names from the LightWeightThemeConsumer
> file? 

querySelector takes in a CSS selector that represents a HTML/XUL element. You can find the selectors by using the Browser Toolbox: https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox


In your case, your feature should set the urlbar selection, so you can do:

let urlBar = document.querySelector("#urlbar");
let input = document.getAnonymousElementByAttribute(urlBar, "anonid", "input");

and then check for the input's computed style.
Here's some documentation about CSS selectors: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Selectors
(In reply to Tim Nguyen :ntim from comment #27)
> Here's some documentation about CSS selectors:
> https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Selectors

Hi Tim, 

I ran the test and received this error UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 0: ordinal not in range(128))

   Below is my code for the automated test browser_ext_themes_selection.js, :
    
    add_task(async function test_theme_text_background_selection(elementName) {
  
     let extension = ExtensionTestUtils.loadExtension({
      manifest: {
      "theme": {
        "colors": {
          "selection": SELECTION_COLOR,
          "selection_text": SELECTION_TEXT_COLOR,
          
          },
        },
      },
    });

      await extension.startup();

      info("Checking selection tab color and text color");
      let urlBar = document.querySelector("#urlbar");
      let input = document.getAnonymousElementByAttribute(urlBar, "anonid", "input");
      Assert.equal(window.getComputedStyle(elementName, "::selection").backgroundColor,
               `rgb(${hexToRGB(SELECTION_COLOR).join(", ")})`,
               "Selection should have theme color");

      await extension.unload();
      });


   

   I get the following output:   

              Traceback (most recent call last):
  File "./mach", line 86, in <module>
    main(sys.argv[1:])
  File "./mach", line 78, in main
    mach = get_mach()
  File "./mach", line 68, in get_mach
    mach = check_and_get_mach(dir_path)
  File "./mach", line 42, in check_and_get_mach
    return load_mach(dir_path, mach_path)
  File "./mach", line 30, in load_mach
    return mach_bootstrap.bootstrap(dir_path)
  File "/Users/danielsemel/src/mozilla-unified/build/mach_bootstrap.py", line 331, in bootstrap
    repo = resolve_repository()
  File "/Users/danielsemel/src/mozilla-unified/build/mach_bootstrap.py", line 186, in resolve_repository
    return mozversioncontrol.get_repository_object(path=mozilla_dir)
  File "/Users/danielsemel/src/mozilla-unified/python/mozversioncontrol/mozversioncontrol/__init__.py", line 458, in get_repository_object
    return HgRepository(path, hg=hg)
  File "/Users/danielsemel/src/mozilla-unified/python/mozversioncontrol/mozversioncontrol/__init__.py", line 212, in __init__
    super(HgRepository, self).__init__(path, tool=hg)
  File "/Users/danielsemel/src/mozilla-unified/python/mozversioncontrol/mozversioncontrol/__init__.py", line 73, in __init__
    self._tool = get_tool_path(tool)
  File "/Users/danielsemel/src/mozilla-unified/python/mozversioncontrol/mozversioncontrol/__init__.py", line 51, in get_tool_path
    path = find_executable(tool)
  File "/usr/local/Cellar/python@2/2.7.15_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/distutils/spawn.py", line 220, in find_executable
    f = os.path.join(p, executable)
  File "/usr/local/Cellar/python@2/2.7.15_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/posixpath.py", line 73, in join
    path += '/' + b
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 0: ordinal not in range(128)
Did you add your test to toolkit/components/extensions/test/browser/browser.ini ?
Flags: needinfo?(dsemel)
(In reply to Tim Nguyen :ntim from comment #29)
> Did you add your test to
> toolkit/components/extensions/test/browser/browser.ini ?

No, I did not. I will do that now.
Flags: needinfo?(dsemel)
(In reply to Tim Nguyen :ntim from comment #29)
> Did you add your test to
> toolkit/components/extensions/test/browser/browser.ini ?

I also ran on the terminal:

'./mach test /Users/danielsemel/src/mozilla-unified/toolkit/components/extensions/test/browser/browser_ext_themes_selection.js'

Is that the correct ray to run the test?
(In reply to Tim Nguyen :ntim from comment #29)
> Did you add your test to
> toolkit/components/extensions/test/browser/browser.ini ?

I found this info about the ascii bug https://bugzilla.mozilla.org/show_bug.cgi?format=default&id=1401718

And here is the patch: https://hg.mozilla.org/mozilla-central/rev/5870f25b4a83

What do you think?
(In reply to dsemel from comment #30)
> (In reply to Tim Nguyen :ntim from comment #29)
> > Did you add your test to
> > toolkit/components/extensions/test/browser/browser.ini ?
> 
> No, I did not. I will do that now.

I added the test to browser.ini and I am still getting the same errors. I also ran other tests in the test/browser file and I get the same errors.


Traceback (most recent call last):
  File "./mach", line 86, in <module>
    main(sys.argv[1:])
  File "./mach", line 78, in main
    mach = get_mach()
  File "./mach", line 68, in get_mach
    mach = check_and_get_mach(dir_path)
  File "./mach", line 42, in check_and_get_mach
    return load_mach(dir_path, mach_path)
  File "./mach", line 30, in load_mach
    return mach_bootstrap.bootstrap(dir_path)
  File "/Users/danielsemel/src/mozilla-unified/build/mach_bootstrap.py", line 331, in bootstrap
    repo = resolve_repository()
  File "/Users/danielsemel/src/mozilla-unified/build/mach_bootstrap.py", line 186, in resolve_repository
    return mozversioncontrol.get_repository_object(path=mozilla_dir)
  File "/Users/danielsemel/src/mozilla-unified/python/mozversioncontrol/mozversioncontrol/__init__.py", line 458, in get_repository_object
    return HgRepository(path, hg=hg)
  File "/Users/danielsemel/src/mozilla-unified/python/mozversioncontrol/mozversioncontrol/__init__.py", line 212, in __init__
    super(HgRepository, self).__init__(path, tool=hg)
  File "/Users/danielsemel/src/mozilla-unified/python/mozversioncontrol/mozversioncontrol/__init__.py", line 73, in __init__
    self._tool = get_tool_path(tool)
  File "/Users/danielsemel/src/mozilla-unified/python/mozversioncontrol/mozversioncontrol/__init__.py", line 51, in get_tool_path
    path = find_executable(tool)
  File "/usr/local/Cellar/python@2/2.7.15_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/distutils/spawn.py", line 220, in find_executable
    f = os.path.join(p, executable)
  File "/usr/local/Cellar/python@2/2.7.15_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/posixpath.py", line 73, in join
    path += '/' + b
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe2 in position 0: ordinal not in range(128)
I opened the files for mozversioncontrol/_init_.py and the mach_bootstrap.py to see if they had code to import unicode literals. They did have code importing unicode. I closed the files and ran the test again. Now I have the errors below.

New errors. When I run the test './mach test /Users/danielsemel/src/mozilla-unified/toolkit/components/ex̧tensions/test/browser/'


I now get these errors: 

Traceback (most recent call last):
  File "./mach", line 86, in <module>
    main(sys.argv[1:])
  File "./mach", line 78, in main
    mach = get_mach()
  File "./mach", line 68, in get_mach
    mach = check_and_get_mach(dir_path)
  File "./mach", line 42, in check_and_get_mach
    return load_mach(dir_path, mach_path)
  File "./mach", line 30, in load_mach
    return mach_bootstrap.bootstrap(dir_path)
  File "/Users/danielsemel/src/mozilla-unified/build/mach_bootstrap.py", line 331, in bootstrap
    repo = resolve_repository()
  File "/Users/danielsemel/src/mozilla-unified/build/mach_bootstrap.py", line 180, in resolve_repository
    import mozversioncontrol
  File "/Users/danielsemel/src/mozilla-unified/build/mach_bootstrap.py", line 366, in __call__
    module = self._original_import(name, globals, locals, fromlist, level)
ImportError: No module named mozversioncontrol
Flags: needinfo?(ntim.bugs)
Hi gps, any idea what’s going on ?
Flags: needinfo?(ntim.bugs) → needinfo?(gps)
Uhh, that error doesn't make much sense!

The traceback in comment 33 shows that mozversioncontrol did import properly at one time. Why it suddenly stopped working in comment 34, I don't know!

Did you modify the source for mozversioncontrol by any chance?

Perhaps the .pyc files are corrupted or something? You can try running `mach clobber python` to nuke .pyc files from your source directory. But with the traceback in comment 34, `mach` may be completely busted! You would have to run something like `hg purge --all -I 'glob:**.pyc'` for a similar effect.
Flags: needinfo?(gps)
(In reply to Gregory Szorc [:gps] from comment #36)
> Uhh, that error doesn't make much sense!
> 
> The traceback in comment 33 shows that mozversioncontrol did import properly
> at one time. Why it suddenly stopped working in comment 34, I don't know!
> 
> Did you modify the source for mozversioncontrol by any chance?
> 
> Perhaps the .pyc files are corrupted or something? You can try running `mach
> clobber python` to nuke .pyc files from your source directory. But with the
> traceback in comment 34, `mach` may be completely busted! You would have to
> run something like `hg purge --all -I 'glob:**.pyc'` for a similar effect.

If I did run `hg purge --all -I 'glob:**.pyc , what would be the next step after that?

Try and run the automated test I wrote before?
Flags: needinfo?(gps)
Hi dsemel, did :gps' command solve the mochitest problem?
Flags: needinfo?(dsemel)
I checked in with dsemel yesterday; he's going to step away from this bug. 

For other contributors who would like to pick this up, please see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp for information on how to get started.
Assignee: dsemel → nobody
Flags: needinfo?(gps)
Flags: needinfo?(dsemel)
Mentor: ntim.bugs
Hi Cait, 

Could I give this a try? I'm finishing a patch on another webextensions bug involving errors with background persistence = true, and think it'd be interesting to learn more about the extensions code base
Flags: needinfo?(cneiman)
edward.i.wu, yes you can go ahead and work on this bug. I'll assign it to you when you attach a patch. Let me know if you have any questions (please mark the "need information from mentor" at the bottom when asking a question).
Mentor: jaws
Flags: needinfo?(cneiman)
great, thanks Jared!

I think I've set up everything accord to the above but the .backgroundColor and .color properties window.getComputedElementByStyle(input), where input is:

let urlBar = document.querySelector("#urlbar");
let input = document.getAnonymousElementByAttribute(urlBar, "anonid", "input");

gives me rgba(0,0,0,0) for background and rgb(0,0,0)for color. Is the urlbar the right element? I thought

:root[lwt-selection] |::selection

finds an element with attribute 'lwt-selection' but I could not find any elements with lwt-selection using the Browser Toolbox.

Flags: needinfo?(jaws)

Bug 1450114 -Update browser themes to allow customization of selection background and text r=Jaws

Attachment #9037815 - Attachment is obsolete: true

Bug 1450114 -Update browser themes to allow customization of selection background and text r=Jaws

Hi Jared, sorry that my last comment was unclear, but the issue turned out to be I was checking the element color, not its ::selection color.

I've made the patch on phabricator, let me know what you think.

Assignee: nobody → edward.i.wu
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)

I tried to add another test for the search bar in the latest patch, but how can I make it active for the browser tests? Is it a manifest option?

Flags: needinfo?(jaws)

(In reply to edward.i.wu from comment #47)

I tried to add another test for the search bar in the latest patch, but how can I make it active for the browser tests? Is it a manifest option?

Add this at the beginning of your test:

ChromeUtils.import("resource://testing-common/CustomizableUITestUtils.jsm", this);
let gCUITestUtils = new CustomizableUITestUtils(window);
add_task(async function setup() {
  await gCUITestUtils.addSearchBar();
  registerCleanupFunction(() => {
    gCUITestUtils.removeSearchBar();
  });
});

and then you can loop through the elements like this:

https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/browser/browser_ext_themes_toolbar_fields.js#52-72

Thanks Tim, I've added that code to the test

Attachment #9005792 - Attachment is obsolete: true

Prior to landing this, I'd like to checkpoint on the property names. This bug introduces the "colors.toolbar_field_selection" property (styles the urlbar/searchbar text selection background color) and the "colors.toolbar_field_selection_color" property (styles the urlbar/searchbar text selection text color).

The convention we've been using has been using the "_text" suffix for text color properties, and no suffix for the background color properties. But here "text" is confusing since both properties affect the text selection. The color suffix sort of solves this, but IMO is a bit redundant since it's already in the "colors" group.

Dão or Mike, what do you think ?

Flags: needinfo?(mconca)
Flags: needinfo?(dao+bmo)

I think we should wontfix this, actually. This bug started with the premise that the default selection color lacks contrast, but that was just a bug that has since been fixed. I see no meaningful demand here.

Flags: needinfo?(dao+bmo)

I'll leave that decision to Mike, but I think there is demand for this for two reasons: the Windows/Linux default selection color is quite bright on dark themes for intentional reasons. The second one is that someone may just want the text selection background to match their theme's accent color.

Flags: needinfo?(jaws)

I'd like to see this feature land.

For naming, we use "_text", "_highlight" and "_highlight_text" elsewhere and I think it would make sense to continue the pattern here. I would propose:

  • toolbar_field_text - color of normal text in urlbar (this already exists)
  • toolbar_field_highlight - background color of selected text in urlbar (new in this patch)
  • toolbar_field_highlight_text - foreground color of selected text in urlbar (new in this patch)

This follows the pattern established for popup and sidebar.

Flags: needinfo?(mconca)

Mike, thanks for your input!

Edward, could you please rename the properties as Mike suggested ? The patch should be pretty good to go with that done.

Flags: needinfo?(edward.i.wu)

Sure, I've updated the patch with the name changes

Flags: needinfo?(edward.i.wu)
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3b328b5fa1cf
Update browser themes to allow customization of selection background and text r=jaws

https://hg.mozilla.org/mozilla-central/rev/3b328b5fa1cf

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

This needs screenshots and browser-compat-data to be updated.

Keywords: dev-doc-needed
Attached file manifest.json —

Verified in Win7x64, Ubuntu 14.0.4x32, MacOS 10.14.1 on FF67.0b5 (20190325125126)

I have created a test theme using the following proprieties (see the attachement):

  • "toolbar_field_highlight_text": "#2bc7d8",
  • "toolbar_field_highlight": "#ef0b9f"

and the issue is no longer reproducing. Marking bug as verified fixed.

Status: RESOLVED → VERIFIED
status-firefox67: fixed → verified

Documentation has been updated for this.

  • Updated the manifest.json page to include example and screenshots for the new toolbar_field_highlight and toolbar_field_heighlight_text color settings
  • Added the toolbar highlight color properties to BCD in PR 4139
Keywords: dev-doc-needed → dev-doc-complete

Added the new manifest settings to the release notes.

You need to log in before you can comment on or make changes to this bug.