Closed Bug 1563124 Opened 1 year ago Closed 5 months ago

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

Categories

(Thunderbird :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 71.0

People

(Reporter: aleca, Assigned: aleca)

References

Details

Attachments

(1 file, 5 obsolete files)

  • 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
Assignee: nobody → alessandro
Mentor: alessandro
Attached patch 1563124-textbox-html-input.patch (obsolete) — Splinter Review

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.

Attachment #9090541 - Flags: feedback?(mkmelin+mozilla)
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
Attachment #9090541 - Flags: feedback?(mkmelin+mozilla) → feedback-
Status: NEW → ASSIGNED
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.

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

Attached image file-filefield.png (obsolete) —

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.

Flags: needinfo?(mkmelin+mozilla)

(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

Flags: needinfo?(mkmelin+mozilla)
Attached patch 1563124-textbox-html-input.patch (obsolete) — Splinter Review

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,

Attachment #9090541 - Attachment is obsolete: true
Attachment #9090763 - Attachment is obsolete: true
Attachment #9090834 - Flags: feedback?(richard.marti)
Attachment #9090834 - Flags: feedback?(mkmelin+mozilla)
Attachment #9090834 - Attachment is obsolete: true
Attachment #9090834 - Flags: feedback?(richard.marti)
Attachment #9090834 - Flags: feedback?(mkmelin+mozilla)
Attachment #9090843 - Flags: feedback?(richard.marti)
Attachment #9090843 - Flags: feedback?(mkmelin+mozilla)
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
Attachment #9090843 - Flags: feedback?(richard.marti)
Attachment #9090843 - Flags: feedback?(mkmelin+mozilla)

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

Attached patch 1563124-textbox-html-input.patch (obsolete) — Splinter Review

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.

Attachment #9090843 - Attachment is obsolete: true
Attachment #9091642 - Flags: ui-review?(richard.marti)
Attachment #9091642 - Flags: review?(mkmelin+mozilla)
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
Attachment #9091642 - Flags: review?(mkmelin+mozilla)
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.
Attachment #9091642 - Flags: ui-review?(richard.marti)

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

Flags: needinfo?(mkmelin+mozilla)
Attachment #9091642 - Attachment is obsolete: true
Attachment #9091784 - Flags: ui-review?(richard.marti)
Attachment #9091784 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9091784 [details] [diff] [review]
1563124-textbox-html-input.patch

Thanks.
Attachment #9091784 - Flags: ui-review?(richard.marti) → ui-review+

All good :)

Flags: needinfo?(mkmelin+mozilla)
Attachment #9091784 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed

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

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0
You need to log in before you can comment on or make changes to this bug.