1563124 - Use HTML input instead of XUL textbox in mail/components/preferences/

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Alessandro Castellani [:aleca]]

• mail/components/preferences/applicationManager.xul
• mail/components/preferences/applications.inc.xul
• mail/components/preferences/chat.inc.xul
• mail/components/preferences/connection.xul
• mail/components/preferences/cookies.xul
• mail/components/preferences/general.inc.xul
• mail/components/preferences/permissions.xul

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: 1563124-textbox-html-input.patch

I'm requesting some early feedback for this patch as I'm touching some areas I'd like to hear your opinion before moving forward.

The style coming from m-c in common.css doesn't account for html|input but only textbox, so I created some extra CSS declarations in the input-fields.css to keep the UI consistent.

The current CSS coming from m-c affects input fields only if they have a type set, that's why I'm adding type="text" even if it's redundant.

It seems like the <observes element="chatSoundUrlLocation" attribute="disabled"/> for the "Browse Sound" button doesn't work once the field is converted to a textbox.

Doing document.getElementById("chatSoundUrlLocation").disabled = ... doesn't seem to work, and I found it necessary to use setAttribute() in order to properly update the disabled attribute.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9090541 [details] [diff] [review]
1563124-textbox-html-input.patch

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

::: mail/components/preferences/applicationManager.xul
@@ +55,5 @@
>    </hbox>
>    <vbox id="appDetails">
>      <separator class="thin"/>
>      <label id="appType"/>
> +    <html:input id="appLocation" readonly="true" class="input-inline"/>

readonly="readonly"

::: mail/components/preferences/chat.js
@@ +189,5 @@
>  
>      document.getElementById("chatSoundType").disabled = !soundsEnabled;
> +    document
> +      .getElementById("chatSoundUrlLocation")
> +      .setAttribute("disabled", soundDisabled);

disabled is also a boolean attribute in html, so use toggleAttribute("disabled", soundDisabled);

::: mail/themes/linux/mail/filefield.css
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +@namespace html url("http://www.w3.org/1999/xhtml");
> +
> +html|input[type="filefield"] {

file. There's no filefield in html

@@ +16,4 @@
>    background-position-x: right 6px;
>  }
>  
> +html|input[type="filefield"][disabled="true"] {

disabled (since it's a boolean attr)

::: mail/themes/shared/mail/incontentprefs/preferences.css
@@ +418,5 @@
>   * toolkit element overrides
>   */
>  
> +groupbox
> +  label:not(.menu-accel):not(.menu-iconic-accel):not(.menu-text):not(.menu-iconic-text):not(.menulist-label):not(.indent):not(.learnMore):not(.tail-with-learn-more),

not sure what happened here

::: mail/themes/windows/mail/filefield.css
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +@namespace html url("http://www.w3.org/1999/xhtml");
> +
> +html|input[type="filefield"] {

I'd get rid of all of the filefield.css and just fold what's needed into the input css file, since it's a file input

Comment 3

[Richard Marti (:Paenglab)]

Comment on attachment 9090541 [details] [diff] [review]
1563124-textbox-html-input.patch

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

::: mail/themes/shared/mail/incontentprefs/preferences.css
@@ +10,5 @@
>    --in-content-item-selected-unfocused: var(--grey-20);
>  }
>  
>  @supports -moz-bool-pref("browser.in-content.dark-mode") {
> +  @media (prefers-color-scheme: dark) {

Please don't indent this block. FX also haven't indented this because the `@supports -moz-bool-pref("browser.in-content.dark-mode")` is only temporary and will be removed.

::: mail/themes/shared/mail/input-fields.css
@@ +11,5 @@
>    padding-inline-start: 4px;
>  }
>  
> +html|input.input-filefield {
> +  border-radius: 2px;

Windows 10 has no border radius. Maybe you can use --toolbarbutton-border-radius.

Comment 4

[Alessandro Castellani [:aleca]]

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

::: mail/themes/shared/mail/incontentprefs/preferences.css
@@ +418,5 @@

toolkit element overrides

groupbox
label:not(.menu-accel):not(.menu-iconic-accel):not(.menu-text):not(.menu-iconic-text):not(.menulist-label):not(.indent):not(.learnMore):not(.tail-with-learn-more),

not sure what happened here

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

::: mail/themes/shared/mail/incontentprefs/preferences.css
@@ +10,5 @@

--in-content-item-selected-unfocused: var(--grey-20);
}

@supports -moz-bool-pref("browser.in-content.dark-mode") {
@media (prefers-color-scheme: dark) {

Please don't indent this block. FX also haven't indented this because the
@supports -moz-bool-pref("browser.in-content.dark-mode") is only temporary
and will be removed.

These changes are due to prettier autoformatting the file on save.
I'll disable it for CSS files.

Thank you both for the feedback!

Comment 5

[Alessandro Castellani [:aleca]]

Attachment: file-filefield.png

I think the filefield is a "custom" type used with this specific purpose of having a text field with a different formatting and content handling than a regular text field.

As you can see from the screenshot, using type="file" causes the field to be rendered as a file picker, even if I set -moz-appearance: none .

Also, the toggleAttribute("disabled", soundDisabled); seems to be completely ignored.

Comment 6

[Magnus Melin [:mkmelin]]

(In reply to Alessandro Castellani (:aleca) from comment #5)

I think the filefield is a "custom" type used with this specific purpose of having a text field with a different formatting and content handling than a regular text field.

Yes, in xul. But now you're changing to html:input and type="filefield" would not do anything. filefield is not used in firefox at all, so you'll have to do a bit of redesign and just use a normal file input instead.

As you can see from the screenshot, using type="file" causes the field to be rendered as a file picker, even if I set -moz-appearance: none .

Also, the toggleAttribute("disabled", soundDisabled); seems to be completely ignored.

You had a typo I copied: soundsDisabled

Comment 7

[Alessandro Castellani [:aleca]]

Attachment: 1563124-textbox-html-input.patch

Right on, thanks for the feedback and suggestions.
I think I found a good solution.

Setting the fields as a regular type="text" allows us to inherit the style and various states used across the preferences tabs and keep visual consistency.
The few visual changes are implemented with a dedicated .input-filefield class, which recreates exactly the current textbox behaviour.

I also updated the input-fields.css file to be OS based, and renamed it inputFields.css for consistency with the other CSS files.

The filefield.css files seem to not be useful anymore, and the classes declared in the global filefield.css are not used anywhere.
Am I safe to remove them?

Richard, can you give me a UI feedback on what I did?
Cheers,

Comment 8

[Alessandro Castellani [:aleca]]

Attachment: 1563124-textbox-html-input.part1.patch

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9090843 [details] [diff] [review]
1563124-textbox-html-input.part1.patch

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

::: mail/themes/osx/mail/filefield.css
@@ +1,5 @@
>  /* 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/. */
>  
> +@namespace html url("http://www.w3.org/1999/xhtml");

The filefield.css files should be removed completely, not just cleared.

::: mail/themes/shared/jar.inc.mn
@@ +136,1 @@
>  #ifndef XP_MACOSX

one space off

::: mail/themes/shared/mail/inputFields.css
@@ +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/. */
> +
> +@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> +@namespace html url("http://www.w3.org/1999/xhtml");

I would prefer the input-fields.css naming of the file. In general, I'd prefer all files to be lower cased. Camel casing for file names is less reliable, and can cause problems especially on windows where case sensitivity doesn't matter.
And, you should have hg moved the file if you wanted to move it.

::: mail/themes/windows/jar.mn
@@ +37,5 @@
>    skin/classic/messenger/mailWindow1.css                      (mail/mailWindow1.css)
>    skin/classic/messenger/messageWindow.css                    (mail/messageWindow.css)
>    skin/classic/messenger/searchBox.css                        (mail/searchBox.css)
>    skin/classic/messenger/junkMail.css                         (mail/junkMail.css)
> +  skin/classic/messenger/inputFields.css                     (mail/inputFields.css)

also one space off

Comment 10

[Alessandro Castellani [:aleca]]

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

In general, I'd prefer all files to be lower cased. Camel casing for file names is less
reliable, and can cause problems especially on windows where case
sensitivity doesn't matter.

It would be helpful to have this alongside other code guidelines somewhere in our DTN.
Or maybe even enforced by Prettier?
What do you think?

Comment 11

[Alessandro Castellani [:aleca]]

Attachment: 1563124-textbox-html-input.patch

This should be good for a full review.
I'm also asking for a UI review from Richard to be sure everything looks as it should in the Preferences Tab and various dialogs.

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9091642 [details] [diff] [review]
1563124-textbox-html-input.patch

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

Looks pretty good, except for all the number inputs which look too thin. Well, that's already the case on trunk. I guess it's due to the spin buttons? Maybe we should hide those like firefox does.

::: calendar/base/content/dialogs/calendar-properties-dialog.xul
@@ +4,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <?xml-stylesheet href="chrome://global/skin/global.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://calendar-common/skin/calendar-properties-dialog.css" type="text/css"?>
> +<?xml-stylesheet href="chrome://messenger/skin/input-fields.css" type="text/css"?>

great, thx for fixing this!

::: mail/components/preferences/connection.xul
@@ +53,5 @@
>                <label value="&http.label;" accesskey="&http.accesskey;" control="networkProxyHTTP"/>
>              </hbox>
> +            <hbox align="center" class="input-container">
> +              <html:input id="networkProxyHTTP"
> +                          type="url"

This is really only a text field. You're supposed to enter a hostname or an ip address, not a full url.

@@ +73,5 @@
>                <label value="&ssl.label;" accesskey="&ssl.accesskey;" control="networkProxySSL"/>
>              </hbox>
> +            <hbox align="center" class="input-container">
> +              <html:input id="networkProxySSL"
> +                          type="url"

here too

@@ +86,5 @@
>                <label value="&socks.label;" accesskey="&socks.accesskey;" control="networkProxySOCKS"/>
>              </hbox>
> +            <hbox align="center" class="input-container">
> +              <html:input id="networkProxySOCKS"
> +                          type="url"

and here

::: mail/components/preferences/general.js
@@ +431,5 @@
>        this.readFontSelection()
>      );
> +    Preferences.addSyncFromPrefListener(
> +      document.getElementById("soundUrlLocation"),
> +      () => this.readSoundLocation()

please add a ; after
in the above case too, I'm surprised linting doesn't complain

::: mail/themes/shared/mail/input-fields.css
@@ +23,5 @@
> +html|input.input-filefield:-moz-locale-dir(rtl) {
> +  background-position-x: right 2px;
> +}
> +
> +html|input.input-filefield[disabled="true"] {

no such thing, as disabled is a boolean attribute

Comment 13

[Richard Marti (:Paenglab)]

Comment on attachment 9091642 [details] [diff] [review]
1563124-textbox-html-input.patch

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

All in all it looks good except the indent on some elements.

(In reply to Magnus Melin [:mkmelin] from comment #12)
> Comment on attachment 9091642 [details] [diff] [review]
> 1563124-textbox-html-input.patch
> 
> Review of attachment 9091642 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks pretty good, except for all the number inputs which look too thin.
> Well, that's already the case on trunk. I guess it's due to the spin
> buttons? Maybe we should hide those like firefox does.

Please no. It needs only fixing some padding.

::: mail/themes/shared/mail/incontentprefs/preferences.css
@@ +316,5 @@
>  #previewDeck {
>    height: 220px;
>  }
>  
> +html|input.indent {

There is not only input that needs this rule. If the namespace is needed, add a second selector.

Comment 14

[Alessandro Castellani [:aleca]]

Looks pretty good, except for all the number inputs which look too thin. Well, that's already the case on trunk. I guess it's due to the spin buttons? Maybe we should hide those like firefox does.

We should fix the input numbers on a dedicated bug as the issue is not related to this change.

Preferences.addSyncFromPrefListener(
  document.getElementById("soundUrlLocation"),
  () => this.readSoundLocation()

please add a ; after in the above case too, I'm surprised linting doesn't complain

This seems correct to me as the 2 arguments of the addSyncFromPrefListener method don't need a ; at the end.
The linter screams at me if I add it.
Same thing we're doing here: https://searchfox.org/comm-central/rev/90a8e53eeb9838a101c2cde459f6abb0a598051c/mail/components/preferences/chat.js#53

Comment 15

[Alessandro Castellani [:aleca]]

Attachment: 1563124-textbox-html-input.patch

Patch updated and try run pushed: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ffb0e4117f4403222ef19ae17c405b5a578227e6

Comment 16

[Richard Marti (:Paenglab)]

Comment on attachment 9091784 [details] [diff] [review]
1563124-textbox-html-input.patch

Thanks.

Comment 17

[Magnus Melin [:mkmelin]]

All good :)

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a466dc8e0863
Use HTML input instead of XUL textbox in mail/components/preferences/. r=mkmelin