1406320 - Remove the dotted focus border from selected row

Status: RESOLVED WONTFIX

(DevTools :: Netmonitor, enhancement, P3)

Description

[[e-mail]]

Attachment: current select request shows dotted border when focus

In network monitor the selected row shows dotted focus border, in new photon design UX decide to remove that effect.

Comment 1

[[e-mail]]

Here's the tip to complete this issue:

After clone the repo from mozilla-central, you can build the whole mozilla-central repo by following
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

Or just follow the guide to run netmonitor on a browser tab.
https://github.com/mozilla/gecko-dev/tree/master/devtools/client/netmonitor#quick-setup


When you bring up the firefox, you can open network monitor, visit any web site to collect some requests.

Then open `Menu > Tools > Web Developer > Browser Toolbox`, use the Inspector in Browser Toolbox to check what style cause the dotted border.
https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox


The result will be add a style in netmonitor.css to apply `outline: none;` on some element's `-moz-focusring` declaration (the element likely start with request-list-).

Comment 2

[[e-mail]]

mithilan91 I saw you want to find something to contribute at bug 1404130 comment 4,

Are you interest in picking this issue?

Comment 3

[ewhite7]

I'm a student at Seneca college learning open source, and I was hoping to work on this bug for my course. i recently clone the repo from Mozilla-central, and was able to remove the dotted focus border inside the browser,but can'r seem to locate the file inside the Mozilla-central folder

Comment 4

[[e-mail]]

Hi ewhite7,

Nice to see you've some progress!

Here's a Mozilla-central source search service, you can type `netmonitor.css` in the search bar to find the right path. ex:

http://searchfox.org/mozilla-central/search?q=netmonitor.css&path=

If you have any question, feel free to reply on bugzilla, chat in IRC or slack

Comment 5

[ewhite7]

OK. please send IRC handle

Comment 6

[Miguel Dorta]

I think I fixed this bug, can I submit it?

Comment 7

[ewhite7]

(In reply to Miguel Dorta from comment #6)
> I think I fixed this bug, can I submit it?

how did you fix it? i was working on this for my open source project.can we collaborate?

Comment 8

[Miguel Dorta]

(In reply to ewhite7 from comment #7)
> (In reply to Miguel Dorta from comment #6)
> > I think I fixed this bug, can I submit it?
> 
> how did you fix it? i was working on this for my open source project.can we
> collaborate?

Yeah!

Uhm I found the file /devtools/client/theme/variables.css a variable called "--theme-focus-outline", It had the value "1px dotted var(--theme-focus-outline-color);"

Then I just turned 1px to 0px and the border disappeared.

Maybe we should remove the references of that variable if it's not going to be used?

Comment 9

[[e-mail]]

Thanks for finding the variable, though I'd expect removing focus-ring in request-list-item like this but in netmonitor.css

http://searchfox.org/mozilla-central/source/devtools/client/themes/common.css#232

not change the whole style which might cause more issues (consider accessibility/keyboard navigation) than fix things.

Comment 10

[ewhite7]

(In reply to Miguel Dorta from comment #8)
> (In reply to ewhite7 from comment #7)
> > (In reply to Miguel Dorta from comment #6)
> > > I think I fixed this bug, can I submit it?
> > 
> > how did you fix it? i was working on this for my open source project.can we
> > collaborate?
> 
> Yeah!
> 
> Uhm I found the file /devtools/client/theme/variables.css a variable called
> "--theme-focus-outline", It had the value "1px dotted
> var(--theme-focus-outline-color);"
> 
> Then I just turned 1px to 0px and the border disappeared.
> 
> Maybe we should remove the references of that variable if it's not going to
> be used?

OK so i did the same thing,but didn't see any change when i rebuild Mozilla
https://i.imgur.com/1WynvHS.gifv

Comment 11

[Miguel Dorta]

That variable changes de border in the developer tools on the network tab, I thought it was that because of the attachment.

Comment 12

[ewhite7]

(In reply to Fred Lin [:gasolin] from comment #9)
> Thanks for finding the variable, though I'd expect removing focus-ring in
> request-list-item like this but in netmonitor.css
> 
> http://searchfox.org/mozilla-central/source/devtools/client/themes/common.
> css#232
> 
> not change the whole style which might cause more issues (consider
> accessibility/keyboard navigation) than fix things.

this should do the trick.
.request-list-item {
  display: table-row;
  height: 24px;
  outline: none !important;
}

.request-list-item.selected {
  color: var(--theme-selection-color);
  outline: none !important;
}

Comment 13

[ewhite7]

if the above code is good please instruct me on how to push, else let me knw if i am close

Comment 14

[ewhite7]

Attachment: outline none was added to request-list-item

please Review this patch

Comment 15

[[e-mail]]

Comment on attachment 8919101 [details] [diff] [review]
outline none was added to request-list-item

Thanks for contribute!

The .patch should only see your changes in diff, not change the whole file


>+++ 
>+++ /* Request list item */
>+++ 
>+++ .request-list-item {
>+++   display: table-row;
>+++   height: 24px;
>+++-  outline: none !important
>++++  outline: none !important;
>+++ }

`!important;` should be used as the last missle, since it make debugging large CSS harder.

As I said in comment 9, consider use `:-moz-focusring` selector instead, ex:

```
.request-list-item:-moz-focusring {
  outline: none;
}
```

>+++ 
>+++ .request-list-item.selected {
>+++   background-color: var(--theme-selection-background);
>+++   color: var(--theme-selection-color);
>+++-  outline: none !important
>++++  outline: none !important;
>+++ }
>+++

Comment 16

[[e-mail]]

Very sorry for the late response, I should find that in advance. 
Please use `needinfo` field if you need feedback.

If you haven't checked the devtools doc, you can take a look first
https://github.com/mozilla/gecko-dev/tree/master/devtools/docs/getting-started


To verify this visual change, besides the normal `build with firefox` way, you can also run netmonitor on the browser new tab to check the visual change

https://github.com/mozilla/gecko-dev/tree/master/devtools/client/netmonitor

If you have any problem, you can reply on this bug or find help on irc/slack.

Comment 17

[ewhite7]

OK so i played around with yarn a few time and everything looks fine with the visual.i also removed the !important and applied the
:-moz-focusring

Comment 18

[ewhite7]

Attachment: 1406320.patch

Comment 19

[[e-mail]]

Comment on attachment 8919440 [details] [diff] [review]
1406320.patch

Thanks for the new PR. Please address below issues

> /* Request list item */
> 
>-.request-list-item {
>+.request-list-item:-moz-focusring{
>   display: table-row;
>   height: 24px;
>+  outline: none;
> }
> 

We can't just replace `request-list-item {` to `.request-list-item:-moz-focusring {` since we will miss the base style.

Please create a separate rule below `.request-list-item {` style


>+.request-list-item:-moz-focusring{

for style convention, please make sure there's a space between -moz-focusring and { `-moz-focusring { `


>-
>diff --git a/devtools/client/netmonitor/yarn.lock b/devtools/client/netmonitor/yarn.lock
>--- a/devtools/client/netmonitor/yarn.lock
>+++ b/devtools/client/netmonitor/yarn.lock
>@@ -5678,17 +5678,17 @@ webpack@^2.2.1:
>     source-map "^0.5.3"
>     supports-color "^3.1.0"
>     tapable "~0.2.5"
>     uglify-js "^2.8.27"
>     watchpack "^1.3.1"
>     webpack-sources "^1.0.1"
>     yargs "^6.0.0"
> 
>-webpack@^3.3.0, webpack@^3.5.6:
>+webpack@^3.3.0:
>   version "3.5.6"
>   resolved "https://registry.yarnpkg.com/webpack/-/webpack-3.5.6.tgz#a492fb6c1ed7f573816f90e00c8fbb5a20cc5c36"
>   dependencies:
>     acorn "^5.0.0"
>     acorn-dynamic-import "^2.0.0"
>     ajv "^5.1.5"
>     ajv-keywords "^2.0.0"
>     async "^2.1.2"

Please don't include the yarn.lock change in this CSS style change patch. We should only change yarn.lock when we update some npm package

Comment 20

[ewhite7]

i created the new rule and all seems well, and to be honest i am not sure how to remove yarn lock.
what i did was go into the dir and run "hg diff > example.diff"

Comment 21

[[e-mail]]

That's fine, you can see what's changed by `hg diff`, then run `hg checkout yarn.lock` and apply on some other files you find not relevant. After that, run `hg diff` again and check if the diff only contains the change you expect.

Comment 22

[ewhite7]

Attachment: new rule

yarn lock might still be an issues

Comment 23

[[e-mail]]

Comment on attachment 8920627 [details] [diff] [review]
new rule

> 
> /* Request list item */
> 
> .request-list-item {
>   display: table-row;
>   height: 24px;
> }
> 
>+.request-list-item:-moz-focusring {
>+  display: table-row;
>+  height: 24px;
>+  outline: none;
>+}
>+

You don't have to add display/height since they will inherit from main `.request-list-item` rule

Comment 24

[ewhite7]

ok i will fix it. i have to get this bug sign off before this weekend in order to get a grade, so if you have any more suggestion please let me know

Comment 25

[[e-mail]]

Remove yarn.lock change and fix issue addressed in comment 23, that's all I think.

Please provide the updated patch and I'm happy to review.

Comment 26

[ewhite7]

Attachment: 1406320.diff

removed display/height

Comment 27

[[e-mail]]

Hi ewhite7,

Please change as below

```
.request-list-item {
  display: table-row;
  height: 24px;
}

.request-list-item:-moz-focusring {
  outline: none;
}

```

don't add `outline: none;` in `request-list-item`. and make sure there's only 1 blank line below `.request-list-item:-moz-focusring` style, then the patch will be fine.

Please remove the devtools/client/netmonitor/yarn.lock changes.

You can open the .diff, remove everything under

```
-
diff --git a/devtools/client/netmonitor/yarn.lock b/devtools/client/netmonitor/yarn.lock
```

and try apply patch to make sure the diff still works.

Comment 28

[Tim Nguyen :ntim]

:gasolin, does this change break keyboard accessibility ? Atm, when tabbing through the netmonitor, the outline is the only focus indicator.

Comment 29

[ewhite7]

Attachment: 1406320.diff

Comment 30

[[e-mail]]

ntim you are right, we'd better rather leave this as it is so we won't break the keyboard navigation.


ewhite7, sorry I made a mistake. We can fix that issue with more effort(not showing the dotted focus border when mouse select, but show it when we use key navigation), but it might not worth the effort.

Comment 31

[Victoria Wang [:victoria]]

Thanks all for the work and discussion on this - and I apologize, I didn't realize my original suggestion would affect keyboard usability.

I wanted to add that we have a new focus ring style for links: https://design.firefox.com/photon/components/links.html#behaviors
However, it needs some adaptation to work for table rows, so for now I'll put this in my polish doc, talk to the design system group, and report back later.