Closed Bug 1541045 Opened 5 years ago Closed 5 years ago

Drag space checkbox should be grey out when not accessible

Categories

(Toolkit :: UI Widgets, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla68
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- unaffected
firefox67 --- verified
firefox68 --- verified

People

(Reporter: asoncutean, Assigned: surkov)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

Attached video 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]:

[Additional Notes]:

  • When in Light or Dark theme, Drag space label turns grey out when unsuasable.
  • Ubuntu is not affected.
Has Regression Range: --- → yes

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

Does this affect all checkboxes?

Flags: needinfo?(surkov.alexander)
Priority: -- → P2
No longer blocks: 1455433
Regressed by: 1455433

(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

Flags: needinfo?(surkov.alexander)

(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?

(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.

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?

Flags: needinfo?(surkov.alexander)

(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.

Flags: needinfo?(surkov.alexander)

(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?

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

Flags: needinfo?(anca.soncutean)

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.

(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...

(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).

Flags: needinfo?(anca.soncutean)

(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.

Assignee: nobody → surkov.alexander
Component: XUL → XUL Widgets
Product: Core → Toolkit
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/70ed6fa7f921
Drag space checkbox should be grey out when disabled r=Gijs
See Also: → 1543733
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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

Flags: needinfo?(surkov.alexander)

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
Flags: needinfo?(surkov.alexander)
Attachment #9057508 - Flags: approval-mozilla-beta?

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.

Attachment #9057508 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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 .

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: