1508724 - Checkbox label for Shockwave Flash in about:addons plugins has improper tab_focus

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Cristian Fogel [:cfogel]]

Attachment: adoutAddons_plugins_Flash_checkBoxTextLabel.png

[Affected versions]:
- 45.3.0esr, 63.0b3, 64.0b10, 65.0a1 (2018-11-20)  

[Affected platforms]:
- win10x64, Ubuntu18.04;

[Steps to reproduce]:
1. Launch Firefox;
2. Access about:addons
3. Click on the Plugins section;
4. (Double)Click on the Showckwave Flash item;
5. Drag and select the Learn More.. text (so it has the dotted outline);
6. Press the Tab key on the keyboard;

[Expected result]:
- the checkbox is marked and selected;

[Actual result]:
- the dotted select line is on the dummy label and not the checkbox;

[Regression range]:
*mozregression run where the considered good version did not apear to have the checkbox at all:
- last good: 2017-04-13 changeset: 819a666afddc804b6099ee1b3cff3a0fdf35ec15
- first bad: 2017-04-14 chngeset: cda24082bff8864a6e53726feeae33cae9e17309
pushlog url: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c40ca7a1bdd93632c6bdc5e23bd33d984d508b19&tochange=308cdb913d717f8edc932f5bac93854e3e54b84d

*mozregression run where the considered good version still had the "line" smaller ~2-3pixels;
- last good: 2017-11-14 changeset: f0c0fb9182d695081edf170d8e3bcb8164f2c96a
- first bad: 2017-11-15 chngeset: e070277ec199fa96fa490ed52d33646a376d0d80

[Additional notes]:
- attached screenshot with the issue;
- on macOS the tabIndex skips this checkbox;
- for Ubuntu Shockwave Flash needs to be added;

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Felipe, can you look in this?

Comment 2

[Marcia Knous [:marcia]]

Discussed during triage, won't fixing this for 63 and ESR.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

So I think the main problem is that this is a checkbox with no text content for the label. A similar problem was handled in bug 669507. Although that bug title appears to have solved the problem for all cases, in reality that bug only fixed it with a workaround.

I think we should just do the same workaround here and not display the inner .checkbox-label-box for this #pluginFlashBlockingCheckbox checkbox.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

To fix this bug, we just need to add something similar to this: https://searchfox.org/mozilla-central/rev/876022232b15425bb9efde189caf747823b39567/devtools/client/shared/widgets/widgets.css#41-43

to this file: https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.css

Comment 5

[TomS [:toms]]

Hello. I'm happy to take this as my first contribution.

Comment 6

[[e-mail]]

(In reply to Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me) from comment #1)
> Felipe, can you look in this?

hey! can i take this bug?

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Hi Tom, are you interested in taking this bug?

Do you already have a copy of the source code, and have you set up the things needed to compile Firefox? If not yet, I recommend that you take a look at this page: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds

Comment 8

[:Felipe Gomes (needinfo for replies!)]

(In reply to Abhishek Chandrasenan [:acnair] from comment #6)
> 
> hey! can i take this bug?

Hi Abhishek, Tom had asked first to work on this bug, so let's give him a chance to work on it. Let me know if you need any help on finding some other good-first-bug to take

Comment 9

[TomS [:toms]]

(In reply to :Felipe Gomes (needinfo me!) from comment #7)
> Hi Tom, are you interested in taking this bug?
> 
> Do you already have a copy of the source code, and have you set up the
> things needed to compile Firefox? If not yet, I recommend that you take a
> look at this page:
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Build_Instructions/Artifact_builds

Hey Felipe. Yes, I've compiled the 65.0a1 nightly and have arc and moz phab installed. I should be good to go.

Comment 10

[[e-mail]]

(In reply to :Felipe Gomes (needinfo me!) from comment #8)
> (In reply to Abhishek Chandrasenan [:acnair] from comment #6)
> > 
> > hey! can i take this bug?
> 
> Hi Abhishek, Tom had asked first to work on this bug, so let's give him a
> chance to work on it. Let me know if you need any help on finding some other
> good-first-bug to take

hey that's cool! I will contact you in IRC! :D thanks for reaching out

Comment 11

[TomS [:toms]]

Adding that line doesn't seem to stop it from focusing on the label. Unless I'm doing something wrong here

https://imgur.com/vbjZUT4

Comment 12

[TomS [:toms]]

Attachment: Bug 1508724 - Checkbox label for Shockwave Flash in about:addons plugins has improper tab_focus

Hid the checkbox-label-box from the Shockwave plugin to correct the tab focus.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad217069266e
Checkbox label for Shockwave Flash in about:addons plugins has improper tab_focus r=Felipe

Comment 14

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/ad217069266e

Comment 15

[Caitlin Neiman [:caitmuenster]]

Thanks so  much, Tom! Your contribution has been added to our recognition wiki (https://wiki.mozilla.org/Add-ons/Contribute/Recognition) and your Mozillians profile has been vouched. :) 

Welcome onboard! We look forward to seeing you around the project.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Did you want to nominate this for Beta uplift?

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Nope, not necessary as it's not really a regression, it has existed one way or another since we added that checkbox

Comment 18

[TomS [:toms]]

(In reply to Caitlin Neiman [:caitmuenster] from comment #15)
> Thanks so  much, Tom! Your contribution has been added to our recognition
> wiki (https://wiki.mozilla.org/Add-ons/Contribute/Recognition) and your
> Mozillians profile has been vouched. :) 
> 
> Welcome onboard! We look forward to seeing you around the project.

Thanks a lot Caitlin!

Comment 19

[David Olah]

I checked the issue on Nightly 66.0a1 (2019-01-03). 

- On Windows 10, the checkbox can be marked with tab->space, but it is not highlighted.
- On Ubuntu 16.04, the checkbox can be marked with tab->space, it is not highlighted and the dotted line appears right next to the checkbox as seen in the printscreen attached.

Should I create a new bug and mark this as verified or reopen this one?

Comment 20

[David Olah]

Attachment: checkbox.png

Comment 21

[TomS [:toms]]

I'm using the same Ubuntu version but the line is not present. I don't know if this should be reopened or not, maybe @Felipe can let us know.

Comment 22

[:Felipe Gomes (needinfo for replies!)]

(In reply to David Olah from comment #19)

I checked the issue on Nightly 66.0a1 (2019-01-03).

• On Windows 10, the checkbox can be marked with tab->space, but it is not
  highlighted.
• On Ubuntu 16.04, the checkbox can be marked with tab->space, it is not
  highlighted and the dotted line appears right next to the checkbox as seen
  in the printscreen attached.

Should I create a new bug and mark this as verified or reopen this one?

The screenshot you provided is not from Nightly, but it's from a release version of Firefox. Are you sure you checked the right version?

This bug was fixed during the 66 cycle, so it's only gonna make it to Beta at the end of January and to Release in the beginning of March.

Comment 23

[David Olah]

Indeed the screenshot is made on release. The dotted line appears only there and not on Nightly.

The biggest concern and question is that, if the checkbox is not highlighted when it is selected with tab, even if it can be checked with space at the moment, the issue should be closed or not?

Comment 24

[Cristian Fogel [:cfogel]]

Marking the issue as verified since the reported issue has been fixed. Can confirm with 67.0.4 that there is no extra lines displayed.

Will still file a new bug since, as per David's concern filed bug 1562836, since I agree with him.
Even tho it's a small cosmetic change it's not consistent with the rest of the browser elements.