1706479 - Focus ring for extension page action is different color than built in page actions when dark mode or theme are enabled

Status: VERIFIED FIXED

(Firefox :: Foxfooding, defect, P2)

Description

[Jody Heavener]

Attachment: browser-action-focus-ring.mov

steps to reproduce/what did you do?

• Install an extension that adds a new page action, such as Firefox Multi-Account Containers
• Using your keyboard, tab into the page actions. Focus on any built-in page action such as "Bookmark", and then tab into the extension page action

expected behavior/ what did you think will happen?

• The focus rings are the same color

actual behavior/ what actually happened?

• The focus ring for the built-in action is theme color (blue)
• The focus ring for the extension action is white

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

I took a look into this and the reason for the different color used for the extension pageAction outline is due to these rules added in urlbar-search.inc.css by Bug 1702034 and Bug 1704752:

@media (-moz-proton) {
  ...
  /* The background can be very dark and if the add-on uses a black-ish svg it
   will be barely visible. In the future we should have a standardized SVG
   solution we can apply to add-on icons, for now we can only try to make them
   visible through some filtering tricks */
:root[lwt-toolbar-field-brighttext] #urlbar:not([focused="true"]) .urlbar-addon-page-action[style*=".svg"],
:root[lwt-toolbar-field-focus-brighttext] #urlbar[focused="true"] .urlbar-addon-page-action[style*=".svg"] {
  filter: grayscale(100%) brightness(20%) invert();
}

@media (prefers-color-scheme: dark) {
  /* As above, but for the default theme in dark mode, which suffers from the same issue */
  :root:not(:-moz-lwtheme) .urlbar-addon-page-action[style*=".svg"] {
    filter: grayscale(100%) brightness(20%) invert();
  }
}
} /*** END proton ***/

and so it is not actually the outline color to be different, but when the Firefox UI is using the dark builtin theme or a system theme in dark mode the filter applied to the svg icon from these CSS rules does change to white both the svg icon itself and the outline visible when the urlbar button is focused.

This is also confirmed by the fact that the outline color around the page action does actually match the one used by the other element in the urlbar if the active Firefox theme is the Light built in theme or if the system theme in in light mode.

In my opinion the issue that Bug 1702034 and Bug 1704752 are currently workarounding is way worse than this one ([1]), and so if we don't have a low risky approach to prevent the filter to be applied to the outline with the pageAction element as is, ("as is" => without changing the DOM elements that compose the pageAction element) then it may be reasonable to not consider this a blocking issue for Proton and look into a proper and more complete fix that we would not uplift (unless it turns out to be really a low risky one).

As additional side notes:

• as I did mention to Marco while we were dealing with Bug 1702034, the extension browserAction buttons do support themed icons, which allow the extensions to provide a separate icon to be used by Firefox in dark themes (see theme_icons property description in the related MDN doc page), pageAction does not support that at the moment, but we may evaluate to add support for that. If the extension opt in to use pageAction theme icons, then we could avoid to apply a filter and the outline would be rendered using the same color used for the other elements in the urlbar

• in the future, as part of manifest_version 3 changes, we may merge the pageAction and browserAction (See bug 1687756), the manifest_version 3 action manifest property will likely have to support everything that was supported by a browserAction (and so that would also include the theme_icons sub property). At the moment I can't tell for sure if we would expand the action API to let the extension to include the icon also in the urlbar (as the pageAction do) or if all extension actions will only be shown as browserAction buttons, but the outcome of that work may also change what we will decide to do about this one.

Hey Marco, wdyt about this? do you agree that this shouldn't be considered a blocking issue? do you have some more ideas about how we may want to deal with it?

---

[1]: e.g. because the icon itself is more often visible in the urlbar than the focus ring, and before those two fixes the icon were not that much visible in dark mode. On the contrary the focus ring is visible way less often (only if the element is currently focused which is easier to trigger and notice while navigating the urlbar items using the keyboard, less easier to trigger and spot while using the mouse), and even if the focus ring does not match the focus ring color of the other elements it does still have a pretty good contrast and so at least it doesn't affect the readability of the UI

Comment 2

[Marco Bonardo [:mak]]

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #1)

Hey Marco, wdyt about this? do you agree that this shouldn't be considered a blocking issue? do you have some more ideas about how we may want to deal with it?

I agree with your analysis. If we'd find a simple css fix for this I'd be glad to take it, otherwise it seems a small visual annoyance, that functionally doesn't break anything, the outline is well visible anyway.
Long term there's still the two things we're missing: 1. standardized svg fill replacement, 2. support for light/dark themes in WebExt API.

Comment 3

[Marco Bonardo [:mak]]

Regarding a potential short term fix, I wonder if we could apply the filter to a descendant of urlbar-addon-page-action, maybe just .button-box > .button-icon? I didn't try, but it may be worth a try.

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Marco Bonardo [:mak] from comment #3)

Regarding a potential short term fix, I wonder if we could apply the filter to a descendant of urlbar-addon-page-action, maybe just .button-box > .button-icon? I didn't try, but it may be worth a try.

I took a look if this was possible, but it doesn't seem it is without reworking the pageAction node elements (and maybe some of the other css rules) a bit more:

at the moment the extension page action is a single XUL image tag element, and both the icon and the outline CSS rules are applied to this single element.

I did some quick experiments on wrapping the extension pageActions into the same kind of hbox wrapper that surrounds the bookmarks star icon but unfortunately just wrapping it wasn't enough on its own to prevent the CSS filter to be applied to the outline (I think that it was because the outline was still be applied on the wrapped image tag and not to the hbox wrapper, but I have to look a bit more into it to get a better picture) and so there are at least some more changes required to be looked into.

Comment 5

[Harry Twyford]

Attachment: Bug 1706479 - Wrap page action icons in an hbox. r?#desktop-theme-reviewers,#extension-reviewers,morgan

Comment 6

[Harry Twyford]

Attachment: Bug 1706479 - Simplify page action style rules. r?#desktop-theme-reviewers

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e90263d64998
Wrap page action icons in an hbox. r=morgan,adw

Comment 8

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on browser_parsable_css.js

backout: https://hg.mozilla.org/integration/autoland/rev/d14f580563830536b83db7b96ce82abebf3c5eb2

push: https://treeherder.mozilla.org/jobs?repo=autoland&revision=e90263d64998370179237e38f4608dc179a227d1&group_state=expanded&selectedTaskRun=GEGwBPEwRiGq_wo1VxXoQQ.0

failure log: https://treeherder.mozilla.org/logviewer?job_id=340036453&repo=autoland&lineNumber=1750

[task 2021-05-18T15:10:55.207Z] 15:10:55 INFO - Buffered messages finished
[task 2021-05-18T15:10:55.208Z] 15:10:55 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_parsable_css.js | Got error message for chrome://browser/skin/browser.css: Ruleset ignored due to bad selector. -
[task 2021-05-18T15:10:55.208Z] 15:10:55 INFO - Stack trace:
[task 2021-05-18T15:10:55.208Z] 15:10:55 INFO - chrome://mochikit/content/browser-test.js:test_ok:1334
[task 2021-05-18T15:10:55.208Z] 15:10:55 INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:messageIsCSSError:247
[task 2021-05-18T15:10:55.208Z] 15:10:55 INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:checkAllTheCSS:468
[task 2021-05-18T15:10:55.208Z] 15:10:55 INFO - Not taking screenshot here: see the one that was previously logged

Comment 9

[Natalia Csoregi [:nataliaCs]]

Also failing: TEST-UNEXPECTED-FAIL | browser/base/content/test/keyboard/browser_toolbarButtonKeyPress.js | Test timed out -

Comment 10

[Harry Twyford]

There's a new revision on Phabricator. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6de0054229cbc42ded87bc4b43b7973bedf6f25

Comment 11

[Harry Twyford]

Attachment: Bug 1706479 - Part 2 - Stop overriding tooltiptext on bookmark icon. r?adw!

This resolves a failure in browser/base/content/test/pageActions-proton/browser_PageActions_bookmark.js. That test checks the tooltiptext on the bookmark button hbox. Before part 1 of this patch, the hbox always had one of the generic tooltiptexts "Bookmark Current Tab"/"Edit This Bookmark". This tooltiptext was set in updateBookmarkPageMenuItem. Then, in __updateStar, we set the tooltiptext on the <image> contained in the hbox to be "Bookmark this page ([shortcut])"/"Edit this bookmark ([shortcut])". Since the <image> filled the entire hbox, the tooltiptext on the hbox underneath was never seen. Since Part 1 of this patch moved the semantic meaning of the page action buttons from the <image> to the hbox, the tooltiptext containing the shortcut was also moved to the hbox. However, updateBookmarkPageMenuItem was overriding the shortcut tooltiptext with the generic text and thus browser_PageActions_bookmark.js was failing intermittently. The timing of when the tooltiptext was overridden was variable.

This patch stops setting the generic tooltiptext on the hbox altogether. I'm splitting this out into its own patch since I think it needs its own review. With Part 1 applied, when actually using the browser, the hbox had the tooltiptext with the shortcut. I'm not totally clear on why that was working despite updateBookmarkPageMenuItem overriding it, so I'd like to make sure this change is okay.

Depends on D114131

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23a910bc6d46
Wrap page action icons in an hbox. r=morgan,adw
https://hg.mozilla.org/integration/autoland/rev/50f0907c08de
Part 2 - Stop overriding tooltiptext on bookmark icon. r=adw

Comment 13

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/23a910bc6d46
https://hg.mozilla.org/mozilla-central/rev/50f0907c08de

Comment 14

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Reproduced the issue with Firefox 90.0a1 (20210420213949) on Windows 10x64.
The issue is verified fixed with Firefox 90.0b4 (20210606185718) on macOS 10.13, Ubuntu 18.04, and Windows 10x64. The focus ring has the same color as other extensions.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b7dd99cdee2
Simplify page action style rules. r=desktop-theme-reviewers,dao

Comment 16

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/8b7dd99cdee2