On Tue, 12 Apr 2011, Kevin Cernekee wrote:
> On Sat, Apr 9, 2011 at 3:49 PM, Jakub Narebski <jnareb@xxxxxxxxx> wrote:
> >
> > This is split version (with assorted cleanups) of J.H. patch adding
> > JavaScript-base ability to change timezone in which dates are
> > displayed.
>
> Jakub,
>
> Thanks for the update. This UI does seem to work better than the
> original "[+]" dropdown interface.
Thanks.
Notice that you can have only one timezone selection menu opened. You
have to close existing to be able to open it in different place (near
different date), even though onclick handler still triggers.
This is caused by the fact that appending DocumentFragment to an element
"empties" it, as described in DOM 2 Core specification[1]:
appendChild
Adds the node newChild to the end of the list of children of this
node. If the newChild is already in the tree, it is first removed.
Parameters
newChild of type Node
The node to add.
If it is a DocumentFragment object, the entire contents of the
document fragment are moved into the child list of this node.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[1]: http://www.w3.org/TR/2000/REC-DOM-Level-2-Core-20001113/core.html#ID-184E7107
That is also the case that we need to read it when closing timezone
selection menu in removeChangeTZForm():
// timezone selection UI was appended as last child
// see also displayChangeTZForm function
var removed = container.removeChild(popup);
if (documentFragment.firstChild !== removed) { // the only child
// re-append it so it would be available for next time
documentFragment.appendChild(removed);
}
Though it wasn't what I thought it is needed for when writing i ;-)
You wrote in your response to original J.H. patch
"Re: [PATCH 1/1] gitweb: javascript ability to adjust time based on timezone"
http://thread.gmane.org/gmane.comp.version-control.git/169384/focus=169891
KC> I'm wondering if there might be a better place on the page to put the
KC> TZ selection. ÂIt isn't immediately obvious to the user what the
KC> extra " + " does, and it seems to cause some issues.
Well, I am not sure if it is immediately obvious to user that he/she
can click on date to change its timezone.
Underlining on hover (like for links... though I guess that we really
should make style similar to links but not the same: it is not true link),
changing cursor and title attribute hopefully help here.
Ideas on having it better discoverable certainly welcome.
KC> If you decide to keep it where it is, you might want to consider
KC> absolute or fixed positioning so that other elements do not wrap
KC> around it. ÂIOW it would work more like the dropdown menus on many
KC> sites.
In this version it is done using absolute positioning, though there
are few quirks and things to know about:
* To be able to position timezone menu relative to some element using
absolute positioning you need to make said element relatively
positioned.
target.style.position = 'relative';
We might have used CSS instead...
* Floating inside block element works if it has some specified size.
Normal, relative and fixed positioned elements have size given by
its parent element. For absolutely positioned block element you have
to specify width explicitly, and not have it fit to width.
That is why close-button uses absolute positioning rather than being
a float.
* The only absolute position relative to parent inline-block relative
positioned element is top left. All others doesn't work correctly
e.g. in my ancient Mozilla 1.17.2 -- in 'log' view timezone menu got
created on the right and outside viewport (page).
That is why current solution is to use "top: 0; left: 0;".
KC> The timezone fixup javascript seemed to work reasonably well, except
KC> for the hiccup with IE6. ÂMaybe it would be worth splitting this into
KC> two patches: one to rewrite the timestamps, and a second one to add
KC> the TZ selection interface.
Well, I have split in more than 2 patches, but rewriting timezone of
timestamps is separate from TZ selection interface.
> The new code appears to degrade gracefully on IE6 (everything shown in UTC).
I don't know about IE6, but there was bug in IE-specific code in
addCssRule (was 'stylesheets' when variable is named 'stylesheet').
Thanks for noticing this; fixed in new iteration.
Now at least in IE8 (Internet Explorer 8.0.6001.18702) it works as
expected.
> Tests on Firefox 3.6.15 looked OK.
Tests in the following browsers looked OK:
* Firefox 1.7.12 (Gecko/20050123)
* Epiphany 1.6.5 (Gecko/20050123)
* Firefox 4.0 - note: not as extensively tested
* Firefox 3.6.16 (Gecko/20110319)
* Internet Explorer 8.0 (8.0.6001.18702)
Tests in Konqueror 3.5.3 (KHTML) shown something strange: it has no
problems with named function expressions here
document.onclick = function onclickHandler(event) {
but was showing syntax error in similar
return function closeTZForm(event) {
that I used in new version of 10/11 patch. As we don't redeploy handler,
we don't need function name, so in current version we use
return function (event) {
Also after displaying timezone selection menu, and either selecting
timezone or canceling selection, so that timezone UI vanishes, the
date that was clicked ends up displaced a bit - moved slightly up.
This doesn't affect layout too badly, and is probably bug in this version
of Konqueror.
Tests in the following browsers gracefully degrade to UTC:
* Lynx 2.8.5rel.1
* ELinks 0.10.3
* w3m 0.5.1+cvs-1.946
>
> Chromium 6.0.472.62 (59676) does not like this operation:
>
> Uncaught Error: NOT_FOUND_ERR: DOM Exception 8
> removeChangeTZForm
> /gitweb-static/gitweb.js:785
> onTZFormChange
>
> line 785: var removed = container.removeChild(container.lastChild);
Strange. I confirm the same behavior in Google Chrome 10.0.648.204,
even after changes that I though should eliminate this exception.
Note that this bug/exception doesn't cause timezones to be not
translated, it only makes timezone selection menu not working.
removeChild() is DOM 2 Core method, `container.lastChild' exists and
is child of `container' element. IE8, Firefox 3.6 and 4.0, Opera 10.63
all work, it's only Chrome / Chromium that has problems...
> Opera 10.63 resets target.selectedIndex to 1 after calling
> removeChangeTZForm() from onTZFormChange(). Net effect is that you're
> always stuck in the "local" zone. Here is a workaround that fixed it
> for me:
>
> var target = event.target || event.srcElement;
> var selectedIndex = target.selectedIndex;
>
> removeChangeTZForm(documentFragment, target, tzClassName);
>
> var selected = target.options.item(selectedIndex);
Or just change order of those statements:
event = event || window.event;
var target = event.target || event.srcElement;
var selected = target.options.item(target.selectedIndex);
removeChangeTZForm(tzSelectFragment, target, tzClassName);
Could you check that this version works correctly in Opera 10?
Thanks in advance.
>
> Couple other random nitpicks:
>
> > + // server-side of gitweb produces datetime in UTC,
> > + // so if tz is 'utc' there is no need for changes
> > + var nochange = tz === 'utc';
>
> If I delete my gitweb_tz cookie, then refresh the page:
>
> - All times show up in my browser's local time (good)
>
> - Clicking UTC/GMT on the dropdown has no immediate effect; I need to
> refresh the page again to see the times change (bad)
I'm sorry, it looks like I tried to be too clever.
We should either do this _only_ in onloadTZSetup and not in fixDatetimeTZ,
or abandon this micro-optimization entirely - we have to enumerate all
elements with "datetime" class to add 'title' attribute anyway.
The first solution was chosen in next version of this patch.
> > + my (undef, undef, $datetime_class) =
> > + gitweb_get_feature('javascript-timezone');
> > + if ($datetime_class) {
> > + $strtime = qq!<span class="datetime">$strtime</span>!;
>
> Should this hard-code class "datetime", or use $datetime_class from
> the gitweb server configuration?
Sorry, my mistake. Fixed.
> > +/* JavaScript-base timezone manipulation */
>
> Might want to reword as "JavaScript-based"
Done.
> > + // refresh cookie, so it expires 7 days from last use of gitweb
> > + setCookie(tzCookieName, tzCookie, { expires: 7, path: '/' });
>
> Hmm, only 7 days?
Well, in new version where expire time is set in one place (was one to
set cookie after timezone selection, one to refresh cookie), and is
increased to 14 days (2 week). Please also remember that it is time
from last visit to gitweb, not last timezone change.
> > + * and instals appropriate onclick trigger (via event delegation).
>
> "installs"
Done.
Thanks again for reviewing it.
--
Jakub Narebski
Poland
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html