1033392 - [Regression] Disabled items in Developer HUD remain active

Status: RESOLVED FIXED

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

Description

[Jan Keromnes [:janx]]

The Developer HUD was accidentally using the download manager's `.disabled` CSS class. Since bug 1017509 removed it, items in the HUD stay active even if the HUD is disabled.

Comment 1

[Jan Keromnes [:janx]]

Attachment: Disabled items in Developer HUD remain active. r=21

Comment 2

[Jan Keromnes [:janx]]

Comment on attachment 8449450 [details] [diff] [review]
Disabled items in Developer HUD remain active. r=21

Review of attachment 8449450 [details] [diff] [review]:
-----------------------------------------------------------------

I am also drive-by fixing some `.disabled` classes to `aria-disabled=true` attributes, continuing the accessibility work of bug 914886.

Before I ask Vivien to do the global review:
- Kevin, could you please look at date_time.js?
- Arthur, could you please look at wifi_select_certificate_file.js?

Comment 3

[Jan Keromnes [:janx]]

(Please note that I am changing the "disabled" opacity from 0.6 to 0.3 to make it more obvious).

Comment 4

[Kevin Grandon :kgrandon]

Comment on attachment 8449450 [details] [diff] [review]
Disabled items in Developer HUD remain active. r=21

Review of attachment 8449450 [details] [diff] [review]:
-----------------------------------------------------------------

I think the accessibility guys would be able to provide better feedback than I, so redirecting.

Comment 5

[Eitan Isaacson [:eeejay]]

Comment on attachment 8449450 [details] [diff] [review]
Disabled items in Developer HUD remain active. r=21

Review of attachment 8449450 [details] [diff] [review]:
-----------------------------------------------------------------

I feel mixed about this.

We are learning the hard way that using aria attributes in selectors for css is making it difficult to tweak the accessibility functionality without messing with visual styling.

So, if you are intent to drive-by-fix this, maybe re-introducing the disabled class, and removing aria-disabled from CSS (but keeping it in JS) is the way to go :)

Removing feedback flag since I don't feel strongly enough to f- this and block Progress.

::: apps/settings/style/settings.css
@@ +203,5 @@
> +
> +*[aria-disabled="true"] {
> +  opacity: 0.3;
> +  pointer-events: none;
> +}

How does this co-exist with other, similar rules on children of disabled elements?
https://github.com/eeejay/gaia/blob/master/apps/settings/style/lists.css#L59

Comment 6

[Jan Keromnes [:janx]]

Thanks for the accessibility feedback!

(In reply to Eitan Isaacson [:eeejay] from comment #5)
> I feel mixed about this.
> 
> We are learning the hard way that using aria attributes in selectors for css
> is making it difficult to tweak the accessibility functionality without
> messing with visual styling.

I was told something similar, thanks for confirming.

> So, if you are intent to drive-by-fix this, maybe re-introducing the
> disabled class, and removing aria-disabled from CSS (but keeping it in JS)
> is the way to go :)
> 
> Removing feedback flag since I don't feel strongly enough to f- this and
> block Progress.

I think I'll do that. Since you are the author of bug 914886 (my reference for this drive-by fix) I trust you enough to know if this is a good approach or not. :)

> ::: apps/settings/style/settings.css
> @@ +203,5 @@
> > +
> > +*[aria-disabled="true"] {
> > +  opacity: 0.3;
> > +  pointer-events: none;
> > +}
> 
> How does this co-exist with other, similar rules on children of disabled
> elements?
> https://github.com/eeejay/gaia/blob/master/apps/settings/style/lists.css#L59

I believe setting `opacity` and `pointer-events` on a container affects the children as well, hence my removal of these rules from the children styles. However, if a disabled element is contained in another disabled element, the opacities are combined.

Comment 7

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8449450 [details] [diff] [review]
Disabled items in Developer HUD remain active. r=21

Redirect to EJ as he is currently working on the wifi panels.

Comment 8

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Just learned from :eeejay's comment, so what's the current progress of this bug ? 

:janx, are you going to follow :eeejay's comment to create a new disabled class for it or ?

Comment 9

[Jan Keromnes [:janx]]

(In reply to EJ Chen [:eragonj][:小龍哥] from comment #8)
> Just learned from :eeejay's comment, so what's the current progress of this
> bug ?

I'll redo the patch using a disabled class instead, but I was hoping for all the owners of the different parts of the code I'm changing to verify I'm not breaking anything.

I'll upload a new patch next week.

> :janx, are you going to follow :eeejay's comment to create a new disabled
> class for it or ?

I think so, but I will try to avoid using [aria-disabled="true"] in CSS selectors.

Comment 10

[Jan Keromnes [:janx]]

Attachment: gaia pull request

Comment 11

[Jan Keromnes [:janx]]

Attachment: screenshot: clearer distinction of devtools in developer HUD

Comment 12

[Jan Keromnes [:janx]]

Comment on attachment 8467742 [details] [review]
gaia pull request

New patch, not using "aria-disabled" in CSS rules.

- Vivien, could you please review the Developer HUD change? (see screenshot)
- EJ, could you please double-check I didn't break ".wifi-certificate-files-List" or "#dateTime"?

Thanks!

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8467742 [details] [review]
gaia pull request

Sounds safe HTML + CSS changes.

Comment 14

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8467742 [details] [review]
gaia pull request

Looks good to me ! THanks.

Comment 15

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: screen.png

BTW, I am not sure why I would get this screenshot when toggling `developer tools` ? Does this happen there ?

Comment 16

[Jan Keromnes [:janx]]

Thanks!

The screenshot is a bit worrying. Once I did see a grey shape like this further down in the menu, and assumed it was a graphics bug, because when I long-pressed home and then re-selected the settings menu it went away.

I'll hold off a bit before landing to investigate a bit more. This problem shouldn't reproduce too easily!

Comment 17

[Jan Keromnes [:janx]]

Looks like a graphics bug because it happens in Date & Time as well, seemingly at random after a few on/off switches (filed bug 1050813).

The problem only appeared after my change though, so I guess it's because I made the opacity apply to a container instead of its child elements. I'll work around this by re-targeting child elements instead of parents (which is also better for performance):

--- a/apps/settings/style/settings.css
+++ b/apps/settings/style/settings.css
@@ -210,8 +210,12 @@
  * Disabled items
  */
 
-.disabled {
+.disabled a,
+.disabled p,
+.disabled h2,
+.disabled label,
+.disabled select {
   opacity: 0.6;
   pointer-events: none;
 }

I also applied Developer HUD "app memory" item `.disabled` class on `li` instead of `li > label > span`, and re-tweaked Developer HUD color preview opacity to 0.4.

Comment 18

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Let me help Jan to put links here : 

This patch has been merged into Gaia/master: https://github.com/mozilla-b2g/gaia/commit/e0144a123949e826e3f316bb0be455ae4d6d69f8