1541045 - Drag space checkbox should be grey out when not accessible

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect, P2)

Description

[Anca Soncutean, Desktop QA]

Attachment: screencast issue.mov

[Affected versions]:

• 67.0b7
• 68.0a1 (2019-04-01)

[Affected platforms]:

• Win 10 x64
• Mac OS 10.10
• macOS 10.13

[Steps to reproduce]:

1. Launch Firefox
2. Go to Menu - Customize
3. Enable Title bar
4. Observe the Drag space checkbox

[Expected result]:

• Drag space checkbox should be greyed out

[Actual result]:

• Drag space checkbox doesn’t change its aspect, misleading the user.

[Regression range]:

• Last good revision: 02c49a3e368dd98c05ff61978f80d98a20464f71
• First bad revision: 623c6ca6ed6cf8128cebf9944524f3be13e0d4d2
• Pushlog:
  https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=02c49a3e368dd98c05ff61978f80d98a20464f71&tochange=623c6ca6ed6cf8128cebf9944524f3be13e0d4d2

[Additional Notes]:

• When in Light or Dark theme, Drag space label turns grey out when unsuasable.
• Ubuntu is not affected.

Comment 1

[Neil Deakin]

Regression from bug 1455433 presumably. Alex, can you take a look?

Does this affect all checkboxes?

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Neil Deakin (away march 22 - 31) from comment #1)

Regression from bug 1455433 presumably. Alex, can you take a look?

It appears the bug is caused by .customizationmode-checkbox:not(:-moz-lwtheme) style applied [1] to a disabled checkbox. I'm not sure what -moz-lwtheme pseudo style is about and how bug 1455433 affects on it, but I suppose it could be fixed by adding [disabled] rule into css. I'd be good to have a thumb up from a peer on this approach.

Does this affect all checkboxes?

I think it is specific to Customize panel checkboxes.

[1] https://searchfox.org/mozilla-central/source/browser/themes/shared/customizableui/customizeMode.inc.css#73

Comment 4

[:Gijs (he/him)]

(In reply to alexander :surkov (:asurkov) from comment #3)

(In reply to Neil Deakin (away march 22 - 31) from comment #1)

Regression from bug 1455433 presumably. Alex, can you take a look?

It appears the bug is caused by .customizationmode-checkbox:not(:-moz-lwtheme) style applied [1] to a disabled checkbox. I'm not sure what -moz-lwtheme pseudo style is about and how bug 1455433 affects on it, but I suppose it could be fixed by adding [disabled] rule into css. I'd be good to have a thumb up from a peer on this approach.

The -moz-lwtheme pseudo would match if the document is using a lightweight theme (ie dark/light theme, like in these steps). That didn't change, I don't think. Why did this work prior to bug 1455433?

Comment 5

[alexander :surkov (:asurkov)]

(In reply to :Gijs (he/him) from comment #4)

It appears the bug is caused by .customizationmode-checkbox:not(:-moz-lwtheme) style applied [1] to a disabled checkbox. I'm not sure what -moz-lwtheme pseudo style is about and how bug 1455433 affects on it, but I suppose it could be fixed by adding [disabled] rule into css. I'd be good to have a thumb up from a peer on this approach.

The -moz-lwtheme pseudo would match if the document is using a lightweight theme (ie dark/light theme, like in these steps). That didn't change, I don't think. Why did this work prior to bug 1455433?

All I can see is this style is not applied in release Firefox, when the checkbox is disabled and dark/light theme is active. Assuming that DevTool are trustworthy, I think it means that -moz-lwtheme pseudo is not set. If it's indeed a cause, then I can't see a correlation between -moz-lwtheme pseudo and bug 1455433 honestly.

Comment 6

[Neha Kochar [:neha]]

Alex, are you the correct assignee for this regression? And also, do you know if this will get a fix in time to uplift to 67 beta?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Neha Kochar [:neha] from comment #6)

Alex, are you the correct assignee for this regression? And also, do you know if this will get a fix in time to uplift to 67 beta?

I'm definitely up to help to with the fix, but I also need some help in unwinding the issue, since there's no yet understanding what caused the bug (comment #4). I think there's a workaround (comment #3), and I suppose it can be a candiadate for uplifting to 67 if the root cause cannot be identified prior this time.

Comment 8

[:Gijs (he/him)]

(In reply to alexander :surkov (:asurkov) from comment #5)

(In reply to :Gijs (he/him) from comment #4)

It appears the bug is caused by .customizationmode-checkbox:not(:-moz-lwtheme) style applied [1] to a disabled checkbox. I'm not sure what -moz-lwtheme pseudo style is about and how bug 1455433 affects on it, but I suppose it could be fixed by adding [disabled] rule into css. I'd be good to have a thumb up from a peer on this approach.

The -moz-lwtheme pseudo would match if the document is using a lightweight theme (ie dark/light theme, like in these steps). That didn't change, I don't think. Why did this work prior to bug 1455433?

All I can see is this style is not applied in release Firefox, when the checkbox is disabled and dark/light theme is active. Assuming that DevTool are trustworthy, I think it means that -moz-lwtheme pseudo is not set. If it's indeed a cause, then I can't see a correlation between -moz-lwtheme pseudo and bug 1455433 honestly.

So to be clear... document.documentElement.matches(":-moz-lwtheme") should be true when either the light or dark theme is active.

It should be false when the default theme is active.

document.documentElement.matches(":not(:-moz-lwtheme)")

should have the opposite behaviour, because of the :not().

Anyway, testing on 66 nightly (from ./mach mozregression --launch 66), I see the same bug in terms of the text colour, prior to your changes, on both windows and mac. This just seems like a bug, that isn't a regression, that we could fix by adding :not([disabled]) to the selector that applies the foreground colour ie https://searchfox.org/mozilla-central/rev/dd7e27f4a805e4115d0dbee70e1220b23b23c567/browser/themes/shared/customizableui/customizeMode.inc.css#70 .

However, before your changes, there was a visual difference in the actual checkbox box itself. I believe that's what's regressed here, not the text colour. Hopefully that helps?

Comment 9

[:Gijs (he/him)]

Anca, can you clarify on what OS you found the regression window, and provide a before/after screenshot?

Comment 10

[:Gijs (he/him)]

The lack of disabled state in the checkbox 'box' itself is caused by the fact that we no longer seem to propagate the 'disabled' attribute onto the <image>, so the -moz-appearance of the checkbox doesn't know it needs to show a "disabled" appearance instead of the normal one.

Comment 11

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #10)

The lack of disabled state in the checkbox 'box' itself is caused by the fact that we no longer seem to propagate the 'disabled' attribute onto the <image>, so the -moz-appearance of the checkbox doesn't know it needs to show a "disabled" appearance instead of the normal one.

And I'd expect that to affect other disabled checkboxes elsewhere in our UI, so I'm tempted to suggest that should affect our decisionmaking about how important a fix for 67 is...

Comment 12

[Anca Soncutean, Desktop QA]

(In reply to :Gijs (he/him) from comment #9)

Anca, can you clarify on what OS you found the regression window, and provide a before/after screenshot?

I used macOS 10.13 to find the regression range. Here is a screencast made on a "good" build - 67.0a1 (2019-03-01) and an affected one - 68.0a1 (2019-04-10).

Comment 13

[:Gijs (he/him)]

(In reply to Anca Soncutean [:Anca], Desktop Release QA from comment #12)

(In reply to :Gijs (he/him) from comment #9)

Anca, can you clarify on what OS you found the regression window, and provide a before/after screenshot?

I used macOS 10.13 to find the regression range. Here is a screencast made on a "good" build - 67.0a1 (2019-03-01) and an affected one - 68.0a1 (2019-04-10).

OK, so this does also show the text colour being the same, only the checkbox changes. The explanation for the checkbox itself is in comment #10.

:surkov: I assume that's enough to help you fix this, let me know if there's anything else I can help clarify.

Comment 14

[alexander :surkov (:asurkov)]

Attachment: Bug 1541045 - Drag space checkbox should be grey out when disabled

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/70ed6fa7f921
Drag space checkbox should be grey out when disabled r=Gijs

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/70ed6fa7f921

Comment 17

[Pascal Chevrel:pascalc]

Alexander, the fix is minimal and fixes a 67 regression, do you want to request an uplift to beta?

Comment 18

[alexander :surkov (:asurkov)]

Comment on attachment 9057508 [details]
Bug 1541045 - Drag space checkbox should be grey out when disabled

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1455433
• User impact if declined: Firefox UI defect in Customize toolbar dialog
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): restored behavior changed by regressing bug
• String changes made/needed: no

Comment 19

[Pascal Chevrel:pascalc]

Comment on attachment 9057508 [details]
Bug 1541045 - Drag space checkbox should be grey out when disabled

Low risk fix to a 67 regression, uplift approved for 67 beta 12, thanks.

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/550ae4e7da5d

Comment 21

[Anca Soncutean, Desktop QA]

This issue is verified fixed with Fx 67.0b12 and Fx 68.0a1 (2019-04-18) on Windows 10 x64, macOS 10.13 and Ubuntu 18.04 x64 .