[Regression] Disabled items in Developer HUD remain active

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: janx, Assigned: janx)

Tracking

({regression})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8449450 [details] [diff] [review]
Disabled items in Developer HUD remain active. r=21
(Assignee)

Comment 2

4 years ago
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?
Attachment #8449450 - Flags: feedback?(kgrandon)
Attachment #8449450 - Flags: feedback?(arthur.chen)
(Assignee)

Comment 3

4 years ago
(Please note that I am changing the "disabled" opacity from 0.6 to 0.3 to make it more obvious).
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.
Attachment #8449450 - Flags: feedback?(kgrandon) → feedback?(eitan)
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
Attachment #8449450 - Flags: feedback?(eitan)
(Assignee)

Comment 6

4 years ago
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 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.
Attachment #8449450 - Flags: feedback?(arthur.chen) → feedback?(ejchen)

Updated

4 years ago
Keywords: regression
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 ?
Flags: needinfo?(janx)
(Assignee)

Comment 9

4 years ago
(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.
Flags: needinfo?(janx)
(Assignee)

Comment 10

4 years ago
Created attachment 8467742 [details] [review]
gaia pull request
Attachment #8449450 - Attachment is obsolete: true
(Assignee)

Comment 11

4 years ago
Created attachment 8468486 [details]
screenshot: clearer distinction of devtools in developer HUD
(Assignee)

Comment 12

4 years ago
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!
Attachment #8467742 - Flags: review?(ejchen)
Attachment #8467742 - Flags: review?(21)
Comment on attachment 8467742 [details] [review]
gaia pull request

Sounds safe HTML + CSS changes.
Attachment #8467742 - Flags: review?(21) → review+
Comment on attachment 8467742 [details] [review]
gaia pull request

Looks good to me ! THanks.
Attachment #8467742 - Flags: review?(ejchen) → review+
Created attachment 8469785 [details]
screen.png

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

Comment 16

4 years ago
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!
(Assignee)

Comment 17

4 years ago
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.
(Assignee)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.