1450114 - Allow theming the selection background and text

Status: VERIFIED FIXED

(WebExtensions :: Themes, defect, P3)

Description

[Tim Nguyen :ntim]

Attachment: selection_text.png

Comment 1

[Manish [:manishkk]]

not getting the starting point. Can you guide me?
Thanks

Comment 2

[Tim Nguyen :ntim]

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.

Comment 4

[shutovby]

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

Comment 5

[Tim Nguyen :ntim]

Manish, I'm freeing this bug for other contributors to work on. Please let me know if you'd like other bug suggestions.

Comment 6

[dsemel]

(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?

Comment 7

[Tim Nguyen :ntim]

(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.

Comment 8

[dsemel]

(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.

Comment 9

[Tim Nguyen :ntim]

(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.

Comment 10

[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.

Comment 11

[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.

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.

Comment 12

[Tim Nguyen :ntim]

(In reply to dsemel from comment #11)
> (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.

Comment 13

[Tim Nguyen :ntim]

Have you done `hg commit -m "..."` yet ? If so, `arc diff` should submit the patch for review.

Comment 14

[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.

Comment 15

[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);
}

Comment 16

[dsemel]

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.

Comment 17

[dsemel]

Attachment: Bug 1450114 - Allow theming the selection background and text

/Users/danielsemel/src/mozilla-unified/toolkit/components/extensions/parent/ext-theme.js

Comment 18

[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.

Okay, hg commit -m and arc diff completed for changed files listed above.

Comment 19

[Tim Nguyen :ntim]

Thanks! I've added some comments on the Phabricator revision.

Comment 20

[dsemel]

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

Corrections made. :)

Comment 21

[Tim Nguyen :ntim]

(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

Comment 22

[dsemel]

(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.

Comment 23

[dsemel]

(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.

Comment 24

[Tim Nguyen :ntim]

(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.

Comment 25

[dsemel]

(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();
});

Comment 26

[Tim Nguyen :ntim]

(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.

Comment 27

[Tim Nguyen :ntim]

Here's some documentation about CSS selectors: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Selectors

Comment 28

[dsemel]

(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)

Comment 29

[Tim Nguyen :ntim]

Did you add your test to toolkit/components/extensions/test/browser/browser.ini ?

Comment 30

[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.

Comment 31

[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?

Comment 32

[dsemel]

(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?

Comment 33

[dsemel]

(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)

Comment 34

[dsemel]

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

Comment 35

[Tim Nguyen :ntim]

Hi gps, any idea what’s going on ?

Comment 36

[Gregory Szorc [: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.

Comment 37

[dsemel]

(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?

Comment 38

[Tim Nguyen :ntim]

Hi dsemel, did :gps' command solve the mochitest problem?

Comment 39

[Caitlin Neiman [:caitmuenster]]

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.

Comment 40

[edward.i.wu]

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

Comment 41

[Jared Wein [:jaws] (please needinfo? me)]

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).

Comment 42

[edward.i.wu]

great, thanks Jared!

Comment 43

[edward.i.wu]

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.

Comment 44

[edward.i.wu]

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

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

Comment 45

[edward.i.wu]

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

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

Comment 46

[edward.i.wu]

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.

Comment 47

[edward.i.wu]

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?

Comment 48

[Tim Nguyen :ntim]

(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

Comment 49

[edward.i.wu]

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

Comment 50

[Tim Nguyen :ntim]

I've pushed this to the try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b66a3b2f7b4a3b42ec11a6fc9b9f4d7e2b128bcf

Comment 51

[Tim Nguyen :ntim]

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 ?

Comment 52

[Dão Gottwald [:dao]]

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.

Comment 53

[Tim Nguyen :ntim]

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.

Comment 54

[Mike Conca [:mconca]]

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.

Comment 55

[Tim Nguyen :ntim]

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.

Comment 56

[edward.i.wu]

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

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b328b5fa1cf
Update browser themes to allow customization of selection background and text r=jaws

Comment 58

[Razvan Maries]

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

Comment 59

[Tim Nguyen :ntim]

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

Comment 60

[Madalin Cotetiu]

Attachment: 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.

Comment 61

[Eric Shepherd [:sheppy]]

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

Comment 62

[Irene Smith]

Added the new manifest settings to the release notes.