1768049 - Show visible focus outline on Screenshots buttons

Status: RESOLVED WONTFIX

(Firefox :: Screenshots, defect, P3)

Description

[Sam Foster [:sfoster] (he/him)]

There is little visible change when the "copy" and "download" buttons are focused - they should have a clear focus outline similar to other browser UI buttons in Firefox.

Via: https://discourse.mozilla.org/t/screenshot-with-keyboard-focus-buttons-dont-appear-show-something/96354

Comment 1

[Sam Foster [:sfoster] (he/him)]

For historical reason, the CSS is question lives in a JS file: inlineSelectionCSS.js

Comment 2

[Fede Santana]

On the line 129 of inlineSelectionCSS.js we have the focus of primaryButton with other properties:

  .button.primary:hover, .primary.highlight-button-cancel:hover, .highlight-button-download:hover, .primary.highlight-button-copy:hover, .button.primary:focus, .primary.highlight-button-cancel:focus, .highlight-button-download:focus, .primary.highlight-button-copy:focus {
    background-color: #0072e5; 
}

maybe removing highlight-button-download:focus from there and put an extra rule:

.highlight-button-download .highlight-button-cancel .highlight-button-copy {
outline: //some outline you usually use
}

( this is my first time on bugzilla, please tell me if I did something wrong )

Comment 3

[Sam Foster [:sfoster] (he/him)]

Yeah that looks about right. Because of the way this UI is constructed and injected into the content document, we can't make direct use of the stylesheets we would normally use to provide consistent style. Here's the rule we would normally use focused buttons

We basically want the same here. We'll need to hard-code the color value rather than use the CSS variable. All these shortcomings are being addressed in an on-going port to a browser component rather than the web extension it is today. But as that is not expected to replace the extension any time soon, I'd be happy to accept a patch for this issue. If you want to have a go at putting together a patch for this, we have getting started documentation. If not I'll mark this as a good-first-bug to make it visible for other potential contributors. I'm happy to answer questions here or in our #fx-screenshots:mozilla.org matrix channel.

For questions about the process of building firefox and getting your first patch in for review, the #introduction channel is there to help.

Comment 4

[aharmon413]

Hi Sam, it looks like there hasn't been any activity on this bug in a while, is it still open? This is my first time contributing to open source and I'd like to take this on!

Comment 5

[Sam Foster [:sfoster] (he/him)]

(In reply to aharmon413 from comment #4)

Hi Sam, it looks like there hasn't been any activity on this bug in a while, is it still open? This is my first time contributing to open source and I'd like to take this on!

Sure. Everything I wrote in comment 3 is still true and applicable. Once you have a work-in-progress patch I can assign the bug to you.

Comment 6

[aharmon413]

Sure. Everything I wrote in comment 3 is still true and applicable. Once you have a work-in-progress patch I can assign the bug to you.

How exactly would I go about making a work in progress patch? Is that using Phabricator? I already have Firefox built on my system.

Comment 7

[Sam Foster [:sfoster] (he/him)]

(In reply to aharmon413 from comment #6)

How exactly would I go about making a work in progress patch? Is that using Phabricator? I already have Firefox built on my system.

Yeah, you can submit with moz-phab submit --wip to share a patch and mark it as in-progress. Or just submit normally and your reviewer (me in this case) will let you know if they think more work is needed, answer any questions you leave on there etc.

Comment 8

[aharmon413]

Attachment: WIP: Bug 1768049 - Located file to be edited

Comment 9

[aharmon413]

Yeah, you can submit with moz-phab submit --wip to share a patch and mark it as in-progress. Or just submit normally and your reviewer (me in this case) will let you know if they think more work is needed, answer any questions you leave on there etc.

I've submitted a WIP patch at https://phabricator.services.mozilla.com/D148525 and I will try to finish this today. Please let me know if I did something incorrectly, this is my first time doing anything like this!

Comment 10

[Sam Foster [:sfoster] (he/him)]

(In reply to aharmon413 from comment #9)

I've submitted a WIP patch at https://phabricator.services.mozilla.com/D148525 and I will try to finish this today. Please let me know if I did something incorrectly, this is my first time doing anything like this!

Yeah so far so good. When you have a working patch please test it locally, then submit for review. You can add r?sfoster to your commit message, or add me as a reviewer in the phabricator UI.

Comment 11

[aharmon413]

Yeah so far so good. When you have a working patch please test it locally, then submit for review. You can add r?sfoster to your commit message, or add me as a reviewer in the phabricator UI.

I've added you as a reviewer and posted a question on Phabricator, when you get a chance could you please look at it? Thanks!

Comment 12

[aharmon413]

Attachment: WIP: Bug 1768049 - Show visible focus outline on Screenshots buttons

Depends on D148525

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 14

[Sam Foster [:sfoster] (he/him)]

This work is specific to the extension, so I'm adding a whiteboard tag so we don't get it confused with ongoing work on the component implementation.

Comment 15

[Sam Foster [:sfoster] (he/him)]

Closing this as we've fixed this bug in the screenshots.browser.component.enabled version, and I think we're close enough to shipping that that it doesn't make sense to patch this in the extension at this point.