Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Clicking labels should trigger click of associated element #8243

Closed
jonathansampson opened this issue Apr 12, 2017 · 2 comments
Closed

Clicking labels should trigger click of associated element #8243

jonathansampson opened this issue Apr 12, 2017 · 2 comments

Comments

@jonathansampson
Copy link
Collaborator

jonathansampson commented Apr 12, 2017

Test plan

#10470 (comment)


  • Did you search for similar issues before submitting this one?
    Yes

  • Describe the issue you encountered:
    Throughout Brave, many controls have an associated label. Clicking these labels doesn't trigger a click of the associated element.

  • Platform (Win7, 8, 10? macOS? Linux distro?):
    Desktop

  • Brave Version (revision SHA):
    61ae1a7

  • Steps to reproduce:

    1. Open Brave Shields on GitHub.com
    2. Click HTTPS Everywhere label
  • Actual result:
    Nothing happens

  • Expected result:
    The associated element's state toggles

  • Will the steps above reproduce in a fresh profile? If not what other info can be added?
    Yes

  • Is this an issue in the currently released version?
    Yes

  • Can this issue be consistently reproduced?
    Yes

  • Extra QA steps:

    1. Match Case in Find on Page UI
    2. Many places throughout about:preferences
  • Screenshot if needed:
    clickable-labels

@bsclifton
Copy link
Member

Great first bug 🐛 😄 👍

Changes can be made to the SwitchControl component

@petemill
Copy link
Member

petemill commented Aug 14, 2017

I just took a look at this. Making the change to the switchControl to also perform the onClick for the text does fix this for some occurrences (e.g. the Brave panel, the Clear browsing data modal and the messageBox component).

However, in some other uses, a wrapper component is used which does not provide the label text as a prop to the switchControl component. These components render their own <label /> as a sibling to the switch. They expect switchControl to render an 'id' attribute, and add a 'for' attribute on the label, most likely expecting the click to happen that way. switchControl is not (anymore?) rendering an 'id' attribute. It's probably not the best idea anyway since it creates an expectation that a component will render html in a specific way, which of course can change (as it perhaps has in this case). The git history of switchControl (and others) was not preserved during the folder structure change, so I wasn't immediately able to see what came first. It's happening in these uses:

Since switchControl supports rendering text identically the same as these components are doing with their own <label /> element, including localization, it's probably best to convert these uses to the built-in text functionality. I also found that the entirety of the styles for switchControl is duplicated in less/about/preferences.less, with some additions and conflicting layout style that seem minor at first glance (inline-block vs flex), so I'll look at refactoring that and removing the duplication.

petemill added a commit to petemill/browser-laptop that referenced this issue Aug 14, 2017
…ed state

Partial resolution for brave#8243 since many usages do not use the built-in text
but render their own text separately. A subsequent commit will address converting these
usages.

This also converts text from span to label for accessbility and more appropriate renderer defaults (the cursor).
petemill added a commit to petemill/browser-laptop that referenced this issue Aug 14, 2017
closes brave#8243 as this ensures all remaining uses of switchControl gain
the built-in label click handler.

covers the case-sensitivity toggle in findbar, as well as all the switches
on the settings page.
@luixxiul luixxiul removed this from the 1.1.0 milestone Aug 14, 2017
@luixxiul luixxiul added this to the 0.21.x (Nightly Channel) milestone Aug 15, 2017
dfperry5 pushed a commit to dfperry5/browser-laptop that referenced this issue Aug 18, 2017
…ed state

Partial resolution for brave#8243 since many usages do not use the built-in text
but render their own text separately. A subsequent commit will address converting these
usages.

This also converts text from span to label for accessbility and more appropriate renderer defaults (the cursor).
dfperry5 pushed a commit to dfperry5/browser-laptop that referenced this issue Aug 18, 2017
closes brave#8243 as this ensures all remaining uses of switchControl gain
the built-in label click handler.

covers the case-sensitivity toggle in findbar, as well as all the switches
on the preferences page.
@bbondy bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel) Oct 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.