1565703 - <input type="number"> port field in Server settings of a movemail account not hidden

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[:aceman]

When you create a "movemail" account (Linux only) and visit the Account settings -> Server settings, the Port field is displayed with a value of -1 and a red outline (indicating error). But this field is not relevant for movemail and should be hidden. The hidefor=movemail attribute is there and a hidden=true attribute gets properly set on the <html:input> element. But it is still visible.

Looks like some problem with <input type="number"> element. Does it properly respond to hidden attribute?

Comment 1

[:aceman]

It looks the problem is worse.
Also the subsequent <div> element around username is not hidden for nntp accounts.

Why are there even <div> elements in a xul file? It was done in bug 1546281 for converting away from <grid>. But why not proper <*box> elements? Is <div> even supposed to handle "hidden" attribute?

Comment 2

[:aceman]

Attachment: 1565703.patch merge hidefor

This simplifies the port elements, but still does not fix the bug.

Comment 3

[Jorg K (CEST = GMT+2)]

Uff, so broken in TB 68?

Comment 4

[:aceman]

Possibly, you can test the nntp (news) server case on Windows. It shows username field, while it shouldn't.

Comment 5

[:aceman]

Attachment: 1565703.patch visibility:hidden

It seems you guys experiment with 'flex' css here (I don't know why such anomaly) and
the rule #amServerSetting {display: inline-grid; } overrides the global [hidden="true"] rule from the global theme, thus the elements stayed visible.

I add the rule '#amServerSetting [hidden="true"]' but it needs to have 'visibility: hidden', not 'display: none' because otherwise the next element takes its place to fill the third column in the flex grid. With 'visibility' the element takes place, it just isn't visible.

So with this patch the hiding of the elements is restored, but with NNTP the hidden username 'row' is still taking some space so there is this slight ugliness.

But we could take this until someone wants to fight with the flex css more or rewrite it all in <html:table>.

Comment 6

[:aceman]

This must go into TB68 fast, as otherwise keeping the input boxes exposed may tempt users to fill them in and then strange things may happen.

Comment 7

[Khushil Mistry [:khushil324]]

I can rewrite this patch with html:table by tomorrow.

Comment 8

[Jorg K (CEST = GMT+2)]

Why is a table necessary?

Comment 9

[alta88]

(In reply to Jorg K (GMT+2) from comment #8)

Why is a table necessary?

+1. It is very well known in html-land that <table> has been misused through the ages, and is the reason css grid was invented. Plus, grid lets you easily handle responsive design mode (realigning elements based on viewport width, etc). Although this needs a fast bandaid, we really need to get a strategy.

Comment 10

[:aceman]

We have a fast bandaid available. If anybody comes up with something better, then is does not need to be used.
I didn't insist on table. If somebody can fix the flex grid, then great, even though the syntax is a total mess and I won't touch it in the future. XUL grid or HTML table is always easier to grasp.

Comment 11

[alta88]

offtopic: I'm not objecting to a bandaid, but I've seen <table> used elsewhere and that means there's no strategy, which I do object to. I'm pretty sure a pure html/css/js web page jockey could grind out the simple forms that are 90% of the Account Manager UI. XUL grid is dead, it doesn't matter how much we loved it. ;)
btw, +1000 for clientid.

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: 1565703-server-grid-1.diff

In my opinion this is a tidier way, as it specifies in the XUL how it should be laid out (unfortunately using inline CSS). Also, visibility: hidden leaves a gap below this section for NNTP accounts.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9078057 [details] [diff] [review]
1565703-server-grid-1.diff

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

Like the approach, but slightly wrong

::: mailnews/base/prefs/content/am-server.css
@@ +6,5 @@
>    display: inline-grid;
>    grid-template-columns: auto auto auto;
>  }
>  
> +#amServerSetting div:not([hidden="true"]) {

should just be [hidden]

hidden is a boolean attribute, and in would be hidden="hidden" (or not set at all) in html

::: mailnews/base/prefs/content/am-server.xul
@@ +39,5 @@
>      <hbox class="dialogheader">
>        <label class="dialogheader-title" value="&serverSettings.label;"/>
>      </hbox>
>  
> +    <div id="amServerSetting">

you shouldn't remove the xmlns="http://www.w3.org/1999/xhtml" declaration here, that will make these divs xul:div which is not a thing (though that undefined element may behave like a box by chance)

Comment 14

[Magnus Melin [:mkmelin]]

(In reply to alta88 from comment #11)

offtopic: I'm not objecting to a bandaid, but I've seen <table> used
elsewhere and that means there's no strategy, which I do object to.

Looks like one or two unwarranted cases snuck in, but agreed table shouldn't be used for layout.

Comment 15

[Geoff Lankow (:darktrojan)]

Attachment: 1565703-server-grid-2.diff

I modified aceman's patch, so I blame him. ;-) Good points though.

Comment 16

[Jorg K (CEST = GMT+2)]

Can we get this landed today, so we can ship it with TB 68 beta 5 which is delayed.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 9078083 [details] [diff] [review]
1565703-server-grid-2.diff

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

Looks good to me, r=mkmelin

Comment 18

[:aceman]

Comment on attachment 9078083 [details] [diff] [review]
1565703-server-grid-2.diff

Thanks, this works good.
But for movemail, when the port elements are hidden, there becomes a big gap between the labels and the inputboxes, probably because the second column of the grid expands to the right.
This is similar to what we had to fight with in bug 1561390, we need the first column to only fit the content width, and then the second column span the whole remaining width.

Comment 19

[:aceman]

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

+#amServerSetting div:not([hidden="true"]) {

should just be [hidden]

hidden is a boolean attribute, and in would be hidden="hidden" (or not set
at all) in html

The code managing 'hidefor' will add hidden=true as all the elements are supposed to be XUL. Now that we have a mess of html/xul, the things are starting to fall apart. '[hidden]' is fine with me, until there appears an element having hidden=false in the future :)
Is there a good way to query an element whether it is XUL or HTML, e.g. using some namespace? Then we could fix the code to add hidden=hidden where needed.

Comment 20

[Magnus Melin [:mkmelin]]

You can always check Element.namespaceURI but I don't think it's needed. There are not that many boolean attributes, so it's just a matter of checking those.

Comment 21

[Jorg K (CEST = GMT+2)]

Aceman, I'll land this now. Maybe you can post a follow-up patch for comment #18.

Comment 22

[:aceman]

Yes, land Geoff's version please, which isn't perfect, but the ugliness is only seen with movemail, which is used rarely (compared to nntp).

Comment 23

[Jorg K (CEST = GMT+2)]

TB 68 beta 5:
https://hg.mozilla.org/releases/comm-beta/rev/1ced37a21841ea21406900036bdcdb8930f86402

TB 69 beta:
https://hg.mozilla.org/releases/comm-beta/rev/a1430256e067950d4f90b666314b814b1f1b1c52

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/26b1781a4483
Hide unused port settings elements correctly. r=mkmelin

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/11bf8f77ab5e
Fix the input[type="number"] width in AM and make the first column of the amServerSetting smaller. r=aceman,mkmelin

Comment 26

[Jorg K (CEST = GMT+2)]

That should have gone into bug 1566281, so I backed it out again :-(
https://hg.mozilla.org/comm-central/rev/6ab99bb2d71a9343480f7f10f2333c67bf4d7ac8