Closed Bug 262592 Opened 20 years ago Closed 19 years ago

Enable templates to use a cookies to remember UI preferences and hide/expose content

Categories

(Bugzilla :: User Interface, enhancement, P3)

2.19
enhancement

Tracking

()

RESOLVED FIXED
Bugzilla 2.22

People

(Reporter: bugreport, Assigned: u197037)

Details

Attachments

(3 files, 5 obsolete files)

Site customizations often need to make template customizations to simplify parts
of the user interface.  Since some users want an extremely simple interface
while others want the full "power-user" interface, templates should be able to
control a cookie to remember UI preferences and make presentation decisions baed
on those preferences.

This logic needs to be available to the templates without adding it to every
script.  One of the standard CGIs (CGI.pm or Template.pm) should 
a) check for the existance of a SetBugzillaPrefs form variable and, if found,
create/update the Bugzilla_Prefs cookie.
b) if the Bugzilla_Prefs cookie is found, pass it to the templates in the form
of a hash.

So, the cookie could be
Bugzilla_Prefs: bugentry,advanced,warnsecure,yes

And a template could make decisions using test like 
[% IF prefs.bugentry == 'advanced' %]

And, when a form variable SetBugzillaPrefs is processed with a value...
bugentry,simple,alwaysccme,yes

the cookie would subsequently become...
Bugzilla_Prefs: bugentry,simple,warnsecure,yes,alwaysccme,yes
OK, this can be done more easily....

Set the cookies with javascript and read them by....


	justdave	[% USE Bugzilla %]
	joel	like pass the CGI object to the templates
	justdave	[% cgi = Bugzilla.cgi %]
	justdave	[% mypref = cgi.cookie("mycookiename") %]
	joel	I didn't know we could do a USE in a template
	justdave	we can for the Bugzilla object
	justdave	that's what Bugzilla/Template/Plugin/Bugzilla is for :)
	justdave	USE in a template loads a template plugin
	justdave	we have a plugin that supplies access to the Bugzilla object
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → WONTFIX
Actually, I will both rename and reopen this....

The new plan is to have a js/ file that can be easily included in templates to
help them manage a cookie and hide/expose content.  The initial application of
this is to facilitate a site customization that presents a stripped down entry
forms to new users, but lets more advanced users use the full form.

So, this will be a javascript library with functions to 
1) hide/reveal document elements of a specified class
2) update cookies from buttons...  initialize button appearance to indicate
current state of cookies.
3) onload, check cookies and call hide/reveal code

The code should be able to live in js/* with no code calling it.  It should be
very easy to 
a) plant code in the user_prefs templates to create a bunch of buttons to
set/reset these cookies.
b) plant code in any Bugzilla template to use the cookies onload to hide/reveal
form elements of a particular class.



Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Summary: Enable templates to use a Bugzilla_Prefs cookie to remember UI preferences → Enable templates to use a cookies to remember UI preferences and hide/expose content
This is a hacked up create template that has Simple and Advanced buttons,
maintain the prior state via cookies, and hides/reveals the dependencies rows
(indicated by the "advanced" class) based on the state of simple versus
advanced.

This needs to be factored out and modularized, but I think it shows the
concept.
Status: REOPENED → ASSIGNED
Assignee: bugreport → dennis.melentyev
Status: ASSIGNED → NEW
Priority: -- → P3
Target Milestone: --- → Bugzilla 2.22
Status: NEW → ASSIGNED
Priority: P3 → --
Target Milestone: Bugzilla 2.22 → ---
Just to clarify the intent in comment 3, ideally this would work as follows...

1) In a site with no need to use these cookies, no template customization would
invoke any of the new code, so it would not affect the site.

2) Adding code to templates to either hide/reveal sections based on the class or
to would involve adding only a very simple block like
[% PROCESS cookiehide cookie = "advanced" class = "advancedclass" %]

and buttons like 
    <input type="button" value="advanced" onclick='return reveal("advanced")'>
    <input type="button" value="simple" onclick='return hide("advanced")'>

[Warning - my example here may be very incorrect, but is intended to convey the
general intent]
Yes, I think the same way: adding particular class name as an argument to
hide/reveal function will let template developer to add as many hidable sections
on the page as wanted. With separate buttons to hide/reveal each section.
Also this would need tunable cookie sets.
In order to do this, the hide/reveal "API" should receive not only the class
name, but some cookie suffix. 
So, cookie name could end up in something like:
BugzillaUI_<suffix>_<class>=hide (or =show, or =visible, or =reveal...)
This allow developer to create both common cross page cookies and the per-page ones.
PS. I get back the Priority=P3 and Milestone=2.22 values set by you, Joel
Priority: -- → P3
Target Milestone: --- → Bugzilla 2.22
These are template files packed in one diff
Minimalistic test.cgi and template for it. Provides test capabilities
Comment on attachment 181526 [details] [diff] [review]
new files to provide hide/reveal/collapse functionality


Initial comments by inspection...:

1) We should use the cookiepath parameter to distinguish between multiple sites
on the same host.  It is available to template code as Param("cookiepath") and
will be blank on many systems.

2) Since the function being called in TUIButton is already known to be a
"button,"  it does not need to be called "btnname."  "name" would be much more
intuitive.

3) We really need to figure out where these templates should go.  global/TUI*
does not sound right, but I don't have a clear idea at the moment.

4) If the cookie is not previously defined, could we tell TUIProcess what we
want to use for the default behavior?  If we do this, we don't just want to act
on the absence of the cookie because some browser configurations will not let
us set cookies from javascript.  Rather, if the cookie is absent, we want to
set it to a specified value and then look at it.  The example for this would be
a UI change that hides some elements (by default) unless a user enables an
advanced mode.	If we cannot control cookies, we want the elements to remain
visible. If the user has never done anything to set the cookies, we want to
presume that the user does not want to see the elements.

I'll experiment with this later.
Ignore my previous comment about cookiepath.  It looks like javascript takes
care of this for you.

-----------
I tried validating the test page...  HTML 4.01 Transiitonal does not like <HR />
and <INPUT whatever />  

-----------

Actually, we should move the javascript functions themselves into a file in the
js/ directory and let the calling template (like invoke them with...

<script src="js/TUI.js" type="text/javascript"></script>

This will let the browser cache the scripts themselves.

followed by, for each cookie...
TUI_tweak('advanced', 'hide')



The second argument is the name of the cookie, and the 2nd argument to TUI_tweak
sets the default, then you can move the CASE statement that is currently in
TUIButton into the javascript itself and then each button can be emitted using...

<input type="button" value="Advanced" 
onclick="return TUI_change('advanced', 'reveal')">
<input type="button" value="Simple" 
onclick="return TUI_change('advanced', 'hide')">

-------------------

If I look at what it would take to hide/reveal a bunch of content on the bug
creation form, I would change only
template/en/default/bug/create/create.html.tmpl to...
1) Add js/TUI.js to javascript_urls when create.html.tmpl calls
global/header.html.tmpl
2) Change some of the form elements to add the 'advanced' class to them
3) Add the call to TUI_Tweak shown above
4) Add the buttons

I tried this out with....

<table class="baa">
<tr><td>one</td></tr>
<tr class="foo simple"><td>two</td></tr>
<tr><td>three</td></tr>

And everything seems to work as well.

------

When you do the next revision of this, I suggest using a patch against the bug
creation form as the demo.  It will show exactly how little needs to be changed
without requiring you to build a whole new cgi.

(In reply to comment #5)
> So, cookie name could end up in something like:
> BugzillaUI_<suffix>_<class>=hide (or =show, or =visible, or =reveal...)
> This allow developer to create both common cross page cookies and the per-page
> ones.

A thought: to avoid cookie inflation -- can we put the cross-page cookies into
one, hash-style? Perhaps the per-page ones as well, per page...
(In reply to comment #9)
> -----------
> I tried validating the test page...  HTML 4.01 Transiitonal does not like <HR />
> and <INPUT whatever />  
> -----------
changed this to be just <input ...></input>

> Actually, we should move the javascript functions themselves into a file in the
> js/ directory and let the calling template (like invoke them with...
> 
> <script src="js/TUI.js" type="text/javascript"></script>
> 
> This will let the browser cache the scripts themselves.
Does the following looks reasonable?

....

  set defaults, without setting cookies
[% TUISuffices = [ [ 'advanced', 'collapse'], ['simple', 'show'] ] %]

  let header.html.tmpl register our TUI.js (could be hidden into TUIInit.html.tmpl)
[% javascript_urls = [ 'TUI.js' ] %]
[% PROCESS header.html.tmpl %]
....
  Add some buttons
[% PROCESS TUIButton.html.tmpl ....%]
[% PROCESS TUIButton.html.tmpl ....%]
....
  Initially process all tags, giving defaults from TUISuffices list of pairs if
cookies are not set/enabled.
[% PROCESS TUIProcess.html.tmpl %]
....

> When you do the next revision of this, I suggest using a patch against the bug
> creation form as the demo.  It will show exactly how little needs to be changed
> without requiring you to build a whole new cgi.
Ok.
(In reply to comment #10)
> (In reply to comment #5)
> > So, cookie name could end up in something like:
> > BugzillaUI_<suffix>_<class>=hide (or =show, or =visible, or =reveal...)
> > This allow developer to create both common cross page cookies and the per-page
> > ones.
> 
> A thought: to avoid cookie inflation -- can we put the cross-page cookies into
> one, hash-style? Perhaps the per-page ones as well, per page...
Could be done on per-suffix base. Something like:
"BugzillaUI_<Suffix>=class:default,class:default,class:default,...;"
The proposal in comment 11 looks good.  Please do make sure that it is possible
for a page to use TUIProcess even if that page does not have any buttons on it.
 For example, some sites may put the buttons on a seperate page.
(In reply to comment #13)
> The proposal in comment 11 looks good.  Please do make sure that it is possible
> for a page to use TUIProcess even if that page does not have any buttons on it.
>  For example, some sites may put the buttons on a seperate page.
With minor restriction of cookie with the appropriate suffix should be set and
page will redraw only after it's reload.
Even more, one should be able to put all that buttons into separate "prference"
page.
Attached patch The code itself (obsolete) — Splinter Review
Reworked patches.
Including comments both from Joel Peshkin and Marc Schumann
Attachment #181526 - Attachment is obsolete: true
The test page patch is generated against the create.html.tmpl instead of
specialy designed test.cgi
Attachment #181527 - Attachment is obsolete: true
Comment on attachment 181881 [details] [diff] [review]
The code itself


This is quite good.  My comments are mostly little minor things.

>+++ template/en/default/global/TUIInit.html.tmpl	2005-04-27 03:57:00.000000000 +0000

I was talking to Joel about the naming on these scripts, and I have a nitpicky
problem with calling them TUI*, only because a developer not familiar with this
part the source will then have to go figure out what TUI means.  Could we
instead use something like "UI_Toggle" for example?  Just something more
outwardly descriptive.

>+  [% BLOCK TUIjscript %]

This block really needs a comment describing its interface.

>+    <!--
>+    var TUISuffices = new Array;
>+    [% FOREACH suffix = TUISuffices %]

nit: s/Suffices/Suffixes/

>+++ template/en/default/global/TUIProcess.html.tmpl	2005-04-27 01:09:49.000000000 +0000

There are several small templates in use.  Would it work better if they were in
a single file, separated by BLOCKs and called using INCLUDE?

>--- /dev/null	2005-04-11 14:18:51.000000000 +0000
>+++ js/TUI.js	2005-04-27 02:55:36.000000000 +0000

>+  function split(source, delimiter) {

Could this be accomplished by calling the split method that's built in to the
javascript String object?


>+  function TUI_tweak( cookie ) {
>+    var dc = document.cookie;
>+    var begin = -1;
>+    var end = -1;
>+    var cookiePrefix = "Bugzilla_TUI_"+cookie+"="; 
>+
>+    // Process cookies, fetch ones with our prefix
>+    while(-1 != (begin = dc.indexOf(cookiePrefix, end))) {

That while line looks like more complication (in the readability sense) than
necessary.  Could the variable assignment be moved out of it?


That's about all I have so far, aside from some things Joel told me he's
already covering.
I think we are almost finished with this.  I really would like to get rid of the
templates as part of the patch.  Including the calls to the functions in the way
I show in comment 9 should be just fine and it reduced the potential for clutter
by getting this down to one .js file.  Please do make that change.

I still do need to do some more testing.
As Joel requiested, got rid of templates at all.
Encounted Erik's remark on bloated while() statement
Changed TUISuffices to TUIClasses, as it's more appropriate name
Attachment #181882 - Attachment is obsolete: true
Attached patch The code itself (obsolete) — Splinter Review
As Joel requiested, got rid of templates at all.
Encounted Erik's remark on bloated while() statement
Changed TUISuffices to TUIClasses, as it's more appropriate name
Attachment #181881 - Attachment is obsolete: true
Almost .. (very close)

The default behavior of this if the cookie has never been set is to hide the
advanced content.  Just need to fix that.
(In reply to comment #21)
> Almost .. (very close)
> 
> The default behavior of this if the cookie has never been set is to hide the
> advanced content.  Just need to fix that.

Hmm.. Works for me.

Just removed all cookies for test site in mozilla cookie manager and opened
patched enter_bug.cgi: all advanced content is collapsed, simple - shown. As it
was expected by setting defaults to "advanced:collapse,simple:show"

PS. The same behaviour in "clean" IE (all cookies were cleaned up)
PPS. Didn't I get your point right?
I said that completely backwards, but I'll retest first.

I need to make sure that the default is used if there had never been a cookie
before (and the default could be controlled) AND that the default is NOT
enforced if javascript is not permitted to set cookies.

Attached patch The code itselfSplinter Review
Revised code to handle disabled cookies policy. Now, show everything, if
cookies are not supported.
But, if cookies was set earlier and are disabled now, code will use those saved
cookie values. Despite it would be unable to set the new defaults.
Also, code will issue an alert() if user will attempt to use the controlling
buttons while cookies are disabled (in both cases).
Attachment #181962 - Attachment is obsolete: true
Comment on attachment 182161 [details] [diff] [review]
The code itself

r=joel

[The alerts should be removed on checkin -- better for it to gracefully degrade
.... also a comment should reference this bug for usage examples -- both can be
done on checkin]
Attachment #182161 - Flags: review+
Flags: approval?
Target Milestone: Bugzilla 2.22 → Bugzilla 2.20
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Do we have any pages that uses this yet?
Not in Bugzilla itself, but in derived works.
This is more in the same category as hooks where none of the standard code uses
the mechanism but it provides a standard mechanism to customizers.
ok, works for me.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago19 years ago
Flags: approval? → approval+
Resolution: --- → FIXED
er, I didn't mean to mark that fixed, not even sure how I did it...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Should I re-request approval?
As for me, it is RESOLVED FIXED, but I'm not sure should I mark it this way
myself now, or wait for final approval/cvs checkin?
Status: REOPENED → ASSIGNED
Checking in js/TUI.js;
/cvsroot/mozilla/webtools/bugzilla/js/TUI.js,v  <--  TUI.js
initial revision: 1.1
done
Status: ASSIGNED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
Thanks!
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.