1453592 - The "filefield" binding will be removed

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[:Paolo Amadini]

The "filefield" binding will be removed in bug 1452624.

Comment 1

[Jorg K (CEST = GMT+2)]

Richard, can you please take a look.

Comment 2

[Richard Marti (:Paenglab)]

It's on my radar.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: hash.patch

Aceman, this patch changes the <filefield> to <textbox> like the m-c patch does.

The sound file chooser in the prefs works for me. The download directory chooser did not when I selected the dektop and then change to a other directory like downloads. It changed the icon but the text stayed. From other directories it worled all the time. I changed the code that it works now. Please can you check, that it's correct, and if not fix it?

The field in AB cards local photo doesn't show any text or icon. You worked in this area recently, please can you fix the code that it works?

The m-c patch got a r+ today. It can be that it will be checked-in soon.

Comment 4

[Richard Marti (:Paenglab)]

Attachment: filefield.patch

Sorry, wrong patch.

Comment 5

[:aceman]

Comment on attachment 8969754 [details] [diff] [review]
filefield.patch

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

This looks good to me.
On the Prefs/Chat tab if the 'Play a sound' is unchecked initially, the subsequent items aren't disabled. Only if you toggle it manually they get enabled/disabled correctly.
That may be a pre-existing problem. Should I make a patch for it?

::: mail/components/addrbook/content/abCard.inc
@@ +451,5 @@
>                  <radio id="FilePhotoType" value="file" label="&PhotoFile.label;"
>                         accesskey="&PhotoFile.accesskey;"/>
>                  <hbox class="indent">
> +                  <textbox id="PhotoFile" type="filefield" readonly="true"
> +                           maxlength="255" flex="1"/>

This box is now missing the path to the chosen file. The filefield binding probably set it automatically when loading the file. Now we have to do it manually at all callers (the preferences ones already do it to only show the leaf names, not fuul path). The progress...

::: mail/themes/linux/mail/filefield.css
@@ -7,5 @@
> -  margin: 2px 4px;
> -  -moz-appearance: textfield;
> -}
> -
> -.fileFieldIcon[disabled="true"] {

You could drop the 'disabled' style because textbox has a working one of its own?

::: mailnews/base/content/virtualFolderProperties.xul
@@ +56,5 @@
>        <row align="center">
>          <label value="&name.label;" accesskey="&name.accesskey;" control="name"/>
>          <textbox id="name" hidden="true"
>                   oninput="doEnabling();"/>
> +        <textbox id="existingName"

This doesn't seem to be so easy. When opening the virtual folder properties, the name is missing in the box. Notice this element is hidden, so there are some special games going on with it. It seems to be coupled with the folder picker in some way to get the VF icon and name as "foldername on servername".

Comment 6

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #5)
> Comment on attachment 8969754 [details] [diff] [review]
> filefield.patch
> 
> Review of attachment 8969754 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me.
> On the Prefs/Chat tab if the 'Play a sound' is unchecked initially, the
> subsequent items aren't disabled. Only if you toggle it manually they get
> enabled/disabled correctly.
> That may be a pre-existing problem. Should I make a patch for it?

It's pre-existing. Yes, please make a patch for it.

> ::: mail/components/addrbook/content/abCard.inc
> @@ +451,5 @@
> >                  <radio id="FilePhotoType" value="file" label="&PhotoFile.label;"
> >                         accesskey="&PhotoFile.accesskey;"/>
> >                  <hbox class="indent">
> > +                  <textbox id="PhotoFile" type="filefield" readonly="true"
> > +                           maxlength="255" flex="1"/>
> 
> This box is now missing the path to the chosen file. The filefield binding
> probably set it automatically when loading the file. Now we have to do it
> manually at all callers (the preferences ones already do it to only show the
> leaf names, not fuul path). The progress...

Please, can you fix it? I looked at it and din't found a solution.

> ::: mail/themes/linux/mail/filefield.css
> @@ -7,5 @@
> > -  margin: 2px 4px;
> > -  -moz-appearance: textfield;
> > -}
> > -
> > -.fileFieldIcon[disabled="true"] {
> 
> You could drop the 'disabled' style because textbox has a working one of its
> own?

I removed it because I think the text changes and this is enough. You think not?

> ::: mailnews/base/content/virtualFolderProperties.xul
> @@ +56,5 @@
> >        <row align="center">
> >          <label value="&name.label;" accesskey="&name.accesskey;" control="name"/>
> >          <textbox id="name" hidden="true"
> >                   oninput="doEnabling();"/>
> > +        <textbox id="existingName"
> 
> This doesn't seem to be so easy. When opening the virtual folder properties,
> the name is missing in the box. Notice this element is hidden, so there are
> some special games going on with it. It seems to be coupled with the folder
> picker in some way to get the VF icon and name as "foldername on servername".

Have you ever seen this field? Me not. Please can you fix it too, if it still is needed?

Comment 7

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #6)
> It's pre-existing. Yes, please make a patch for it.
Ok.

> > ::: mail/components/addrbook/content/abCard.inc
> > > +                  <textbox id="PhotoFile" type="filefield" readonly="true"
> > > +                           maxlength="255" flex="1"/>
> > 
> > This box is now missing the path to the chosen file. The filefield binding
> > probably set it automatically when loading the file. Now we have to do it
> > manually at all callers (the preferences ones already do it to only show the
> > leaf names, not fuul path). The progress...
> 
> Please, can you fix it? I looked at it and din't found a solution.

If we can live with this broken for a few days, I'd rather fix it in bug 691141 as it would break my patch there.

> > ::: mailnews/base/content/virtualFolderProperties.xul
> > @@ +56,5 @@
> > >        <row align="center">
> > >          <label value="&name.label;" accesskey="&name.accesskey;" control="name"/>
> > >          <textbox id="name" hidden="true"
> > >                   oninput="doEnabling();"/>
> > > +        <textbox id="existingName"
> > 
> > This doesn't seem to be so easy. When opening the virtual folder properties,
> > the name is missing in the box. Notice this element is hidden, so there are
> > some special games going on with it. It seems to be coupled with the folder
> > picker in some way to get the VF icon and name as "foldername on servername".
> 
> Have you ever seen this field? Me not. Please can you fix it too, if it
> still is needed?

If we can't have the icon, then drop the class and the SpecialFolder attribute and in virtualFolderProperties.js::InitDialogWithVirtualFolder change the folderNameField.setAttribute("label", name); to use "value".

Comment 8

[:aceman]

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

> > On the Prefs/Chat tab if the 'Play a sound' is unchecked initially, the
> > subsequent items aren't disabled. Only if you toggle it manually they get
> > enabled/disabled correctly.
> > That may be a pre-existing problem. Should I make a patch for it?
> 
> It's pre-existing. Yes, please make a patch for it.

Actually just add this.updatePlaySound(); to the init() function in chat.js .

Comment 9

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #7)
> 
> If we can live with this broken for a few days, I'd rather fix it in bug
> 691141 as it would break my patch there.

This would be okay for me.

Comment 10

[Richard Marti (:Paenglab)]

Attachment: filefield.patch v2

This patch needs bug 691141
The virtualfolderdialog is fixed by not showing the icon. The text appears now. The inactive chat button is fixed too.

The remaining part is the AB photo textbox which doesn't work yet.

Comment 11

[:aceman]

Comment on attachment 8969945 [details] [diff] [review]
filefield.patch v2

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

::: mailnews/base/content/virtualFolderProperties.xul
@@ -6,5 @@
>  
>  <?xml-stylesheet href="chrome://messenger/skin/dialogs.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://messenger/skin/searchDialog.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://messenger/skin/messenger.css" type="text/css"?>
> -<?xml-stylesheet href="chrome://messenger/skin/folderMenus.css" type="text/css"?>

You can't remove this as the file contains another folder picker widget which needs these icons/style. This same dialog is also used for creating new saved search folders and then that picker is exposed.

Comment 12

[:aceman]

Attachment: 1453592.patch v3

Thanks, and this fixes the display of the file name in the AB card photo picker.

Please check it (it gave me the icon too on Linux) and I can give you review then. I also removed the dropping of the mentioned folderMenu.css .

Comment 13

[Richard Marti (:Paenglab)]

Comment on attachment 8969946 [details] [diff] [review]
1453592.patch v3

Great, works like a charm.

I'm fine tuning the appearance and upload then the final patch for review.

Comment 14

[Richard Marti (:Paenglab)]

Attachment: filefield.patch v3.1

This one should be the final patch.

I added now a inactive state for the textbox including the image.

Comment 15

[:aceman]

Comment on attachment 8969983 [details] [diff] [review]
filefield.patch v3.1

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

It seems you only added some new css, then I'm now happy with the patch.
I asked about the removed [disabled=true] css rules, assuming textbox[disabled=true] is working and we inherit this now. That was probably true, but the icon didn't get greyed out when the textbox is disabled. Now I see this patch solves this. Thanks.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/da1dffe2b605
Change <filefield> to <textbox> after its removal in bug 1452624. r=aceman