Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Thunderbird 18.0

Status

Thunderbird
Instant Messaging
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aryx, Assigned: Paenglab)

Tracking

Trunk
Thunderbird 18.0
ARM
Android
Bug Flags:
in-testsuite -

Thunderbird Tracking Flags

(thunderbird17 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Created attachment 657544 [details]
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.
Created attachment 657545 [details]
account settings cut off
(Assignee)

Comment 2

5 years ago
Created attachment 661477 [details] [diff] [review]
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.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #661477 - Flags: ui-review?(bwinton)
Attachment #661477 - Flags: review?(florian)
(Assignee)

Comment 3

5 years ago
Created attachment 661478 [details]
comparison

Comparison for easier ui-review
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.
Attachment #661477 - Flags: review?(florian) → review-
(Assignee)

Comment 5

5 years ago
(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.
(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.
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?
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
Created attachment 661913 [details] [diff] [review]
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.
Attachment #661477 - Attachment is obsolete: true
Attachment #661477 - Flags: ui-review?(bwinton)
Attachment #661913 - Flags: ui-review?(bwinton)
Attachment #661913 - Flags: review?(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?
(Assignee)

Comment 11

5 years ago
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?
(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.
(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.
(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.
(Assignee)

Comment 15

5 years ago
Created attachment 662215 [details] [diff] [review]
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.
Attachment #661913 - Attachment is obsolete: true
Attachment #661913 - Flags: ui-review?(bwinton)
Attachment #661913 - Flags: review?(florian)
Attachment #662215 - Flags: ui-review?(bwinton)
Attachment #662215 - Flags: review?(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.
Attachment #662215 - Flags: review?(florian) → review+
(Assignee)

Comment 17

5 years ago
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.
(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 on attachment 662215 [details] [diff] [review]
patch v3

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

Thanks,
Blake.
Attachment #662215 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 20

5 years ago
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.
Attachment #662215 - Flags: approval-comm-aurora?
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/432feab3693c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
Attachment #662215 - Flags: approval-comm-aurora? → approval-comm-aurora+
https://hg.mozilla.org/releases/comm-aurora/rev/dc65b9f42ac9
status-thunderbird17: --- → fixed
You need to log in before you can comment on or make changes to this bug.