Closed Bug 1565703 Opened 1 year ago Closed 1 year ago

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

Categories

(MailNews Core :: Account Manager, defect, critical)

defect
Not set
critical

Tracking

(thunderbird68+ fixed, thunderbird69 fixed, thunderbird70 fixed)

RESOLVED FIXED
Thunderbird 70.0
Tracking Status
thunderbird68 + fixed
thunderbird69 --- fixed
thunderbird70 --- fixed

People

(Reporter: aceman, Assigned: darktrojan)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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?

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?

Flags: needinfo?(khushil324)
Attached patch 1565703.patch merge hidefor (obsolete) — Splinter Review

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

Attachment #9077841 - Flags: review?(khushil324)

Uff, so broken in TB 68?

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

Assignee: nobody → khushil324
Flags: needinfo?(khushil324)
Attached patch 1565703.patch visibility:hidden (obsolete) — Splinter Review

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>.

Attachment #9077841 - Attachment is obsolete: true
Attachment #9077841 - Flags: review?(khushil324)
Attachment #9078043 - Flags: review?(richard.marti)
Attachment #9078043 - Flags: review?(jorgk)
Attachment #9078043 - Flags: review?(geoff)

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.

Severity: normal → critical
Regressed by: 1546281

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

Why is a table necessary?

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

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.

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.

Attached patch 1565703-server-grid-1.diff (obsolete) — Splinter Review

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.

Attachment #9078057 - Flags: review?(richard.marti)
Attachment #9078057 - Flags: review?(acelists)
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)
Attachment #9078057 - Flags: review?(richard.marti)
Attachment #9078057 - Flags: review?(acelists)
Attachment #9078057 - Flags: review-
Attachment #9078043 - Attachment is obsolete: true
Attachment #9078043 - Flags: review?(richard.marti)
Attachment #9078043 - Flags: review?(jorgk)
Attachment #9078043 - Flags: review?(geoff)

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

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

Assignee: khushil324 → geoff
Attachment #9078057 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #9078083 - Flags: review?(mkmelin+mozilla)

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

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
Attachment #9078083 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → Thunderbird 70.0
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.

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

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.

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

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).

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/26b1781a4483
Hide unused port settings elements correctly. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Regressions: 1566281
Pushed by mozilla@jorgk.com:
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
You need to log in before you can comment on or make changes to this bug.