1038291 - Investigate the new InContent General pane layout

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Richard Marti (:Paenglab)]

Bug to update the new General pane layout. For reference see attachment 8439538 [details] and attachment 8439539 [details].

Comment 1

[Richard Marti (:Paenglab)]

Attachment: mainGerman.png

Michael, I checked the German Nightly and saw they are using really long labels which makes it hard to follow your proposal. What do you think how can this styling be applied on locales with such long labels?

The only change I did on this screenshot was aligning the two first rows to show the issues better. The pane is on full width of 800px.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Richard Marti (:Paenglab) from comment #1)

Let's make the buttons wrap to the next line instead of breaking the grid. For German, it can wrap like the following:

When Nightly starts: [Show my home page                    ↓]
          Home page: [Nightly start page                    ]
                     [Use current pages] [Use bookmark...]
                     [Restore to Default]

The above diagram shows that there may be blank space at the end of the first row of buttons. It would be even better if you can use flex-grow to get the "Use current pages" and "Use bookmark..." buttons to grow to consume the available space in the row so there is no gap of space.

Richard, is this enough for you to pick up this bug again?

Comment 3

[Richard Marti (:Paenglab)]

Yes, I can try doing this.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Great, thanks!

Comment 5

[Richard Marti (:Paenglab)]

Attachment: GeneralPaneLayout.patch

This patch wraps the buttons but I have two problems:
- The third button when wrapped to the second line will use the full width. But I need the flex on all buttons to make them the same width when on one line. Is this okay or what can be done to look like you drawn in comment 2?
- When the box wraps, the content below doesn't shift down. What is needed to make the following content shifting down?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8546089 [details] [diff] [review]
GeneralPaneLayout.patch

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

::: browser/components/preferences/in-content/main.xul
@@ +153,5 @@
>      <separator class="thin"/>
>    </vbox>
>  #endif
> +
> +  <grid>

Can you try using an <html:table> instead of a <xul:grid> to see if that fixes the problem with the other content on the page not moving when the row wraps?

@@ +218,5 @@
> +                  accesskey="&chooseBookmark.accesskey;"
> +                  id="useBookmark"
> +                  preference="pref.browser.homepage.disable_button.bookmark_page"/>
> +          <button label="&restoreDefault.label;"
> +                  style="flex-grow: 1;"

It looks like there isn't a way to have this last item have both flex-grow:1; as well as not grow if it is moved to its own line. I sent an email to www-style@w3 to make sure that I didn't miss anything in the spec.

Let's stay with the item taking up the full width if it wraps. We need the wrapping due to some locales having long strings for their labels.

Comment 7

[Richard Marti (:Paenglab)]

Attachment: GeneralPaneLayout.patch

With <html:table> this works.
I've used inline styles where I think it's for the layout and the other styles in theme which should be themeable.

I haven't added the spacing between the radio lines in "Downloads" and "Tabs" as attachment 8439538 [details] proposes because I think this makes the page to tall and scrolling is needed too much. If still desired I could add a separator with class=thin which has a height of 0.5em.

Comment 8

[Richard Marti (:Paenglab)]

Attachment: GeneralPaneLayout.patch

Oops, this wasn't the latest patch.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8547047 [details] [diff] [review]
GeneralPaneLayout.patch

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

::: browser/components/preferences/in-content/main.xul
@@ +154,5 @@
>    </vbox>
>  #endif
> +
> +  <html:table id="startupTable"
> +              style="border-spacing: 0;">

These inline styles should be moved to an external stylesheet.

@@ +156,5 @@
> +
> +  <html:table id="startupTable"
> +              style="border-spacing: 0;">
> +    <html:tr>
> +      <html:td style="text-align: right; width: 1%;"> <!--  1% to remove the gaps -->

This doesn't look RTL friendly. You'll want text-align:end;

Also, what "gaps" is this referring to? border-spacing:0; should have removed the space between cells that you may have been seeing.

Comment 10

[Richard Marti (:Paenglab)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> Comment on attachment 8547047 [details] [diff] [review]
> GeneralPaneLayout.patch
> 
> Review of attachment 8547047 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/preferences/in-content/main.xul
> @@ +154,5 @@
> >    </vbox>
> >  #endif
> > +
> > +  <html:table id="startupTable"
> > +              style="border-spacing: 0;">
> 
> These inline styles should be moved to an external stylesheet.

Would a "chrome://browser/*content*/preferences/in-content/preferences.css" be okay for this rules or should they go to the normal stylesheet?

> @@ +156,5 @@
> > +
> > +  <html:table id="startupTable"
> > +              style="border-spacing: 0;">
> > +    <html:tr>
> > +      <html:td style="text-align: right; width: 1%;"> <!--  1% to remove the gaps -->
> 
> This doesn't look RTL friendly. You'll want text-align:end;
> 
> Also, what "gaps" is this referring to? border-spacing:0; should have
> removed the space between cells that you may have been seeing.

Without the width: 1%; the first column would flexing and be wider than the widest text.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> It looks like there isn't a way to have this last item have both
> flex-grow:1; as well as not grow if it is moved to its own line. I sent an
> email to www-style@w3 to make sure that I didn't miss anything in the spec.

I got confirmation from Tab that this is not possible with current flexbox but it is a high priority item for flexbox-2, http://lists.w3.org/Archives/Public/www-style/2015Jan/0150.html

Comment 12

[Richard Marti (:Paenglab)]

Attachment: GeneralPaneLayout.patch

Is this better?

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

I talked with Gijs about this patch and he noticed that the spec doesn't include the Default Browser checkbox. We weren't sure if the presence of the checkbox was taken in to account with the grid layout of the Startup section.

Michael, do you still want the Default Browser checkbox to be the first thing in the Startup section?

Comment 14

[Michael Maslaney [:mmaslaney] ([e-mail])]

(In reply to Richard Marti (:Paenglab) from comment #1)
> Created attachment 8455476 [details]
> mainGerman.png
> 
> Michael, I checked the German Nightly and saw they are using really long
> labels which makes it hard to follow your proposal. What do you think how
> can this styling be applied on locales with such long labels?
> 
> The only change I did on this screenshot was aligning the two first rows to
> show the issues better. The pane is on full width of 800px.

I would suggest the we left-align the form elements within the "Startup" grouping. If we could edge-case this locale that would be ideal from a design standpoint, but I can understand the complexity that requires on your end of the code, so it's not required.

Comment 15

[Michael Maslaney [:mmaslaney] ([e-mail])]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #13)
> I talked with Gijs about this patch and he noticed that the spec doesn't
> include the Default Browser checkbox. We weren't sure if the presence of the
> checkbox was taken in to account with the grid layout of the Startup section.
> 
> Michael, do you still want the Default Browser checkbox to be the first
> thing in the Startup section?

Yes, let's leave that in as the first item within the "Startup" grouping.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8549881 [details] [diff] [review]
GeneralPaneLayout.patch

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

::: browser/components/preferences/in-content/main.css
@@ +6,5 @@
> +  border-spacing: 0;
> +}
> +
> +#startupTable > tr > td:first-child {
> +  text-align: right;

You can use text-align: end; and then get rid of the RTL version below.

@@ +7,5 @@
> +}
> +
> +#startupTable > tr > td:first-child {
> +  text-align: right;
> +  width: 1%; /* make the column as small as possible */

I think we can do something better than this, but I need more time to figure it out. Have you played with using table-layout:fixed; on the #startupTable?

@@ +22,5 @@
> +#startupTable > tr > td:first-child > label {
> +  white-space: nowrap;
> +}
> +
> +#startupTable > tr > td:nth-child(2) > * {

Please add a class to each of these buttons and then use the className here instead of the universal selector. Anytime a universal selector (*) is used as the right-most selector we will have very bad performance as all elements on a page are first selected and then the selection is narrowed as the selector is parsed from right-to-left.

::: browser/components/preferences/in-content/main.xul
@@ +196,5 @@
> +    <html:tr>
> +      <html:td>
> +        <hbox/>
> +      </html:td>
> +      <html:td style="flex-wrap: wrap;">

This should be in the stylesheet. You can use classes or IDs on the <tr> and <td> so you don't have to use nth-child.

::: browser/themes/shared/incontentprefs/preferences.inc.css
@@ +135,5 @@
> +  padding: 0; /* remove the padding from html.css */
> +}
> +
> +#startupTable > tr:not(:first-child) > td {
> +  padding-top: 0.5em; /* add a spacing between the rows */

These rules should be moved to the main.css file.

Comment 17

[Richard Marti (:Paenglab)]

Attachment: GeneralPaneLayout.patch

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> Comment on attachment 8549881 [details] [diff] [review]
> GeneralPaneLayout.patch
> 
> Review of attachment 8549881 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/preferences/in-content/main.css
> @@ +6,5 @@
> > +  border-spacing: 0;
> > +}
> > +
> > +#startupTable > tr > td:first-child {
> > +  text-align: right;
> 
> You can use text-align: end; and then get rid of the RTL version below.

done

> @@ +7,5 @@
> > +}
> > +
> > +#startupTable > tr > td:first-child {
> > +  text-align: right;
> > +  width: 1%; /* make the column as small as possible */
> 
> I think we can do something better than this, but I need more time to figure
> it out. Have you played with using table-layout:fixed; on the #startupTable?

I tried this but it doesn't help. Where is no change.

> @@ +22,5 @@
> > +#startupTable > tr > td:first-child > label {
> > +  white-space: nowrap;
> > +}
> > +
> > +#startupTable > tr > td:nth-child(2) > * {
> 
> Please add a class to each of these buttons and then use the className here
> instead of the universal selector. Anytime a universal selector (*) is used
> as the right-most selector we will have very bad performance as all elements
> on a page are first selected and then the selection is narrowed as the
> selector is parsed from right-to-left.

done

> ::: browser/components/preferences/in-content/main.xul
> @@ +196,5 @@
> > +    <html:tr>
> > +      <html:td>
> > +        <hbox/>
> > +      </html:td>
> > +      <html:td style="flex-wrap: wrap;">
> 
> This should be in the stylesheet. You can use classes or IDs on the <tr> and
> <td> so you don't have to use nth-child.

done

> ::: browser/themes/shared/incontentprefs/preferences.inc.css
> @@ +135,5 @@
> > +  padding: 0; /* remove the padding from html.css */
> > +}
> > +
> > +#startupTable > tr:not(:first-child) > td {
> > +  padding-top: 0.5em; /* add a spacing between the rows */
> 
> These rules should be moved to the main.css file.

moved

Comment 18

[Richard Marti (:Paenglab)]

Attachment: td_width.png

This screenshot shows the difference with and without the width: 1%; for the first column.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8556600 [details] [diff] [review]
GeneralPaneLayout.patch

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

::: browser/components/preferences/in-content/main.css
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Actually, these are all in the wrong file. These should be moved to /browser/themes/shared/incontentprefs/preferences.css as well as the other rules within this main.css file (excluding the one for the binding).

@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#startupTable {
> +  border-spacing: 0;

This should be border-collapse: collapse;

border-spacing is only relevant when border-collapse: separated; is used, but we don't want `separated` here. border-collapse: collapse; gives us the desired look and also matches the desired table rendering model.

@@ +15,5 @@
> +}
> +
> +#startupTable > tr > .label-cell {
> +  text-align: end;
> +  width: 1%; /* make the column as small as possible */

Use `width: 0;` here. I couldn't find an alternate solution, but width:0; works the same as width:1% for me.

@@ +26,5 @@
> +#startupTable > tr > .content-cell {
> +  display: flex;
> +}
> +
> +#startupTable > tr > .wrap-content {

The .wrap-content class isn't referenced within the XUL file.

CSS class names should not describe the behavior of the styles. In the future when the styles change, the name will become inconsistent. A better name would be .homepage-buttons

@@ +30,5 @@
> +#startupTable > tr > .wrap-content {
> +  flex-wrap: wrap;
> +}
> +
> +#startupTable > tr > td > .grow-content {

This class can be renamed to `.content-cell-item`

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8556600 [details] [diff] [review]
GeneralPaneLayout.patch

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

::: browser/components/preferences/in-content/main.xul
@@ +195,5 @@
> +      </html:td>
> +    </html:tr>
> +    <html:tr>
> +      <html:td class="label-cell">
> +        <hbox/>

Please remove this <hbox />

Comment 21

[Richard Marti (:Paenglab)]

Attachment: GeneralPaneLayout.patch

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #19)
> Comment on attachment 8556600 [details] [diff] [review]
> GeneralPaneLayout.patch
> 
> Review of attachment 8556600 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/preferences/in-content/main.css
> @@ +1,3 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> Actually, these are all in the wrong file. These should be moved to
> /browser/themes/shared/incontentprefs/preferences.css as well as the other
> rules within this main.css file (excluding the one for the binding).

moved

> @@ +2,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +#startupTable {
> > +  border-spacing: 0;
> 
> This should be border-collapse: collapse;
> border-spacing is only relevant when border-collapse: separated; is used,
> but we don't want `separated` here. border-collapse: collapse; gives us the
> desired look and also matches the desired table rendering model.

done

> 
> @@ +15,5 @@
> > +}
> > +
> > +#startupTable > tr > .label-cell {
> > +  text-align: end;
> > +  width: 1%; /* make the column as small as possible */
> 
> Use `width: 0;` here. I couldn't find an alternate solution, but width:0;
> works the same as width:1% for me.

done

> @@ +26,5 @@
> > +#startupTable > tr > .content-cell {
> > +  display: flex;
> > +}
> > +
> > +#startupTable > tr > .wrap-content {
> 
> The .wrap-content class isn't referenced within the XUL file.
> 
> CSS class names should not describe the behavior of the styles. In the
> future when the styles change, the name will become inconsistent. A better
> name would be .homepage-buttons

Forgot to re-add after a test.

> @@ +30,5 @@
> > +#startupTable > tr > .wrap-content {
> > +  flex-wrap: wrap;
> > +}
> > +
> > +#startupTable > tr > td > .grow-content {
> 
> This class can be renamed to `.content-cell-item`

done

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8557244 [details] [diff] [review]
GeneralPaneLayout.patch

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

r=me, nice job.

::: browser/components/preferences/in-content/main.xul
@@ +198,5 @@
> +      <html:td class="label-cell" />
> +      <html:td class="content-cell homepage-buttons">
> +        <button id="useCurrent"
> +                class="content-cell-item"
> +                label=""

Looks like this label="" can be removed from the markup here.

Comment 23

[Richard Marti (:Paenglab)]

Attachment: GeneralPaneLayout.patch

Patch for check-in.

Comment 24

[Richard Marti (:Paenglab)]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbf7c9260825

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/7df637478de0

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7df637478de0