262811 - When hovering a ToDo item long lines of the description doesn't break into multiple lines [task/event mouseover preview wrap]

Status: RESOLVED FIXED

(Calendar :: Sunbird Only, defect)

Description

[Gábor Katona]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; hu-HU; rv:1.7.3) Gecko/20040910
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; hu-HU; rv:1.7.3) Gecko/20040910

When I set a ToDo (Task) item, and provide it with a longer description (which
is not in english), and then hover the mouse pointer over the Task item in the
ToDo list on the left hand-side, then the flying frame doesn't display the whole
description unless it has line breaks in it. This way one cannot read easily the
detaild description of the task without opening the task setting window by
double clicking.

Reproducible: Always
Steps to Reproduce:
1. set a todo item with a long description without line breaks, e.g. a few sentence
2. hover the mouse over the item in the list
Actual Results:  
the details of the task item is listed, but with line truncated at the border of
the flying frame

Expected Results:  
long lines should be broken into multiple lines

Calendar version: 2004091012-cal

Comment 1

[k b]

confirm with:
20040924-cal on 
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a5) Gecko/20041001

Comment 2

[Joey Minta]

I can confirm this in Calendar 2005011112 (firefox 1.0, winXP) as well as
Sunbird .02rc2, but suggest that the bug severity should be set to 'minor.'

Reporter,
   Please change the severity, so I can confirm this bug.

Comment 3

[gekacheka]

Attachment: Adds wrapping to mouseover preview text in header values and body description (0.2 branch patch).

(patch -l -p 2 -i file.patch)

Adds wrapping to mouseover preview text in header values and body
description (notes).


CSS changes:

tooltipBody uses style white-space: -moz-pre-wrap to obey newlines
(paragraphs).

vbox.tooltipBox has max-width: 40em (normal tooltip max-width) to force wrap.
  (Putting max-width on description.tooltipBody does not work --- the 
   enclosing vbox width is then computed as the tooltipBody width before
   wrapping, so the vbox becomes much too wide.)

Header grid column.tooltipValueColumn has max-width 35em to force header
  values to wrap.  (Grid does not obey the enclosing vbox max-width.
  Putting this max-width on description.headerDescriptions does not
  work --- the enclosing column width is computed using the header value
  widths before wrapping, so the column becomes much too wide.)

label.tooltipHeaderLabel, tooltipHeaderDescription, and tooltipBody all have
  margin: 0pt to remove excess space between lines.  tooltipHeaderLabel
  has a right margin to provide space between label and value.

separator.TooltipBodySeparator is a new separator to add a thin 1ex spece
  between the headers and the body.


MouseoverPreviews changes:

Tooltip height is now limited by 1/2 screen height rather than four lines.
  (If larger than 1/2 screen height, some tool tips extend off screen.)

A thin XUL separator rather than empty text line is now added between
  headers grid and body description.

Added class to tooltipBox and tooltipValueColumn for CSS max-width.


Tooltip setup changes:
(calendarWindow.js, unifinder.js, unifinderToDo.js)

Tooltip does not automatically size to wrapped height, so added
    tooltip.sizeTo(vbox.boxObject.width, vbox.boxObject.height);
The result of sizeTo is sometimes a tooltip with no bottom border, but this 
  only happens when a description (notes) body text is added, not when the
  header grid appears alone.  To workaround this bug, added 1+2+2+1 to the
  height (for top and bottom border and padding).
  (Future work: How can it read the tooltip's current values for border and
   padding, so this workaround will work even if a theme changes them?)
Tooltip may not always shrink to new size, so added
    tooltip.sizeTo(0,0)
  before adding new contents and resizing it.


Testing:

Tested using 0.2 branch on Moz1.8b1 and TB1.0.
Can test on trunk after nightly build becomes available.

Long title:  An event or task with a long title produces a mouseover
  preview where
  * the title is wrapped,
  * the tooltip is 40em wide, with no extra space.

Long body paragraphs: An event or task with long unwrapped paragraphs
  in the notes field produces a mouseover preview where
  * the paragraphs are wrapped.
  * the tooltip is 40em wide, with no extra space.

Long body: An event or task with many lines (say 100) in the body produces a
  mouseover preview where
  * the tooltip is about 1/2 the screen height.
  * the bottom border of the tooltip still appears.
  * the body is cut off (future work: is there some way to detect that
    the contents was limited by the max height, and indicate it?)

Comment 4

[gekacheka]

Attachment: Wrap mouseover preview header values and description text (trunk patch)

(patch -l -p 2 -i file.patch)

Updated patch for trunk.  See comment #3 for description of changes.

Comment 5

[gekacheka]

Attachment: Wrap mouseover preview header values and description text (trunk patch)

(patch -l -p 2 -i file.patch)

Adds code to set size of tooltip to size of holder box with wrapped text.
(Added code used to be in patch for bug 295146, but is more relevant to this
bug.
 Now this patch depends on that bug's patch.)

Comment 6

[gekacheka]

Comment on attachment 184468 [details] [diff] [review]
Wrap mouseover preview header values and description text (trunk patch)

(added part split from bug 295146 patch.  thanks.)

Comment 7

[Ilja Sekler]

The actual implementation of the mouseover preview silently hides all lines of an event/task description after the 4th line from the user. There is not a slightest pointer for him about the description extending below the bottom of the tooltip unless he tries to edit the event/task. It's a major usability issue IMHO that really should be fixed (soon), removing the limitation to 4 description lines.

Comment 8

[Matthew (lilmatt) Willis]

Reassigning all automatically assigned bugs from Mostafa to nobody@m.o

Bugspam filter: TorontoMostafaMove

Comment 9

[Simon Paquet [:sipaq]]

gekacheka,
is this patch still valid or has it bitrotted? If the latter, could you please update it and move it to another reviewer (I'd suggest Daniel Boelzle or lilmatt). 

Thanks a lot.

Comment 10

[gekacheka]

Attachment: Wrap mouseover preview header values and description text (updated)

(patch -l -p 2 -i file.patch)

Updated patch.  See comment 3 for description.  
Adds CSS to lightning.css as well.

Comment 11

[Matthew (lilmatt) Willis]

Comment on attachment 248840 [details] [diff] [review]
Wrap mouseover preview header values and description text (updated)

>--- mozilla/calendar/resources/content/mouseoverPreviews.js	2006-08-23 12:53:18.000000000 -0400
> /*
> ** add newContentBox,
>+** then resize the tooltip to the size of the new preview content box
>+** (plus some constants for borders and padding to avoid bugs).
> */
Please fix this comment's line wrapping, and change the style to:
/**
 * foo
 */


> function setToolTipContent(toolTip, holderBox)
> {
>   while (toolTip.hasChildNodes())
>     toolTip.removeChild( toolTip.firstChild );
This isn't your fault, but the alleration in the code makes it tough to read. Please add { } to the while statement.

>+  toolTip.sizeTo(0,0); // workaround bug (sometimes tooltip doesn't shrink)
Is there a bug in layout filed on this?  If so, add the bug number here. If not, please try and make a testcase and file one.
Either way, please add a space after the comma.

>   toolTip.appendChild( holderBox );
>+  // add top and bottom border and padding to workaround bug where bottom
>+  // tooltip border disappears if description present below header grid.
>+  const topBottomBorderAndPadding = 1 + 2 + 2 + 1;
>+  toolTip.sizeTo(holderBox.boxObject.width,
>+                 holderBox.boxObject.height + topBottomBorderAndPadding);
Is this a layout bug that we're working around, or what?

> }
> 
> /**
>@@ -125,6 +133,10 @@
>   if( toDoItem )
>   {
>     const vbox = document.createElement( "vbox" );
>+    vbox.setAttribute("class","tooltipBox");
Add a space after the comma.

>+    // tooltip appears above or below pointer, so may have as little as
>+    // one half the screen height available (avoid top going off screen).
>+    vbox.maxHeight = Math.floor(screen.height/2);
Add spaces on either side of /.

>@@ -195,11 +207,10 @@
>     var description = toDoItem.getProperty("DESCRIPTION");
>     if (description)
>     {
>-      // display up to 4 description lines like body of message below headers
>+      // display wrapped description lines like body of message below headers
>       if (hasHeader)
>-        boxAppendText(vbox, ""); 
>-
>-      boxAppendLines(vbox, description, 4);
>+        boxAppendBodySeparator(vbox);   
Add { } to the if (hasHeader) statement for clarity. This line also has trailing spaces ^^^

>+      boxAppendBody(vbox, description);
>     }
>       
Remove this psuedo-blank line ^^^

>     return ( vbox );


>@@ -220,6 +231,10 @@
> function getPreviewForEvent( event, instStartDate, instEndDate )
> {
>   const vbox = document.createElement( "vbox" );
>+  vbox.setAttribute("class","tooltipBox");
Add a space after the comma.

>+  // tooltip appears above or below pointer, so may have as little as
>+  // one half the screen height available (avoid top going off screen).
>+  vbox.maxHeight = Math.floor(screen.height/2);
Add spaces on either side of /.

>@@ -310,8 +325,16 @@
>   }
> }
> 
>-/** PRIVATE: Append 1 line of text to box inside an xul description node containing a single text node **/
>-function boxAppendText(box, textString)
>+/** PRIVATE: Append a separator, a thin space between header and body **/
Please make this a javadoc style comment.

>+function boxAppendBodySeparator(vbox) {
This file's prevailing style appears to put the { on the next line.

>+  const separator = document.createElement("separator");
>+  separator.setAttribute("class","tooltipBodySeparator");
Add a space after the comma.

>+  vbox.appendChild(separator);
>+}
>+
>+/** PRIVATE: Append multiple lines of text to box, each line inside an xul description node containing a single text node.  Long lines are pre-wrapped for tooltip. **/
Please make this a javadoc style comment, and line wrap it.

>@@ -319,30 +342,6 @@
>   xulDescription.appendChild(textNode);
>   box.appendChild(xulDescription);
> }
>-/** PRIVATE: Append multiple lines of text to box, each line inside an xul description node containing a single text node **/
>-function boxAppendLines(vbox, textString, maxLineCount)
>-{
>-  if (!maxLineCount)
>-    maxLineCount = 4;
>-
>-  // trim trailing whitespace
>-  var end = textString.length;
>-  for (; end > 0; end--)
>-    if (" \t\r\n".indexOf(textString.charAt(end - 1)) == -1)
>-      break;
>-  textString = textString.substring(0, end);
>-
>-  var lines = textString.split("\n");
>-  var lineCount = lines.length;
>-  if( lineCount > maxLineCount ) {
>-    lineCount = maxLineCount;
>-    lines[ maxLineCount ] = "..." ;
>-  }
>-
>-  for (var i = 0; i < lineCount; i++) {
>-    boxAppendText(vbox, lines[i]);
>-  }
>-}
> /** PRIVATE: Use dateFormatter to format date and time.
>     Append date to box inside an xul description node containing a single text node. **/
Please make this a javadoc style comment, and line wrap it.

>@@ -374,12 +373,17 @@
> function boxInitializeHeaderGrid(box)
> {
>   var grid = document.createElement("grid");
>+  grid.setAttribute("class","tooltipHeaderGrid");
Add a space after the comma.

>   var rows;
>   {
>     var columns = document.createElement("columns");
>     {
>-      columns.appendChild(document.createElement("column"));
>-      columns.appendChild(document.createElement("column"));
>+      var labelColumn = document.createElement("column");
>+      labelColumn.setAttribute("class","tooltipLabelColumn");
Add a space after the comma.

>+      columns.appendChild(labelColumn);
>+      var valueColumn = document.createElement("column");
>+      valueColumn.setAttribute("class","tooltipValueColumn");
Add a space after the comma.


>--- mozilla/calendar/sunbird/themes/pinstripe/sunbird/calendar.css	2006-12-11 11:06:13.000000000 -0500
> /*--------------------------------------------------------------------
>  * tooltips
>  *-------------------------------------------------------------------*/
Throughout this file, I'm not cool with the whole "align the colon" thing.  In lines you add or change, please just use "selector: 10em;" style notation. It causes less concern when someone adds a long selector and worries over whether to realign all the other lines or not.

All comments in this file apply to the other 3 css files as well.

>+vbox.tooltipBox
>+{
>+  max-width   : 40em;
>+}
>+
>+column.tooltipValueColumn
>+{
>+  max-width   : 35em; /* tooltipBox max-width minus space for label */
>+}
>+
> label.tooltipHeaderLabel
> {
>   font-weight : bold;
>   text-align  : right;
>+  margin      : 0em 1em 0em 0em; /* 1em space before value */
> }
> 
> description.tooltipHeaderDescription
> {
>   font-weight : normal;
>   text-align  : left;
>-  white-space : normal;
>+  margin      : 0pt;
pt or px?

>+}
>+
>+separator.tooltipBodySeparator
>+{
>+  height : 1ex; /* 1ex space above body text, below last header. */
> }
> 
> description.tooltipBody
> {
>   font-weight : normal;
>-  white-space : normal;
>+  white-space : -moz-pre-wrap;
>+  margin      : 0pt;
> }


r=lilmatt with those style nits cleaned up

A before and after screenshot would also be nice.

Comment 12

[gekacheka]

Attachment: image illustrating problems: English text not wrapped, cropped at edge of screen, indentation lost

Comment 13

[gekacheka]

Attachment: image illustrating solution: English text wrapped, indentation preserved

Comment 14

[gekacheka]

Attachment: image illustrating solution: Long text; max height is 1/2 screen height

Comment 15

[gekacheka]

Attachment: ics file containing test cases used for illustrations

Comment 16

[gekacheka]

Attachment: v5 Wrap mouseover preview header values and description text (nits addressed)

(patch -l -p 1 -i file.patch)

Nits addressed.  See comment #3 for overview.

Comment 17

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 253920 [details] [diff] [review]
v5 Wrap mouseover preview header values and description text (nits addressed)

just one nit: Javadoc style comments needs a leading * for every line, like so:
/**
 * comment
 */
r2=mvl with that fixed.

Comment 18

[gekacheka]

Javadoc does not require leading '*'.  

A leading '*' is sometimes useful in javadoc if part of the comments use <pre> preformatted text with indentation (tells javadoc where the left margin ends and the indentation starts), but most comments do not.

For the non-preformatted/indented comments in this patch, adding a leading '*' is not needed.  Using it increases typing, makes comments harder to edit, increases download size (less compressible than pure whitespace indent), and can make comments harder to read (is it part of the comment or not).

Comment 19

[Michiel van Leeuwen (email: mvl+moz@)]

I find the leading * to be easier to read: you draw a box, with the leading * indicating that the line is part of a comment. No need to look at other lines.

Anyway, the leading * is part of calendar code style. Please just fix it for code consistency.

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Fixed additional *'s and whitespaces.

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED

Comment 21

[Stefan Sitter [:ssitter]]

This checkin regressed Bug 389341 and Bug 396298.

Comment 22

[Stefan Sitter [:ssitter]]

This checkin also regressed Bug 390313.

Comment 23

[Stefan Sitter [:ssitter]]

gekacheka, are you going to address the regressions caused by your patch?