854492 - The Switches style sheet is hiding <input> elements that it should not hide

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[Jonathan Watt [:jwatt]]

The first issue to be resolved before bug 854469 can be fixed is some styling in shared/style/switches.css that seems to be far too general. Specifically this |opacity: 0;| line is causing problems:

https://github.com/mozilla-b2g/gaia/blob/940124b970/shared/style/switches.css#L16

label:not([for]) input {
  margin: 0;
  opacity: 0;
  position: absolute;
  top: 0;
  left: 0;
}

(This issue seems to have led people to add bogus empty for="" attributes to <label> elements in order to avoid that, but that seems like the wrong thing to do to me. Really, the style sheet should be fixed to apply only to those things that it should apply to.)

With regards to <input type=range>, until recently we wanted to hide any <input type=range> elements in gaia because we use a polyfill hack to display sliders, or what are called seekbars in gaia:

http://telefonicaid.github.com/Gaia-UI-Building-Blocks/index.html#widgets/seekbars/

The polyfill hack has hidden this bug in shared/style/switches.css from being seen with <input type=range> until now.

Now that <input type=range> is enabled (bug 841948) we want to get rid of this polyfill hack though, but before that can happen, this bug needs to be addressed.

Right now the |opacity: 0;| line is hiding all the <input>'s with type="range" in:

https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/index.html

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Looks to me like this is what we should be doing.

Comment 2

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 729104 [details] [diff] [review]
patch

Jonathan,
in Building Blocks we have 2 different styles for checkboxes, regular ones (type="checkbox") and switches (type="checkbox" data-type="switch").
You are adding extra classes here, which we try to avoid.
I would feel more confortable replacing: 
label:not([for]) input {...}
for:
label:not([for]) input[type="checkbox"],
label:not([for]) input[type="radio"] {...}
What you are proposing means we have to go through all apps and add that extra class to all checkboxes.

Comment 3

[Jonathan Watt [:jwatt]]

Hi Arnau. Why do you try to avoid adding classes? That seems to be _exactly_ what classes are for, to say clearly "this is an element of classification X" so that you can apply the styling that elements of type X should have specifically to those elements. Using selectors that depend on other arbitrary conditions of X elements that happen to be true seems to create a hazard. Other coders can easily get unlucky and write code that happens to have those same conditions, resulting in their elements getting unexpected and undesirable styling due to it becoming bycatch of those selectors. It may be more upfront work to add appropriate classes to elements in all the necessary places, but longer term the code would be a lot healthier for it.

For my purposes:

label:not([for]) input[type="checkbox"],
label:not([for]) input[type="radio"] {...}

would solve this bug so I guess that's what I'll do if that's what you want. I'm just concerned that the :not([for]) part of the selector continues to create an (albeit reduced) bycatch hazard.

Comment 4

[Jonathan Watt [:jwatt]]

Pull request is here: https://github.com/mozilla-b2g/gaia/pull/8821

Comment 5

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

I have no problem using as many classes as needed, but when we created the Building blocks, we decided to use aria roles or data-types for defining elements when possible, and that approach is what I try to defend.
Thanks for the PR! r+ from my side, let's hear from Kaze, if he has any concern.

Comment 6

[Fabien Cazenave [:kaze]]

Thanks Arnau. As a side note, we’ve (obviously) reaching the limits of this approach, and we should probably start thinking of a more flexible one for v2 (e.g. OOCSS + proper taxonomy)…

This latest patch looks much safer to me (less risk to break existing stuff).

Comment 7

[Fabien Cazenave [:kaze]]

Comment on attachment 729104 [details] [diff] [review]
patch

Thanks for this patch!

Comment 8

[Fabien Cazenave [:kaze]]

https://github.com/jwatt/gaia/commit/40a1295ac09bed8ad0e8e653fd564c9dc115a52f

Comment 9

[Jonathan Watt [:jwatt]]

Thanks guys!