Re: [PATCH 00/11] gitweb: Change timezone

On Tue, 12 Apr 2011, Kevin Cernekee wrote:

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.

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

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

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.

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.
 

Sorry, my mistake.  Fixed.

Done.
 

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.

Done.

Thanks again for reviewing it.