Add focus ring to about page form elements

[jkup]

• Submitted a ticket for my issue if one did not already exist.
• Used Github auto-closing keywords in the commit message.
• Added/updated tests for this change (for new code or code which already has tests).
• Ran git rebase -i to squash commits (if needed).

Fix #5971

Auditor @bsclifton, @cezaraugusto

[luixxiul]

Are these expected?

I implemented https://github.com/brave/browser-laptop/blame/master/less/button.less#L9 to avoid unexpected regressions like this: #5661

@bradleyrichter @jkup should

browser-laptop/less/forms.less

Line 614 in f716273

outline: none;

be fixed too?

[luixxiul]

Also is this expected on about:preferences#payments?

[bradleyrichter]

oh no! This may have arrived with a chromium update?

We need to prevent this selection from ALL mouse input outside of the page content box.

[luixxiul]

If only elements on about pages should have the outline, I would add like this in preferences.less:

.prefBody {
  display: flex;
  height: 100%;
  margin-left: @sideBarWidth;

  button[class*="Button"],
  input,
  select {
    &:focus {
      outline: -webkit-focus-ring-color auto 5px;
    }
  }
}

[luixxiul]

@bradleyrichter I thought only button, input and select element should have the outline. Please correct me if I'm wrong.

Also the screenshots were taken after testing the commit by @jkup.

[bradleyrichter]

To be more specific, this selection should only be shown when invoked for accessibility as an indicator for tabbing through elements when a mouse can not be physically used. If we can control this, it can be used in the toolbars as well. I just don't want it to appear outside of an accessibility mode.

[luixxiul]

I just don't want it to appear outside of an accessibility mode.

Can we detect it via JS?

[bradleyrichter]

@luixxiul @jkup

Reviewing Chrome and Firefox shows that no browser toolbar items are tab-advance selectable except for the URL field which should have its own style and not use the item-select border.

Thus, "-webkit-focus-ring-color " should only apply to content area elements and never the toolbar elements.

[jkup]

We want not only prefs pages but also the bookmarks bar to show the outline like Chrome does. Also there is no way to use JS to detect accessibility mode. I'll update the selectors to be more specific.

[bsclifton]

Sorry that I'm just now getting to this; the outline style looks great 😄

When I tried latest against our prefs page, it seems to only be highlighting:

• URL bar
• text fields
• select fields
• bookmarks toolbar (nice!! 😎)

I didn't see buttons get focus, even if I clicked them and then tried the tab ☹️

[jkup]

Fixed it for the navigation buttons like reload/back/forward but the links in preferences#payments should get a focus ring so users know they can interact with them.

What @luixxiul mentioned here is the correct behavior: #6037 (comment)

[jkup]

@bsclifton @luixxiul is this good to go?

[jkup]

Hey @cezaraugusto I know @bsclifton is super busy lately.. any chance you can give this a review for me and add the ready-to-merge label if it looks good? 🙏

[cezaraugusto]

++ Manually tested, LGTM

Not specifically related, but per @luixxiul comment here, it looks weird for me as well. Simple solution would be add display: flex (or block) to link inside .site. But it's ok as-is, just polish.

In the future, as we talk about accessibility, would love to see switch buttons tabbable as well.

[jkup]

@cezaraugusto thanks! I created #6439 to track that UI polish!

[bsclifton]

ran it manually; looking good 😄 Will run travis-ci tests and merge into 0.13.1-branch if passing

[bsclifton]

Last travis-ci run showed several errors that I believe are not related; I re-ran each of those locally and they pass as expected 😄 Merging...

[bsclifton]

Removed milestone after revert with 2aa39bb

[luixxiul]

@bsclifton I'm not sure why this needed to be reverted. Will you please let me know?

[bsclifton]

@luixxiul the focus ring negatively affected too many elements on the page. For example, see these:
#6760
#6761

Also, having the bookmarks toolbar items outlines is undesirable. cc: @bbondy

[bradleyrichter]

There must be a way to contain this to the DOM/page region so it never effects the toolbar elements...

[luixxiul]

#window > .mainContainer could be that way

[bsclifton]

I think at this point, it would be better to go back to the issue and re-open if we think it's important. We can lay out requirements and set an appropriate milestone