remove grid usage from comm/mail/components/preferences/cookies.xul and comm/mail/components/preferences/display.inc.xul

RESOLVED FIXED in Thunderbird 68.0

Status

enhancement
RESOLVED FIXED
a month ago
25 days ago

People

(Reporter: khushil324, Assigned: khushil324)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 68.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

a month ago
Assignee: nobody → khushil324
Status: NEW → ASSIGNED
Depends on: tb-burn-xul-grids
(Assignee)

Updated

a month ago
No longer depends on: tb-burn-xul-grids
Comment hidden (obsolete)
Attachment #9051643 - Attachment description: Bug 1536028 - Vendor pytest-mock via |mach vendor python|. r=ahal → Bug 1536029 - Vendor pytest-mock via |mach vendor python|. r=ahal
Comment hidden (obsolete)
(Assignee)

Comment 3

a month ago
Posted patch remove_grid_prefs_1.patch (obsolete) — Splinter Review
Attachment #9052070 - Flags: review?(mkmelin+mozilla)

Comment 4

a month ago
Comment on attachment 9052070 [details] [diff] [review]
remove_grid_prefs_1.patch

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

I don't know what the `width="*"` do, but you might want to check if that is compatible with all locales.

::: mail/components/preferences/display.inc.xul
@@ +22,5 @@
>              <label><html:h2>&fontsAndColors1.label;</html:h2></label>
>  
> +            <vbox id="fontsGrid">
> +              <hbox id="fontRow">
> +                <hbox flex="1" align="center">

Not sure if `<hbox flex="1" align="center">` makes a difference here, you might be able to remove that, and make everything a direct child of `#fontRow` (and add align="center" there if needed).

@@ +63,5 @@
> +                <button id="advancedFonts" label="&fontOptions.label;" icon="select-font" accesskey="&fontOptions.accesskey;" oncommand="gDisplayPane.configureFonts();"/>
> +              </hbox>
> +              <hbox id="colorsRow">
> +                <hbox flex="1"/>
> +                <button style="width: 99px" id="colors" icon="select-color" label="&colorButton.label;" accesskey="&colorButton.accesskey;" oncommand="gDisplayPane.configureColors();"/>

You can drop `<hbox flex="1"/>` and use pack="end" on #colorsRow
Comment on attachment 9052070 [details] [diff] [review]
remove_grid_prefs_1.patch

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

::: mail/components/preferences/cookies.xul
@@ +54,4 @@
>        <treechildren id="cookiesChildren"/>
>      </tree>
>      <hbox id="cookieInfoBox">
> +      <vbox flex="1" id="cookieInfoGrid">

I wonder if just using <html:table> would work for this It's basically table information

@@ +58,5 @@
> +        <hbox align="center">
> +          <hbox pack="end" width="65">
> +            <label id="nameLabel" control="name" value="&props.name.label;"/>
> +          </hbox>
> +          <textbox flex="1" id="name" readonly="true" class="plain"/>

please keep ids first

@@ +61,5 @@
> +          </hbox>
> +          <textbox flex="1" id="name" readonly="true" class="plain"/>
> +        </hbox>
> +        <hbox align="center">
> +          <hbox pack="end" width="65">

hardcoding a 65 looks wrong

@@ +94,5 @@
> +        <hbox align="center" id="userContextRow">
> +          <hbox pack="end" width="65">
> +            <label id="userContextLabel" control="userContext" value="&props.container.label;"/>
> +          </hbox>
> +          <textbox flex="1" id="userContext" readonly="true" class="plain"/>

pleased keep the ids first (except for is=)

::: mail/components/preferences/display.inc.xul
@@ +21,4 @@
>            <groupbox id="fontsGroup">
>              <label><html:h2>&fontsAndColors1.label;</html:h2></label>
>  
> +            <vbox id="fontsGrid">

rename the id to fontSettings

@@ +63,5 @@
> +                <button id="advancedFonts" label="&fontOptions.label;" icon="select-font" accesskey="&fontOptions.accesskey;" oncommand="gDisplayPane.configureFonts();"/>
> +              </hbox>
> +              <hbox id="colorsRow">
> +                <hbox flex="1"/>
> +                <button style="width: 99px" id="colors" icon="select-color" label="&colorButton.label;" accesskey="&colorButton.accesskey;" oncommand="gDisplayPane.configureColors();"/>

I don't think the button should have a style width like this. I doubt that would be good on localizations.

In general, could have a look at what Firefox has: https://searchfox.org/comm-central/source/mozilla/browser/components/preferences/in-content/main.xul#136
Attachment #9052070 - Flags: review?(mkmelin+mozilla)
Attachment #9052070 - Flags: review-
Attachment #9052070 - Flags: feedback+
(Assignee)

Comment 6

a month ago
Posted patch remove_grid_prefs_1.patch (obsolete) — Splinter Review
Attachment #9052070 - Attachment is obsolete: true
Attachment #9052611 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9052611 [details] [diff] [review]
remove_grid_prefs_1.patch

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

While it looks nice, the hard coded widths are scary and likely the wrong thing to do.

For connection.xul, the de-grid of browser/components/preferences/connection.xul hasn't happened yet. We're basically just keeping our copy up do date with that, so it could actually be worth just not including that part in this patch. Also, probably preferable to just do one file per patch/bug (for future reference).

Please also fix the commit message

::: mail/components/preferences/connection.xul
@@ +38,5 @@
>                 id="systemPref" hidden="true"/>
>          <radio value="1" label="&manualTypeRadio.label;" accesskey="&manualTypeRadio.accesskey;"/>
> +        <vbox class="indent" flex="1">
> +          <hbox align="center">
> +            <hbox pack="end" width="100">

no width please

@@ +51,5 @@
> +                        onsyncfrompreference="return gConnectionsDialog.readHTTPProxyPort();"/>
> +            </hbox>
> +          </hbox>
> +          <hbox>
> +            <hbox width="104"/>

and here + a few more

@@ +82,5 @@
> +                        preference="network.proxy.socks"
> +                        onsyncfrompreference="return gConnectionsDialog.readProxyProtocolPref('socks', false);"/>
> +              <label value="&SOCKSport.label;" accesskey="&SOCKSport.accesskey;" control="networkProxySOCKS_Port"/>
> +              <textbox id="networkProxySOCKS_Port" type="number" max="65535" size="5"
> +                        preference="network.proxy.socks_port"

misaligned with one char it seems
Attachment #9052611 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 8

a month ago

(In reply to Magnus Melin [:mkmelin] from comment #7)

Comment on attachment 9052611 [details] [diff] [review]

For connection.xul, the de-grid of
browser/components/preferences/connection.xul hasn't happened yet. We're
basically just keeping our copy up do date with that, so it could actually
be worth just not including that part in this patch. Also, probably
preferable to just do one file per patch/bug (for future reference).

So should I just remove connection.xul changes from this file right now? Should I open the new bug for that now?

I think that's easiest. We can file a new bug later.

(Assignee)

Comment 10

a month ago

Cool, will update the patch in a while.

(Assignee)

Updated

a month ago
Summary: remove grid usage from comm/mail/components/preferences/connection.xul, comm/mail/components/preferences/cookies.xul and comm/mail/components/preferences/display.inc.xul → remove grid usage from comm/mail/components/preferences/cookies.xul and comm/mail/components/preferences/display.inc.xul
(Assignee)

Comment 11

a month ago
Posted patch remove_grid_prefs_1.patch (obsolete) — Splinter Review
Attachment #9052611 - Attachment is obsolete: true
Attachment #9052863 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9052863 [details] [diff] [review]
remove_grid_prefs_1.patch

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

Please fix the commit message now that connection.xul is not included

::: mail/components/preferences/display.inc.xul
@@ +28,5 @@
> +                  <menulist id="defaultFont" flex="1" sizetopopup="pref" crop="center" onsyncfrompreference="return gDisplayPane.readFontSelection()">
> +                    <menupopup crop="center"/>
> +                  </menulist>
> +                  <label accesskey="&defaultSize.accesskey;" control="defaultFontSize">&defaultSize.label;</label>
> +                  <menulist flex="1" id="defaultFontSize">

id first please
Attachment #9052863 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 13

a month ago
Attachment #9052863 - Attachment is obsolete: true
Attachment #9052972 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

a month ago
Keywords: checkin-needed

Comment 15

a month ago

Magnus, can you please check the line length. There are lines over 180 characters(!!!) long, that doesn't comply with any coding standard :-( - I will shorten them now.

Comment 16

a month ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f9d9f70bc490
remove grid usage from preferences' cookies.xul and display.inc.xul. r=mkmelin

Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

a month ago
Target Milestone: --- → Thunderbird 68.0
(Assignee)

Updated

a month ago
Attachment #9052972 - Flags: review?(mkmelin+mozilla)

Updated

25 days ago
Depends on: 1540423

Updated

25 days ago
No longer depends on: 1540423
You need to log in before you can comment on or make changes to this bug.