multi-account-containers

multi-account-containers copied to clipboard

Part 1 actually fixes the bug as filed. Firefox only lets Mozilla-signed extensions use context-fill. Add-ons declared they are created by Mozilla by appending @mozilla.org to their ID.

The rest of the patch cleans up some of the icons, uses context-fill more consistently, and removes icons that are no longer needed.

I updated the patch to not set the ID. I tried it with Bug 1710917 applied and with extensions.experiments.enabled set to true and it works!

I updated the patch to not set the ID. I tried it with Bug 1710917 applied and with extensions.experiments.enabled set to true and it works!

@htwyford great! I was going to comment here, but I went on PTO soon after pushing the patch to autoland.

Feel free to remove the draft flag from this PR and ask one of the maintainers of this project a final review on it, thanks again for your patience!

@groovecoder @maxxcrawford could one of you please take a look? Also can you confirm that this extension is Mozilla-signed?

@groovecoder @maxxcrawford could one of you please take a look? Also can you confirm that this extension is Mozilla-signed?

@htwyford Mozilla-signed should not be required once submitted on AMO, in Firefox >= 91 (where Bug 1710917) multi-account-containers should be allowed as is because of its "line extension" badge, see the "By Firefox" badge in the AMO listing)

Ah, that reminds me that this patch doesn't work nicely with Firefox <91, because I removed the light/dark icons in favour of context-filled ones. I'm so used to the Firefox trains model, I forgot about supporting lower versions :) I'll fix it.

Hey there. I offered to review some PRs for the maintainers.

I tested this in FF 91 and also in 90, and I love it. It appears to work as usual (as expected) in 90. In 91 I noticed that it didn't work with Alpenglow, and so I tried it with a bunch of different themes and with Firefox Colors, and it seems to work with some of them, but not all. I'm not sure why. Do you have any ideas? Here is a screen recording of the icons in FF 91.

Btw, I spent like a whole day a while ago trying to figure out how to context fill the icons to match the theme, only to find out I couldn't! So, cool, thanks!

It looks like it only works on the themes that use the dark icon (i.e. the ones that use the 'light' classification).

The following CSS rule in browser/skin/urlbar-searchbar.css (line 359) is what prevents the pageAction icon to inherit the right color with certain themes:

:root[lwt-toolbar-field-brighttext] #urlbar:not([focused="true"])
.urlbar-addon-page-action[style*=".svg"] > .urlbar-icon,
:root[lwt-toolbar-field-focus-brighttext] #urlbar[focused="true"]
.urlbar-addon-page-action[style*=".svg"] > .urlbar-icon {
    filter: grayscale(100%) brightness(20%) invert();
}

I've tested with a few different themes and haven't found any bad side effect in removing this rule. I'll try to find the right person to fix/review this upstream and hopefully we could get this PR merged.

I've tested with a few different themes and haven't found any bad side effect in removing this rule. I'll try to find the right person to fix/review this upstream and hopefully we could get this PR merged.

Seems like we can't remove the rule because it's a workaround to make sure pageAction icons from third-party addons are following the colorscheme of the default themes that use monochrome colors.

After discussing it with a few folks working on the front-end, the solution would be to add an exception (e.g. :not(#pageAction-urlbar-_testpilot-containers)) to the rule in the Firefox codebase. Next week, I'll try to push a patch upstream for this.

@htwyford would you be willing to rebase your patch on the main branch in the meantime?