787640 - XMPP: Account wizard and account settings window dimension too small, input fields cut off/cropped, port input hidden

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: account wizard cut off

Mozilla/5.0 (Windows NT 5.1; rv:16.0) Gecko/20120828 Thunderbird/16.0

Chat: Account wizard and account settings window dimension too small, input fields cut off/cropped, port input hidden. See the attached screenshots for the problem with the German Thunderbird.

Account wizard: Extending the XMPP options even causes scrollbars in the English Thunderbird 15.0

Account settings: We (German localizers) need the encryption input on a new line so it doesn't get cut off.

Comment 1

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: account settings cut off

Comment 2

[Richard Marti (:Paenglab)]

Attachment: patch

Changed the menulist from hbox to a vbox. This gives the needed width to show the full text in it under English. For the German text I would say the third option text should be shortened, it's unneeded way too long.

I've changed also the layout in AccountWizard to a grid to give the textboxes a equal width.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: comparison

Comparison for easier ui-review

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 661477 [details] [diff] [review]
patch

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

Overall, this seems a great improvement (except the change in the username page that I don't understand), thanks!

::: mail/components/im/content/imAccountWizard.js
@@ -82,5 @@
>                                                         aDefaultValue) {
>      var hbox = document.createElement("hbox");
>      hbox.setAttribute("id", aName + "-hbox");
>      hbox.setAttribute("align", "baseline");
> -    hbox.setAttribute("equalsize", "always");

Why are you removing this?
Here is a screenshot before and after this change: http://imgur.com/a/NIisk

@@ +171,5 @@
>    },
>  
>    createTextbox: function aw_createTextbox(aType, aValue, aLabel, aName) {
> +    var row = document.createElement("row");
> +    row.setAttribute("align", "center");

Any reason for the change from align baseline to align center? I don't see an obvious difference right now, but years ago the difference between baseline and center was very visible on some platforms.

@@ +191,5 @@
>    },
>  
>    createMenulist: function aw_createMenulist(aList, aLabel, aName) {
> +    var vbox = document.createElement("vbox");
> +    vbox.setAttribute("flex", "1");

I'm surprised that putting a flexible vbox as a row in a grid produces a good result, but it does, and this is a great improvement compared to what we had before your patch! I just hope this vbox in grid thing that looks nice isn't a XUL bug that may disappear :-).

::: mail/components/im/content/imAccountWizard.xul
@@ +88,5 @@
> +      <columns>
> +        <column flex="1"/>
> +        <column flex="1"/>
> +      </columns>
> +      <rows id="protoSpecific"/>

Nit: please indent <columns> and <rows>.

I wonder if these new XUL elements (grid, column, ...) should have ids. I don't see an immediate need for it, but some add-on or theme authors may want them.

Comment 5

[Richard Marti (:Paenglab)]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> 
> ::: mail/components/im/content/imAccountWizard.js
> @@ -82,5 @@
> >                                                         aDefaultValue) {
> >      var hbox = document.createElement("hbox");
> >      hbox.setAttribute("id", aName + "-hbox");
> >      hbox.setAttribute("align", "baseline");
> > -    hbox.setAttribute("equalsize", "always");
> 
> Why are you removing this?
> Here is a screenshot before and after this change: http://imgur.com/a/NIisk

Because the input field jumps in width when I enter something on Win7. Without it's stable in width.

> @@ +171,5 @@
> >    },
> >  
> >    createTextbox: function aw_createTextbox(aType, aValue, aLabel, aName) {
> > +    var row = document.createElement("row");
> > +    row.setAttribute("align", "center");
> 
> Any reason for the change from align baseline to align center? I don't see
> an obvious difference right now, but years ago the difference between
> baseline and center was very visible on some platforms.

I copied it from where I took the grid implementation.
 
> @@ +191,5 @@
> >    },
> >  
> >    createMenulist: function aw_createMenulist(aList, aLabel, aName) {
> > +    var vbox = document.createElement("vbox");
> > +    vbox.setAttribute("flex", "1");
> 
> I'm surprised that putting a flexible vbox as a row in a grid produces a
> good result, but it does, and this is a great improvement compared to what
> we had before your patch! I just hope this vbox in grid thing that looks
> nice isn't a XUL bug that may disappear :-).

Me too ;) I'd like to put it below the server/port but I haven't found a way.

> ::: mail/components/im/content/imAccountWizard.xul
> @@ +88,5 @@
> > +      <columns>
> > +        <column flex="1"/>
> > +        <column flex="1"/>
> > +      </columns>
> > +      <rows id="protoSpecific"/>
> 
> Nit: please indent <columns> and <rows>.
> 
> I wonder if these new XUL elements (grid, column, ...) should have ids. I
> don't see an immediate need for it, but some add-on or theme authors may
> want them.

I think if a Add-on wants to put something in it then it has also to use the rows. But if you like I can add ids.

Comment 6

[Florian Quèze [:florian]]

(In reply to Richard Marti [:Paenglab] from comment #5)
> (In reply to Florian Quèze [:florian] [:flo] from comment #4)

> > > -    hbox.setAttribute("equalsize", "always");
> > 
> > Why are you removing this?
> > Here is a screenshot before and after this change: http://imgur.com/a/NIisk
> 
> Because the input field jumps in width when I enter something on Win7.
> Without it's stable in width.

:-(
Would a grid work better in the username page too?


> I'd like to put it below the server/port but I haven't found a way.

What makes you think it would be an improvement? (just curious, I don't really mind either way).

The order is defined by the order of the options in the code at http://mxr.mozilla.org/comm-central/source/chat/protocols/xmpp/xmpp.js#29 I think you can easily change it.

> I think if a Add-on wants to put something in it then it has also to use the
> rows.

Possibly.

> But if you like I can add ids.

I don't really mind either way.

To be clear, the reason for r- was really the username page that looks broken with the patch, I don't care so much about my other comments.

Comment 7

[Florian Quèze [:florian]]

I just tested with DOM Inspector what happens if the label of one of the advanced options has a long localized string. It's not pretty :( http://i.imgur.com/ZSwoR.png

Is there any way to make these labels wrap?

Comment 8

[Richard Marti (:Paenglab)]

In MDN I found this: By default, label text does not wrap. To enable wrapping, use a text node instead of the value attribute.

I'll try this this evening.

Comment 9

[Richard Marti (:Paenglab)]

Attachment: patch v2

This patch lets the username page as it was. I've added a css rule for Windows to not use italic for the placeholder text. This made the textbox jumping when entering a text. The grid had the same problem.

The other things are unchanged except the indent.

Comment 10

[Florian Quèze [:florian]]

(In reply to Richard Marti [:Paenglab] from comment #9)

> The other things are unchanged except the indent.

So using <label>text</label> instead of <label value="text"/> didn't help with the wrapping for long localized labels?

Comment 11

[Richard Marti (:Paenglab)]

I'm not sure if it was the right way, I tried it with document.createTextNode() and label.appendChild(), but it didn't wrap.

Is this the right way or exists a other?

Comment 12

[Florian Quèze [:florian]]

(In reply to Richard Marti [:Paenglab] from comment #11)
> I'm not sure if it was the right way, I tried it with
> document.createTextNode() and label.appendChild(), but it didn't wrap.
> 
> Is this the right way or exists a other?

That seems right. Another way is to just use label.textContent but that's just a shortcut doing the same thing.

Comment 13

[Patrick Cloke [:clokep]]

(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> (In reply to Richard Marti [:Paenglab] from comment #11)
> > I'm not sure if it was the right way, I tried it with
> > document.createTextNode() and label.appendChild(), but it didn't wrap.
> > 
> > Is this the right way or exists a other?
> 
> That seems right. Another way is to just use label.textContent but that's
> just a shortcut doing the same thing.

https://developer.mozilla.org/en-US/docs/XUL/label#Wrapping suggests using node.textContent, for what it's worth.

Comment 14

[Florian Quèze [:florian]]

(In reply to Richard Marti [:Paenglab] from comment #11)
> I'm not sure if it was the right way, I tried it with
> document.createTextNode() and label.appendChild(), but it didn't wrap.
> 
> Is this the right way or exists a other?

I tried replacing the "label.setAttribute("value", aLabel);" line with either:
label.appendChild(document.createTextNode(aLabel));
or
label.textContent = aLabel;

Both work and produce the exact same result:
http://i.imgur.com/xSfKZ.png
The label wraps, making the horizontal scrollbar disappear, but the controls aren't usable at that size. We will need something more. My first idea (and trying in DOM Inspector shows it seems to work) is to add a min-width to the second column of the grid.

Comment 15

[Richard Marti (:Paenglab)]

Attachment: patch v3

I've used now label.textContent = aLabel; for the labels in grid. The columns have now IDs (label-column and value-column). The second column has now a min-width like in comment 14 proposed.

Comment 16

[Florian Quèze [:florian]]

Comment on attachment 662215 [details] [diff] [review]
patch v3

This looks great in the account wizard.

The account settings window would like to benefit from the same improvements (using a grid, wrapping labels, and a min width for the fields) but if you prefer handling this in a separate bug it's fine with me.

Comment 17

[Richard Marti (:Paenglab)]

I think in account settings window showing the labels above the textboxes should stay as it is. We have enough vertical space to show all and the textboxes are long to show the full content at once.

Comment 18

[Florian Quèze [:florian]]

(In reply to Richard Marti [:Paenglab] from comment #17)
> I think in account settings window showing the labels above the textboxes
> should stay as it is. We have enough vertical space to show all and the
> textboxes are long to show the full content at once.

Fair enough.
I think making the same changes in the account settings window would be an improvement for consistency (both UIs would look exactly the same) and for potential very long labels (maybe some protocols added as add-ons could have them? Or in some locales?) but as I said, I won't require these changes.
The current patch is already a great improvement, thanks for making it!

Comment 19

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 662215 [details] [diff] [review]
patch v3

I agree, this is an improvement.  ui-r=me!

Thanks,
Blake.

Comment 20

[Richard Marti (:Paenglab)]

Comment on attachment 662215 [details] [diff] [review]
patch v3

[Approval Request Comment]
It would be nice for the users if they have this already in TB 17 for better usability and don't have to wait one year.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/432feab3693c

Comment 22

[Florian Quèze [:florian]]

https://hg.mozilla.org/releases/comm-aurora/rev/dc65b9f42ac9