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

RESOLVED FIXED in 0.7

Status

Calendar
Sunbird Only
--
minor
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: Gábor Katona, Assigned: gekacheka)

Tracking

unspecified
Dependency tree / graph

Details

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

13 years ago
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

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

Comment 2

13 years ago
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.
(Reporter)

Updated

13 years ago
Severity: major → minor

Updated

13 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

13 years ago
OS: Windows XP → All
Hardware: PC → All
Summary: When hovering a ToDo item long lines of the description doesn't break into multiple lines → When hovering a ToDo item long lines of the description doesn't break into multiple lines [task/event mouseover preview wrap]
(Assignee)

Comment 3

13 years ago
Created attachment 176428 [details] [diff] [review]
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?)
(Assignee)

Updated

13 years ago
Attachment #176428 - Attachment description: Adds wrapping to mouseover preview text in header values and body description (notes). → Adds wrapping to mouseover preview text in header values and body description (0.2 branch patch).
Attachment #176428 - Attachment is obsolete: true
(Assignee)

Comment 4

13 years ago
Created attachment 184279 [details] [diff] [review]
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.
Attachment #184279 - Flags: first-review?(mvl)
(Assignee)

Updated

13 years ago
Attachment #184279 - Flags: first-review?(mvl)
(Assignee)

Comment 5

13 years ago
Created attachment 184468 [details] [diff] [review]
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.)
Attachment #184279 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Depends on: 295146
(Assignee)

Comment 6

13 years ago
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.)
Attachment #184468 - Flags: first-review?(mvl)

Updated

12 years ago
QA Contact: gurganbl → sunbird

Comment 7

12 years ago
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.
Reassigning all automatically assigned bugs from Mostafa to nobody@m.o

Bugspam filter: TorontoMostafaMove
Assignee: mostafah → nobody

Comment 9

11 years ago
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.
Assignee: nobody → gekacheka
(Assignee)

Comment 10

11 years ago
Created attachment 248840 [details] [diff] [review]
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.
Attachment #184468 - Attachment is obsolete: true
Attachment #248840 - Flags: first-review?(lilmatt)
Attachment #184468 - Flags: first-review?(mvl)
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.
Attachment #248840 - Flags: first-review?(lilmatt) → first-review+
(Assignee)

Comment 12

11 years ago
Created attachment 253914 [details]
image illustrating problems: English text not wrapped, cropped at edge of screen, indentation lost
(Assignee)

Comment 13

11 years ago
Created attachment 253915 [details]
image illustrating solution: English text wrapped, indentation preserved
(Assignee)

Comment 14

11 years ago
Created attachment 253918 [details]
image illustrating solution: Long text; max height is 1/2 screen height
(Assignee)

Comment 15

11 years ago
Created attachment 253919 [details]
ics file containing test cases used for illustrations
(Assignee)

Comment 16

11 years ago
Created attachment 253920 [details] [diff] [review]
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.
Attachment #253920 - Flags: second-review?(mvl)
(Assignee)

Updated

11 years ago
Attachment #248840 - Attachment is obsolete: true
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.
Attachment #253920 - Flags: second-review?(mvl) → second-review+
(Assignee)

Comment 18

11 years ago
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).
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.
Whiteboard: [checkin needed]

Updated

11 years ago
Whiteboard: [checkin needed] → [checkin needed after 0.5]
Fixed additional *'s and whitespaces.

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed after 0.5]
Target Milestone: --- → 0.7

Comment 21

10 years ago
This checkin regressed Bug 389341 and Bug 396298.

Comment 22

10 years ago
This checkin also regressed Bug 390313.
Depends on: 389341, 390313, 396298

Comment 23

10 years ago
gekacheka, are you going to address the regressions caused by your patch?
You need to log in before you can comment on or make changes to this bug.