Details

Reviewers

mak
dao
ntim

Group Reviewers

desktop-theme-reviewers

Commits

rMOZILLACENTRALf124983faac7: Bug 1691544 - Part 2 - Update focused-and-closed state for Proton. r=mak…

Bugzilla Bug ID

1691544

Summary

This patch mainly does three things:

Changes the necessary CSS to meet the Proton spec.
Removes several calls to UrlbarInput.startLayoutBreakout(), because the Urlbar no longer expands when the panel is not open.
Introduces a [suppress-focus-border] attribute. This attribute hides the 2px focus ring around the Urlbar. This ensures that we don't flash the focus ring when the user clicks a Urlbar that will autoOpen, seeing as the open Urlbar no longer has a border. It also hides the focus border after the user presses Esc, as specified in Figma.

Diff Detail

Repository

rMOZILLACENTRAL mozilla-central

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Build Status

Buildable 292224
Build 385284: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Addressed review feedback and fixed an issue where the Urlbar lost its focus border when the panel was already open and an overridden new tab was opened.

Harbormaster completed remote builds in B289194: Diff 401010.Feb 17 2021, 4:15 PM

In D105097#3424483, @mak wrote:

If I have the urlbar focused and open, and I open a new tab, the focus outline is missing. If instead I focus content then it works correctly. When opening a new tab we should always show the focus outline.

We determined on Slack that this was because the call to remove suppress-focus-border was inside the check for newTab.isEmpty, which is fixed in the latest revision.

The outline should have a 4px radius border, like the other toolbar buttons.

The radius is being set to 4px on Windows here: https://phabricator.services.mozilla.com/D103505#change-RdcMpfyni9RO

browser/base/content/browser.js
2875 ↗	(On Diff #400637)	We don't want to apply `suppress-focus-border` for this instance of autoOpen. If we applied `suppress-focus-border` everytime autoOpen was called, and the user had Top Sites disabled, then when they mousedowned on the address bar, we'd hide the focus border then show it again after we get to `UrlbarView.onQueryFinished`. This would create a flashing effect when an already-focused address was clicked.
browser/components/urlbar/UrlbarView.jsm
510	Sorry, I meant to add a comment on Phabricator about this. This change is not necessary to make this patch work, but I was stepping through autoOpen and noticed that we don't enter this `if` statement on mousedowns on the address bar, which seemed like an oversight. We call autoOpen from `UrlbarInput._on_focus`, so `event.type` is `"focus"` even if there was a mousedown on the address bar. I can remove this change if you'd like, but I thought it was well-suited as a ridealong.
browser/themes/shared/urlbar-searchbar.inc.css
134	Yes, because of LWT override. I'll clarify the comment.

The analysis task source-test-mozlint-license failed, but we could not detect any issue.
Please check this task manually.

The analysis task source-test-mozlint-file-whitespace failed, but we could not detect any issue.
Please check this task manually.

If you see a problem in this automated review, please report it here.

• harry updated this revision to Diff 401199.Feb 17 2021, 10:28 PM

Harbormaster completed remote builds in B289341: Diff 401199.Feb 17 2021, 10:28 PM

• harry added a child revision: D105535: Bug 1691535 - Add Proton styles for the Urlbar results panel. r?#desktop-theme-reviewers!,mak.Feb 17 2021, 10:28 PM

ntim resigned from this revision.Feb 17 2021, 11:31 PM

ntim removed a reviewer: ntim.

Getting this out of my queue, maybe @dao wants to look at it since this is quantumbar related?

It also hides the focus border after the user presses Esc, as specified in Figma.

What's the rationale here? I'm not a fan of the focus ring being conditional to begin with, but this is extra inconsistent on top of that and makes it less clear where focus went.

browser/base/content/browser.js
2875 ↗	(On Diff #400637)	How about making this conditional with an opt-out param to `autoOpen` that `_on_mousedown` would then set? Code external to the address bar having to manage this attribute seems rather nasty.
browser/themes/shared/urlbar-searchbar.inc.css
131	So this makes us use this focus color on Linux: https://searchfox.org/mozilla-central/source/browser/themes/shared/browser.inc.css#31 Linux themes can have all kinds of shades and colors, without falling into the `(prefers-contrast)` category. I think we need to keep using the native highlight color here, at least on Linux.
140	nit: 0 rather than 0px
140	Shadows need to be fully black, otherwise they produce the wrong visual effect on a background that's darker than your shadow's color. You can reduce the opacity to make the shadow fainter.
140	The shadow is cut off at the bottom when the address bar is closed. You need to either reduce the radius / adjust the offset in that case, or position the address bar so it can escape the nav bar (might be tricky).

This revision now requires changes to proceed.Feb 18 2021, 10:06 AM

Afaict, this works pretty well, I applied it on top of the current central that includes toolbar changes and enabled all the proton prefs.

I don't see any critical blocking issues, thus approving, I don't know if @dao wants to further check it, we still have time to address minor things anyway.
The major things that I don't like are:

remove that "focus" fix in autoopen and eventually move it to its own bug
your test is probably leaving a provider registered after its end

browser/base/content/tabbrowser.js
1304 ↗	(On Diff #401199)	nit: for self-containing code reasons (avoid spreading urlbar code everywhere), I wonder if we could move this to UrlbarInput._afterTabSelectAndFocusChange, if we don't autoopen, though I wonder if that would set it with a delay and make perceived performance worse. Would you mind trying?
browser/components/urlbar/UrlbarInput.jsm
2696	Am I thinking right that this would basically be a no-op so this is excluded since it should be removed once proton is released?
browser/components/urlbar/UrlbarView.jsm
510	I can't reproduce an actual user issue here, when we get "focus", we first got "mousedown", so the result is pretty much the same, starting a query twice looks pointless. maybe a part of autoopen is not necessary in _on_focus? If there's an actual user issue, it'd require a test covering it. For which I think you should remove it from this patch and file a separate non-proton bug. In any case, I prefer this to not be part of a proton style patch.
browser/components/urlbar/tests/browser/browser_suppressFocusBorder.js
47	nit: the right thing to do would probably be to use a MutationObserver and when it fires check that the popup is not open. waitForCondition is for when we have no other way to observe changes. It will work anyway. And since this is used in multiple tests it seems to make sense to split it out into a global scope helper fn
56	This is done by almost every test, and your last test is not unregistering the provider. maybe you could have a await withAwaitProvider(args, promise, async provider => { // your test }); and it would handle all the boilerplate and unregisterProvider in a finally.
browser/themes/shared/urlbar-searchbar.inc.css
21	Considering the wanted missing indentation, can we use END comment similar to the ones in proton tabs? https://searchfox.org/mozilla-central/rev/8432d4fe31245ae9121231d7c0335aaa342675d2/browser/themes/shared/tabs.inc.css#220 It's more verbose may it may help with reading braces

In D105097#3429100, @dao wrote:

It also hides the focus border after the user presses Esc, as specified in Figma.

What's the rationale here? I'm not a fan of the focus ring being conditional to begin with, but this is extra inconsistent on top of that and makes it less clear where focus went.

The rationale is that we move from "open panel without a border" to "input field with a thick border flashing in your face". I think it's annoying, but mostly I think it's a problem for the flashing part and users sensible to that. So eventually we could move it under prefers-reduced-motion.

I don't think it makes focus less clear, thanks to the drop shadow.

In D105097#3429329, @mak wrote:

In D105097#3429100, @dao wrote:

It also hides the focus border after the user presses Esc, as specified in Figma.

What's the rationale here? I'm not a fan of the focus ring being conditional to begin with, but this is extra inconsistent on top of that and makes it less clear where focus went.

The rationale is that we move from "open panel without a border" to "input field with a thick border flashing in your face".

That's kind of the point of focus rings. They make it clear where focus is. Chrome behaves the same btw.

I think it's annoying, but mostly I think it's a problem for the flashing part and users sensible to that. So eventually we could move it under prefers-reduced-motion.

I'm not sure calling this "flashing" is fair, it just appears like any other focus ring would. I don't think prefers-reduced-motion is the right proxy for light-sensitivity either.

I don't think it makes focus less clear, thanks to the drop shadow.

A drop shadow doesn't generally indicate focus. I understand that's the idea here, but we can't just make up new GUI conventions and expect users to understand them. The drop shadow is also going to be close to invisible with dark themes, including high contrast ones.

In D105097#3429570, @dao wrote:

I'm not sure calling this "flashing" is fair, it just appears like any other focus ring would. I don't think prefers-reduced-motion is the right proxy for light-sensitivity either.

The problem exists though, because the picked color is very bright, and the border is particularly thick.
Other browsers solve this by using a less lively color for the focus border.

This is my use-case:

focus urlbar (suppose top sites are disabled so we show the border)
type something
press ESC to check something in the current page
keep typing

This flashes between border, no border, border, no border.
What's your suggestion about this?

I'm not sure whether it's due to the latest patch, but now if I open the urlbar and then click on an engine shortcut button, the open panel gets the focus border

mak requested changes to this revision.Feb 18 2021, 2:47 PM

ntim removed a subscriber: ntim.Feb 18 2021, 2:54 PM

In D105097#3429692, @mak wrote:

In D105097#3429570, @dao wrote:

I'm not sure calling this "flashing" is fair, it just appears like any other focus ring would. I don't think prefers-reduced-motion is the right proxy for light-sensitivity either.

The problem exists though, because the picked color is very bright, and the border is particularly thick.
Other browsers solve this by using a less lively color for the focus border.

Chrome appears to be using the native highlight color on Ubuntu. I guess that's the case on Windows and Mac too?

This is my use-case:

focus urlbar (suppose top sites are disabled so we show the border)

type something

press ESC to check something in the current page

keep typing

This flashes between border, no border, border, no border.
What's your suggestion about this?

I'm afraid that use case isn't common enough to really optimize for it, at least not the way this patch does, sacrificing accessibility. However I think using a non-native color for focus rings in chrome is generally a mistake, so if we undid that I guess that would mitigate your problem based on what you said above.

In D105097#3430058, @dao wrote:

Chrome appears to be using the native highlight color on Ubuntu. I guess that's the case on Windows and Mac too?

I'm not sure, with the system dark theme, Chrome and Edg-ium use totally different colors. Windows Explorer doesn't have a focus outline so I'm not sure how to tell what color is right.
Edge uses a much brighter color, similar to what we plan to use. And they don't seem to care about the flashing problem.

Let's file this problem apart and land this with simple boolean behavior, closed and focused has border, open and focused has not.

• harry updated this revision to Diff 401667.Feb 18 2021, 8:49 PM

• harry mentioned this in D105672: Bug 1691544 - Part 1 - Use -moz-accent-color for focus ring on Linux. r?dao!,yzen.

• harry retitled this revision from Bug 1691544 - Update focused-and-closed state for Proton. r?mak to Bug 1691544 - Part 2 - Update focused-and-closed state for Proton. r?mak.

• harry marked 10 inline comments as done.

Harbormaster completed remote builds in B289751: Diff 401667.Feb 18 2021, 8:49 PM

• harry added a parent revision: D105672: Bug 1691544 - Part 1 - Use -moz-accent-color for focus ring on Linux. r?dao!,yzen.Feb 18 2021, 9:24 PM

• harry marked an inline comment as done.

• harry added inline comments.

browser/base/content/tabbrowser.js
1304 ↗	(On Diff #401199)	I moved it to `_afterTabSelectAndFocusChange`. I think it looks okay, although that's on a machine much faster than the reference machine. What do you think?
browser/components/urlbar/UrlbarInput.jsm
2696	That's right, yes.
browser/themes/shared/urlbar-searchbar.inc.css
131	See D105672

Addressed feedback and rebased on centralx

Harbormaster completed remote builds in B290264: Diff 402213.Feb 19 2021, 9:41 PM

dao requested changes to this revision.Feb 22 2021, 11:30 AM

dao added inline comments.

browser/components/urlbar/UrlbarView.jsm
599	Shouldn't the `close` method itself always remove the `suppress-focus-border` attribute? Not sure if this matters in practice but seems cleaner anyway.

This revision now requires changes to proceed.Feb 22 2021, 11:30 AM

UrlbarView.close() now manages removeAttribute("suppress-focus-border"). Also fixed a bug where the border would flash after a result was selected.

Harbormaster completed remote builds in B290788: Diff 402805.Feb 22 2021, 4:54 PM

dao requested changes to this revision.Feb 22 2021, 11:15 PM

dao added inline comments.

browser/components/urlbar/UrlbarView.jsm
439	Please make these named parameters of an object instead, to avoid the need for the /* showFocusBorder */ comment, as well as the need to specify the first parameter in order to set the second one.
browser/themes/shared/urlbar-searchbar.inc.css
144	From what I understand there's still a problem for the popup opening behind a very dark / black background, due to the shadow blending in and the removal of the border.

This revision now requires changes to proceed.Feb 22 2021, 11:15 PM

mak added inline comments.Feb 23 2021, 12:16 PM

browser/components/urlbar/UrlbarInput.jsm
1626	nit: I don't think this comment adds anything to the code.
browser/themes/shared/urlbar-searchbar.inc.css
144	Imo, there is also a similar problem when the window is unfocused, the panel can completely blur with the content if they have a similar background color. For example I see that with the panel open on the new tab page, it's impossible to distinguish the boundary between the panel and the border. Maybe we should have a very subtle border anyway, or we should keep the shadow also when the window is unfocused. Worth asking Amy imo. Having a subtle border would also help with the very dark background problem? Other browsers don't suffer this problem because they close the panel when the window loses focus.
144	I meant "the boundary between the panel and the content", I wrote border by mistake.

Addressed review feedback. Haven't yet addressed the dark mode contrast issue since that will require UX input.

Harbormaster completed remote builds in B291274: Diff 403381.Feb 23 2021, 2:42 PM

you can ignore my comments about the open panel on unfocused window, I had the devtools option to keep panel open and forgot about it.

Imo this is a good start. There will be more work to do, but having the basic styling in may speed up and parallelize the polish work.
For which, I'd leave the dark no-border problem to a follow-up that we can address once UX answers our question.
I didn't find major issues while testing, even if of course the dark theme can't be properly tested without colors defined.
I'm leaving the last word to Dao, since this is mostly a theme problem.

browser/components/urlbar/UrlbarView.jsm
493–494	It'd be nice to also make this named like in the close() case, even if for now there is only a single option. queryOptions should also not be optional anymore. Also queryOptions only contains event now... so... probably you just want to make this: autoOpen({ event, suppressFocusBorder = true }) and fix the few callers.

mak added 1 blocking reviewer(s): dao.Feb 23 2021, 5:57 PM

Addressed review feedback

Harbormaster completed remote builds in B291482: Diff 403624.Feb 23 2021, 10:14 PM

dao accepted this revision.Feb 24 2021, 9:50 AM

dao added inline comments.

browser/components/urlbar/UrlbarView.jsm
488–491	Please update this.

This revision is now accepted and ready to land.Feb 24 2021, 9:50 AM

mak added inline comments.Feb 24 2021, 10:35 AM

browser/components/urlbar/UrlbarView.jsm
509	I just noticed that this condition lost the !this.isOpen check. I'd prefer avoid changing things so subtly unless there's a clear explanation, regressions are easy and we don't want them in styling patches.