Open Bug 1238271 Opened 7 years ago Updated 3 years ago

Add "showfor" attribute for elements in the account manager UI

Categories

(MailNews Core :: Account Manager, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: aceman, Assigned: aceman)

Details

Attachments

(1 file)

There exists a "hidefor" attribute on elements in the Account manager UI that hides settings/controls that are not applicable for the server type of the current account. See hideShowControls() in AccountManager.js.

I think it would be useful to add the opposite "showfor" attribute for elements that should be shown only for specific server types.

I think it would be more correct to use the new attribute for 
e.g. the several options relevant only for IMAP.
Currently we use "hidefor=pop3,nntp,movemail" but that is not very scalable if we add new server types in the future. Changing to "showfor=imap" would solve the problem.

I'm not sure how JSAccounts or MS Exchange-providing-addons handle this. I would be interested to know if they can use this in any way. Unless they just use their own xul file for the account manager so none of these problems apply (I think that is what IM does).
Agreed, the negative logic is always confusing and you always have to think twice not to miss a type.
> I'm not sure how JSAccounts or MS Exchange-providing-addons handle this. I
> would be interested to know if they can use this in any way. Unless they
> just use their own xul file for the account manager so none of these
> problems apply (I think that is what IM does).

Assuming that "hidefor" should be retained for reasons of backwards compatibility with any add-ons using it, how would a conflict between "hidefor" and "showfor" be resolved if it's encountered?
"I'm not sure how JSAccounts or MS Exchange-providing-addons handle this."

One way is folderProps.xul is overridden so that certain items are hidden if the server type is ExQuilla:

    // We'll hide non-imap controls plus a few extra
    hideShowControls("imap");
    let hideme = ["SharingTab",
                  "QuotaTab",
                 ];
    hideme.forEach(function(id) {
      document.getElementById(id).hidden = true;
    });

In other words, you overlay JS to kludge the problem. I agree that a showfor would be a better solution. Keep in mind that you need some clear method that an extension can use to affect it without having to completely replace a JS function.

AFAIK there are no real users of new account types other than me, and one guy trying to a full C++ implementation of EWS that has very few users. I don't think that backwards compatibility is a major problem.

For JSAccounts, the plan is to get the minimum C++ and .idl pieces in place for TB 45, then be somewhat aggressive about fixing issues in the TB 45 cycle as we do some test implementations of different account types. There will be needed pieces that are not landed initially, but can hopefully be added later, even if that means breaking the few implementations. If you are using JSAccounts in TB 45 you should expect some instability.
(In reply to rsx11m from comment #2)
> Assuming that "hidefor" should be retained for reasons of backwards
> compatibility with any add-ons using it, how would a conflict between
> "hidefor" and "showfor" be resolved if it's encountered?

"hidefor" will be preserved as it does make sense in many cases (or sometimes I just don't know so I can leave elements as they are).

If both elements are present that would be a programming error so we could show a warning. I do not propose to do any analysis if the attribute values really do conflict (e.g. showfor=X + hidefor="all-X" would not conflict, but the determination of what "all" is (all server types) could be problematic).
(In reply to Kent James (:rkent) from comment #3)
>     // We'll hide non-imap controls plus a few extra
>     hideShowControls("imap");
>     let hideme = ["SharingTab",
>                   "QuotaTab",
>                  ];
>     hideme.forEach(function(id) {
>       document.getElementById(id).hidden = true;
>     });
> 
> In other words, you overlay JS to kludge the problem. I agree that a showfor
> would be a better solution. Keep in mind that you need some clear method
> that an extension can use to affect it without having to completely replace
> a JS function.

Thanks, I see now. You could also do getElementById("SharingTab").setAttribute("hidefor","ExQuilla") (or actually append the value to the existing list) and then run hideShowControls("ExQuilla"), which would be semantically more correct as you would not pretend to be imap.

Actually for your case if the current definition is:
<tab id="QuotaTab" hidefor="movemail,pop3,rss,none,nntp"/>

In base TB, I will convert it to: 
<tab id="QuotaTab" showfor="imap"/>

And then hideShowControls("ExQuilla") would actually do the right job without you needing to hack the attribute.
Attached patch patchSplinter Review
Would this work for you?
Attachment #8737582 - Flags: feedback?(rkent)
Comment on attachment 8737582 [details] [diff] [review]
patch

I assume like Kent will not respond anytime soon.
Magnus, Alta, I think you are interested in the account manager, maybe you'll have an opinion here. Thanks
Attachment #8737582 - Flags: feedback?(rkent)
Attachment #8737582 - Flags: feedback?(mkmelin+mozilla)
Attachment #8737582 - Flags: feedback?(alta88)
Comment on attachment 8737582 [details] [diff] [review]
patch

Yes, showFor is a good addition and practice (I implemented it in the port of AM to html ;).

It's nice you consider the case of |hidden| being or not being a node property, but doesn't the base class for dom nodes have |hidden|? If using the property, it should then be ensured the attribute is set for css purposes, which I think is also always done.

I think it's a good practice to also set the disabled property if the element is hidden, so a form node input can't be filled. Also, should such nodes be hidden by default, so as never to be in a situation where they are even briefly shown falsely?
Attachment #8737582 - Flags: feedback?(alta88) → feedback+
Comment on attachment 8737582 [details] [diff] [review]
patch

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

I agree opting in is better than opting out, so in that regard it's preferable.

But this whole thing is an odd beast. Why on earth are we hiding/showing things using JavaScript? I also quite dislike adding custom attributes like this to alter behavior, as it's not at all clear for someone looking at this what it will do, without going into research mode.

The normal thing in the web world would be to either not build unneeded stuff into the dom to begin with, or if done, hide/show using CSS.

What I think you should do is to get rid of the hidefor altoghethere, add instead class="account-type-specific" to relevant items. If it's for imap, class="account-type-specific imap". Then you can easily have css rules like
.account-type-specific { display: none; } 
.account-type-specific.imap { display: initial; }
Attachment #8737582 - Flags: feedback?(mkmelin+mozilla)
(In reply to alta88 from comment #8)
> It's nice you consider the case of |hidden| being or not being a node
> property, but doesn't the base class for dom nodes have |hidden|?

I'm not sure but I could check.

> I think it's a good practice to also set the disabled property if the
> element is hidden, so a form node input can't be filled.

OK, that could be added, but wasn't done till now on the hidden elements.

> Also, should such
> nodes be hidden by default, so as never to be in a situation where they are
> even briefly shown falsely?

Yes, it could happen that elements show and then are hidden immediately.
How would you do this? I can imagine a solution where all elements are hidden by default in the xul file. And only upon display the needed elements are exposed. But this would be slower and potentially more jumpy as all elements would suddenly appear (instead of the current state where all elements appear and a few get hidden).

Also, this is not a problem of this patch, it already exists ;)

(In reply to Magnus Melin [:mkmelin] from comment #9)
> But this whole thing is an odd beast. Why on earth are we hiding/showing
> things using JavaScript? I also quite dislike adding custom attributes like
> this to alter behavior, as it's not at all clear for someone looking at this
> what it will do, without going into research mode.
> 
> The normal thing in the web world would be to either not build unneeded
> stuff into the dom to begin with, or if done, hide/show using CSS.

Fortunately we are not in the web world yet.
The stuff has to be there as we use a single xul file (or set of files) for all account types and only hide elements as needed. Do you suggest to make a set of files for each account type (and addons providing their own) ???
 
> What I think you should do is to get rid of the hidefor altoghethere, add
> instead class="account-type-specific" to relevant items. If it's for imap,
> class="account-type-specific imap". Then you can easily have css rules like
> .account-type-specific { display: none; } 
> .account-type-specific.imap { display: initial; }

Will this allow e.g. class="account-type-specific imap pop3"? And what about the 'hidefor' feature? How do you display an element when the account type is NOT in the class? Do we create _not_IMAP class with {display:initial}? Or do we kill that feature and only allow 'showfor' and make every element have a specific list of types it applies to? Forcing addons to opt into fields they want?
Would addons need to supply their own css rule .account-type-specific.addon_type { display: initial; }?
(In reply to :aceman from comment #10)
> The stuff has to be there as we use a single xul file (or set of files) for
> all account types and only hide elements as needed. Do you suggest to make a
> set of files for each account type (and addons providing their own) ???

No, but you'd dynamically set a class for a higher level node in the tree. Let's say class="account-type imap".

> > What I think you should do is to get rid of the hidefor altoghethere, add
> > instead class="account-type-specific" to relevant items. If it's for imap,
> > class="account-type-specific imap". Then you can easily have css rules like
> > .account-type-specific { display: none; } 
> > .account-type-specific.imap { display: initial; }

This was incomplete, but something like

.account-type.imap account-type-specific.imap { display: initial; }

> Will this allow e.g. class="account-type-specific imap pop3"?

Sure

.account-type.pop3 account-type-specific.pop3 { display: initial; }

> And what about > the 'hidefor' feature? How do you display an element when the account type
> is NOT in the class?
> Do we create _not_IMAP class with {display:initial}? 

No. But of course there's css trickery with ::not() you could apply as needed.
 
> do we kill that feature and only allow 'showfor' and make every element have
> a specific list of types it applies to? Forcing addons to opt into fields
> they want?

The motivation for this bug was not to do that anymore, right? To cater for extra account types. So hidefor should be removed.

> Would addons need to supply their own css rule
> .account-type-specific.addon_type { display: initial; }?

Exactly.
(In reply to Magnus Melin [:mkmelin] from comment #11)
> (In reply to :aceman from comment #10)
> > The stuff has to be there as we use a single xul file (or set of files) for
> > all account types and only hide elements as needed. Do you suggest to make a
> > set of files for each account type (and addons providing their own) ???
> No, but you'd dynamically set a class for a higher level node in the tree.
> Let's say class="account-type imap".

Dynamically? So how is that different from what the JS does now? And why is this new class for higher level needed?
 
> > > What I think you should do is to get rid of the hidefor altoghethere, add
> > > instead class="account-type-specific" to relevant items. If it's for imap,
> > > class="account-type-specific imap". Then you can easily have css rules like
> > > .account-type-specific { display: none; } 
> > > .account-type-specific.imap { display: initial; }
> 
> This was incomplete, but something like
> 
> .account-type.imap account-type-specific.imap { display: initial; }

What?
 
> > Will this allow e.g. class="account-type-specific imap pop3"?
> 
> Sure
> 
> .account-type.pop3 account-type-specific.pop3 { display: initial; }

I mean, if the class cna be set so that the field shows for IMAP AND POP3 too (but not others).
 
> > And what about > the 'hidefor' feature? How do you display an element when the account type
> > is NOT in the class?
> > Do we create _not_IMAP class with {display:initial}? 
> 
> No. But of course there's css trickery with ::not() you could apply as
> needed.

Any example?
  
> > do we kill that feature and only allow 'showfor' and make every element have
> > a specific list of types it applies to? Forcing addons to opt into fields
> > they want?
> The motivation for this bug was not to do that anymore, right?
> To cater for extra account types. So hidefor should be removed.

Not to do what? If hidefor is removed, every element will have a "showfor" (or a class) attached for which types it should display. So any addon would need to go through the whole document and add its own class to elements it wants. I'm not against that, it just needs addons to adapt.
 
> > Would addons need to supply their own css rule
> > .account-type-specific.addon_type { display: initial; }?
> 
> Exactly.

Why is that better than handling it automatically? On the the other hand, if the css can be generated automatically from the account type, we could do that internally.


Also, if you do all this via css, the elements will not have their 'hidden' and 'disabled' attributes set, which is quite against what alta has said. They will still be active and operatable, just visually hidden by css.
(In reply to :aceman from comment #12)
> (In reply to Magnus Melin [:mkmelin] from comment #11)
> > (In reply to :aceman from comment #10)
> > > The stuff has to be there as we use a single xul file (or set of files) for
> > > all account types and only hide elements as needed. Do you suggest to make a
> > > set of files for each account type (and addons providing their own) ???
> > No, but you'd dynamically set a class for a higher level node in the tree.
> > Let's say class="account-type imap".
> 
> Dynamically? So how is that different from what the JS does now? And why is
> this new class for higher level needed?

Of course there needs to be dynamics, else you can't display anything. But you only set it on one node, instead of looking through all elements.

CSS is designed exactly to be able to do things like this. Looking up nodes, checking for attributes, and then hiding is possible, but just not optimal. With CSS it's bound to be a magnitude faster too. The idea is to let each component do what it does best, and this is css land.

> > > > What I think you should do is to get rid of the hidefor altoghethere, add
> > > > instead class="account-type-specific" to relevant items. If it's for imap,
> > > > class="account-type-specific imap". Then you can easily have css rules like
> > > > .account-type-specific { display: none; } 
> > > > .account-type-specific.imap { display: initial; }
> > 
> > This was incomplete, but something like
> > 
> > .account-type.imap account-type-specific.imap { display: initial; }
> 
> What?

Meaning that if we're displaying the pane for an imap account, and the child element is imap specific, show the element.

> > > Will this allow e.g. class="account-type-specific imap pop3"?
> > 
> > Sure
> > 
> > .account-type.pop3 .account-type-specific.pop3 { display: initial; }
> 
> I mean, if the class cna be set so that the field shows for IMAP AND POP3
> too (but not others).

Of course, you just comma the selectors

.account-type.pop3 .account-type-specific.pop3,
.account-type.imap .account-type-specific.imap { display: initial; }

>  
> > > And what about > the 'hidefor' feature? How do you display an element when the account type
> > > is NOT in the class?
> > > Do we create _not_IMAP class with {display:initial}? 
> > 
> > No. But of course there's css trickery with ::not() you could apply as
> > needed.
> 
> Any example?

For non-imap

.account-type-specific:not(.imap) { ... }

> it just needs addons to adapt.

Yes, but there's only one, if any. jsaccount isn't even working properly anymore AFAIR...
 
> > > Would addons need to supply their own css rule
> > > .account-type-specific.addon_type { display: initial; }?
> > 
> > Exactly.
> 
> Why is that better than handling it automatically? On the the other hand, if
> the css can be generated automatically from the account type, we could do
> that internally.

Why? See comment above. 

Anyway, this is all-in-all just less than 10 lines of css, so there's no need to generate anything.
That would also go against the goal here, that new account types would opt in for what they need/support.

> Also, if you do all this via css, the elements will not have their 'hidden'
> and 'disabled' attributes set, which is quite against what alta has said.
> They will still be active and operatable, just visually hidden by css.

Pretty sure they aren't. How would you operate on a control that's hidden? This is status quo too and I haven't heard of any problems.
> 
> > Also, if you do all this via css, the elements will not have their 'hidden'
> > and 'disabled' attributes set, which is quite against what alta has said.
> > They will still be active and operatable, just visually hidden by css.
> 
> Pretty sure they aren't. How would you operate on a control that's hidden?
> This is status quo too and I haven't heard of any problems.

When reusing forms pages (to use html terminology) for different account types, you have to ensure controls (inputs, checkboxes) are cleared/reset. Otherwise if the design is to gather up inputs and save them on Accept, as I think it is in accountmanager code, then there could be an issue. Not saying there *is* an issue, just noting better practices and things to think about rather than practicing the development by fifty followup bugs method.
Nice bug, and patch! Thumbs up
Comment on attachment 8737582 [details] [diff] [review]
patch

>+      if ("hidden" in control)
All XUL controls (indeed, elements) have a "hidden" property.

(In reply to Magnus Melin from comment #9)
> But this whole thing is an odd beast. Why on earth are we hiding/showing
> things using JavaScript?

While it would be nice semantically to be able to set the server type as an attribute on the page and let CSS hide or show anything, the current approach of using hidefor attributes does make it much easier for an extension to change which controls are visible.

Directly hiding the controls as per comment #3 is awkward as you have to do it on every account change and you also have to remember to show any controls that only your account type hides; updating hidefor attributes on all the controls you don't use is much easier.

An additional advantage of hidefor is that you can easily add it to controls that all the built-in accounts show, so while adding showfor would be useful, you wouldn't want to remove hidefor.

The only other idea I had was to have a method on the incoming server to identify which controls to show but that doesn't handle containers neatly.
(In reply to neil@parkwaycc.co.uk from comment #16)
> While it would be nice semantically to be able to set the server type as an
> attribute on the page and let CSS hide or show anything, the current
> approach of using hidefor attributes does make it much easier for an
> extension to change which controls are visible.

Can you explain why? Sure there's likely to be some boilerplate css code to copy for an add-on, but I would imagine it's quite straight forward.

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

(In reply to neil@parkwaycc.co.uk from comment #16)

While it would be nice semantically to be able to set the server type as an
attribute on the page and let CSS hide or show anything, the current
approach of using hidefor attributes does make it much easier for an
extension to change which controls are visible.

Can you explain why? Sure there's likely to be some boilerplate css code to
copy for an add-on, but I would imagine it's quite straight forward.

Yes, I also think if we can make the css method work, then the addon can add classes to needed elements in the same way as it can add them into showfor/hidefor attributes.

We just need to make the css rules work. Can anybody pull it off? But I do not agree addons should add some rules themselves. If we just need to generate rules in the form:
.account-type-specific { display: none; }
.account-type-specific.imap { display: initial; }
.account-type-specific::not(.imap) { display: none; }

Can anybody imagine what exact rules are needed? Magnus mentioned trickery with ::not(), but that is not sure if really css can support the cases we need to cover. Paenglab, would you know what the roles need to look like?
I also haven't yet understood why we need the ".account-type.imap account-type-specific.imap" rules.

Then we the AM code can do that automatically when switching panes in the AM. Every pane belongs to a specific account/server type and the AM knows what it is. So no need for addons to do anything, AM can generate the rules for any encountered server type, it just uses the string from account.incomingServer.type to build the rules.

If you don't think add-ons should add the rules themselves I think you miss the point. That is what allows add-ons to opt in or out of what they want, and I think, in a pretty simple way. If the AM would generate the rules, we need a way to specify the rules and feed them to the AM. The css rules ARE the rules, and then it's already done.

The addons opt in by properly setting the classes on their input elements. But I see no reason they would also need to generate/ship their own .account-type-specific.* rules, if the AM can do it automatically.

What do you mean setting the classes on their input elements? If you're talking about the showfor/hidefor, that's exactly what we're trying to get away from, right?

If you're talking about the showfor/hidefor, that's exactly what we're trying to get away from, right?

No, this bug is about adding a showfor="imap" instead of hidefor="movemail,pop3,none,rss,nntp". Did you see the patch?

-        <button hidefor="movemail,pop3,none,rss,nntp"
+        <button showfor="imap"

showfor makes it a lot easier to add a new account type than hidefor.

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

What do you mean setting the classes on their input elements? If you're talking about the showfor/hidefor, that's exactly what we're trying to get away from, right?

No, you haven't proposed anything semantically different from showfor/hidefor attributes, you just propose to do the hiding via css and classes on the input elements (instead of attributes). So the addon just sets classes on the elements instead of the attributes. But the needed css rules can be generated by the AM code. Or what am I missing?

(In reply to Magnus Melin from comment #17)

(In reply to neil@parkwaycc.co.uk from comment #16)

While it would be nice semantically to be able to set the server type as an attribute on the page and let CSS hide or show anything, the current approach of using hidefor attributes does make it much easier for an extension to change which controls are visible.

Can you explain why? Sure there's likely to be some boilerplate css code to copy for an add-on, but I would imagine it's quite straight forward.

Ah, so what you're saying that we'd add CSS classes to all the elements that we're currently adjusting the hidefor attribute on? Sorry, I misunderstood you in that case.

FYI this patch reduces the number of elements in the server pane that Owl currently has to adjust the hidefor attribute on from 22 to 8.

(In reply to neil@parkwaycc.co.uk from comment #24)

Ah, so what you're saying that we'd add CSS classes to all the elements that we're currently adjusting the hidefor attribute on? Sorry, I misunderstood you in that case.

I also thought this is the case (and that would be basically the same effort and logic, if we can get the css rules right), but now Magnus sounds like there is something more to it.

FYI this patch reduces the number of elements in the server pane that Owl currently has to adjust the hidefor attribute on from 22 to 8.

Nice, thanks :)

You need to log in before you can comment on or make changes to this bug.