1997198 - Add moz-card to setting-group

Status: RESOLVED FIXED

(Firefox :: Settings UI, enhancement, P3)

Description

[Tim Giles [:tgiles]]

Attachment: startup setting group with moz-card wrapper

The new Settings design feature a card container for each setting group. We should add moz-card to setting-group in order to achieve this. There are some open questions though:

• Should we convert the <groupbox> -> <label> -> <h2|h1> elements into the <moz-card> heading or the <moz-fieldset> label?
  • startup-header
  • preferences-web-appearance-header
  • update-application-allow-description, not an <h2>, simply a <label>
  • performance-title, <h1>
  • browsing-title, <h1>
  • network-proxy-connection-description, a <span> within a <description>
  • sitedata-header
  • autofill-payment-methods-title
  • autofill-addresses-title
  • security-browsing-protection
  • httpsonly-header
  • There may be others but these were the ones I easily noticed
• Should we address the "headings, labels, etc" as a follow-up task or tasks?
  • It would allow us to deliver an improvement to the current UI, as far as I can tell

Attaching a screenshot showing the "Startup" setting group with a moz-card wrapper. The following is a WIP patch that can be used to see what this change would bring:

diff --git a/browser/components/preferences/widgets/setting-group/setting-group.mjs b/browser/components/preferences/widgets/setting-group/setting-group.mjs
index 34d1374120b7..7a2552e9bc9a 100644
--- a/browser/components/preferences/widgets/setting-group/setting-group.mjs
+++ b/browser/components/preferences/widgets/setting-group/setting-group.mjs
@@ -112,15 +112,26 @@ export class SettingGroup extends MozLitElement {
     if (!this.config) {
       return "";
     }
-    return html`<moz-fieldset
-      data-l10n-id=${ifDefined(this.config.l10nId)}
-      .headingLevel=${this.config.headingLevel}
-      .supportPage=${ifDefined(this.config.supportPage)}
-      @change=${this.onChange}
-      @click=${this.onClick}
-      @visibility-change=${this.handleVisibilityChange}
-      >${this.config.items.map(item => this.itemTemplate(item))}</moz-fieldset
+    return html`<moz-card
+      ><moz-fieldset
+        data-l10n-id=${ifDefined(this.config.l10nId)}
+        .headingLevel=${this.config.headingLevel}
+        .supportPage=${ifDefined(this.config.supportPage)}
+        @change=${this.onChange}
+        @click=${this.onClick}
+        @visibility-change=${this.handleVisibilityChange}
+        >${this.config.items.map(item => this.itemTemplate(item))}</moz-fieldset
+      ></moz-card
     >`;
+    // return html`<moz-fieldset
+    //   data-l10n-id=${ifDefined(this.config.l10nId)}
+    //   .headingLevel=${this.config.headingLevel}
+    //   .supportPage=${ifDefined(this.config.supportPage)}
+    //   @change=${this.onChange}
+    //   @click=${this.onClick}
+    //   @visibility-change=${this.handleVisibilityChange}
+    //   >${this.config.items.map(item => this.itemTemplate(item))}</moz-fieldset
+    // >`;
   }
 }
 customElements.define("setting-group", SettingGroup);

Comment 1

[Mark Striemer [:mstriemer]]

Bug 1994888 is on file to convert the headings of the areas that are still relying on groupbox. If that's done then we shouldn't need to worry about headings here

One thing we should consider is that updating setting-group to use a card could lead to inconsistent UI when they're mixed with groupbox elements since those aren't using cards.

We do already have sub-panes being generated by setting-pane though, which could be a good candidate for when to enable the cards since they're on their own page and we only allow setting-group elements in there, we won't hit any inconsistency within that area.

Comment 2

[Mark Striemer [:mstriemer]]

This should be done behind the browser.settings-redesign.enabled pref for the main panes and right away for sub-panes

Comment 3

[Mark Striemer [:mstriemer]]

Attachment: Bug 1997198 - Part 1: Wrap setting-groups in moz-cards r?#settings-reviewers

Comment 4

[Mark Striemer [:mstriemer]]

Attachment: Bug 1997198 - Part 2: Trim leading on moz-fieldsets with headings r?#recomp-reviewers

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/5d62ceaa0828
https://hg.mozilla.org/integration/autoland/rev/785d618fb94f
Part 1: Wrap setting-groups in moz-cards r=mconley
https://github.com/mozilla-firefox/firefox/commit/66c0bf0d3c3f
https://hg.mozilla.org/integration/autoland/rev/471c019a40bd
Part 2: Trim leading on moz-fieldsets with headings r=desktop-theme-reviewers,tgiles

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a8fdc928a8c1
https://hg.mozilla.org/integration/autoland/rev/83896b2c92b7
Revert "Bug 1997198 - Part 2: Trim leading on moz-fieldsets with headings r=desktop-theme-reviewers,tgiles" for causing mochitest failures on test_setting_group.

Comment 7

[Cosmin Sabou [:CosminS]]

Backed out for causing several failures.

Push with failures

Failure logs:

• TEST-UNEXPECTED-FAIL | browser/components/preferences/tests/chrome/test_setting_group.html | First child is a fieldset - got "moz-card", expected "moz-fieldset" - https://treeherder.mozilla.org/logviewer?job_id=542674205&repo=autoland&task=To9pfs9nTsOQevagssX5sw.0
• TEST-UNEXPECTED-FAIL | browser/tools/mozscreenshots/preferences/browser_preferences.js | capture - Unexpected exception in [ prefsGeneral-browsingGroup ]: TypeError: can't access property "scrollIntoView" - https://treeherder.mozilla.org/logviewer?job_id=542680506&repo=autoland&task=AyRFsgu6ShSZTBMZWs7AjA.0
• TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_storagePressure_notification.js - Element should not be null, when checking visibility - https://treeherder.mozilla.org/logviewer?job_id=542675018&repo=autoland&task=dh94aMWxTN6FT9KELcXTVA.0
• also this TV - https://treeherder.mozilla.org/logviewer?job_id=542674169&repo=autoland&task=XkhzfCiHSC-Nm6J33QWSjg.0

Backout link

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c7be064d4955
https://hg.mozilla.org/integration/autoland/rev/36759c1fbe2b
Part 1: Wrap setting-groups in moz-cards r=mconley
https://github.com/mozilla-firefox/firefox/commit/b40dd2405295
https://hg.mozilla.org/integration/autoland/rev/1d73b1588c81
Part 2: Trim leading on moz-fieldsets with headings r=desktop-theme-reviewers,tgiles

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a02d8e0df2da
https://hg.mozilla.org/integration/autoland/rev/7ba4ae8ea1a1
Revert "Bug 1997198 - Part 2: Trim leading on moz-fieldsets with headings r=desktop-theme-reviewers,tgiles" for causing ESlint failure

Comment 10

[Cristina Horotan [:chorotan]]

Backed out for causing ESlint failure and bc failures

Backout link

Push with failures

Failure log
Failure log bc
Failure log ss

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/0bf8cd844b80
https://hg.mozilla.org/integration/autoland/rev/b31ebb86cb87
Part 1: Wrap setting-groups in moz-cards r=mconley
https://github.com/mozilla-firefox/firefox/commit/a2e5eeff754b
https://hg.mozilla.org/integration/autoland/rev/13d496847602
Part 2: Trim leading on moz-fieldsets with headings r=desktop-theme-reviewers,tgiles

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/b31ebb86cb87
https://hg.mozilla.org/mozilla-central/rev/13d496847602

Comment 13

[Mark Striemer [:mstriemer]]

[Tracking Requested - why for this release]: This is used in bug 2005804 to create the cards on the AI Controls page

I rebased this onto beta locally and it had a minor merge conflict but worked fine (doesn't automatically add cards since the patch that tells it to use cards isn't on beta, but that's fine, manually setting it worked). I'll rebase the stack from bug 2005804 (and others) onto this on beta when they hit central and put up a revision to uplift. There shouldn't be any issue with landing them separately though, so we could do that too

Comment 14

[Mark Striemer [:mstriemer]]

Attachment: Bug 1997198 - Part 1: Wrap setting-groups in moz-cards

Original Revision: https://phabricator.services.mozilla.com/D275335

Comment 15

[Mark Striemer [:mstriemer]]

Attachment: Bug 1997198 - Part 2: Trim leading on moz-fieldsets with headings

Original Revision: https://phabricator.services.mozilla.com/D275336

Comment 16

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/bd44969b169b
https://hg.mozilla.org/releases/mozilla-beta/rev/148ed2b369e6
https://github.com/mozilla-firefox/firefox/commit/a03a17f9dd67
https://hg.mozilla.org/releases/mozilla-beta/rev/9ea897e23a3d