Bug 171276 - [Forms] layout issue with long scope set name
Summary: [Forms] layout issue with long scope set name
Status: RESOLVED WORKSFORME
Alias: None
Product: Platform
Classification: Eclipse Project
Component: User Assistance (show other bugs)
Version: 3.2.1   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Adam Archer CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed
Depends on:
Blocks:
 
Reported: 2007-01-22 13:54 EST by Jamie Liu CLA
Modified: 2011-04-20 15:51 EDT (History)
3 users (show)

See Also:
mike.pawlowski: review+
cgold: review+


Attachments
screenshot of described bug (342.80 KB, image/bmp)
2007-01-22 14:51 EST, Jamie Liu CLA
no flags Details
patch (5.81 KB, patch)
2007-07-09 15:48 EDT, Adam Archer CLA
no flags Details | Diff
patch (5.68 KB, patch)
2007-07-09 17:09 EDT, Adam Archer CLA
no flags Details | Diff
patch (5.92 KB, patch)
2007-07-11 14:05 EDT, Adam Archer CLA
mike.pawlowski: review+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jamie Liu CLA 2007-01-22 13:54:08 EST
Build ID: M20060921-0945

Steps To Reproduce:
1. Press F1 for the dynamic help view to appear
2. Click "Search" at the bottom
3. Click link "Default" near "Search scope"
4. Press "New" button
5. Enter a long string for the scope set name; Click OK
6. Select the new scope set just entered; Click OK
7. Notice the string "Search Scope" wraps such that it is unreadable in some cases


More information:
Comment 1 Jamie Liu CLA 2007-01-22 14:51:53 EST
Created attachment 57288 [details]
screenshot of described bug
Comment 2 Chris Goldthorpe CLA 2007-06-13 18:48:26 EDT
This bug is still present in 3.3RC4.
Comment 3 Chris Goldthorpe CLA 2007-07-03 18:21:44 EDT
Assigning to Adam.
Comment 4 Adam Archer CLA 2007-07-09 15:48:07 EDT
Created attachment 73381 [details]
patch

This turned out to be a forms specific problem, rather than help. The search part is using the section's text client in a very unusual way. In most cases, this is used for an icon or something similarly small and inconsequential. Thus, the layout algorithm gives the text client all the space it needs and gives whatever is leftover to the title.

This patch changes the ExpandableComposite's layout to check if the text client is wrappable. If it is, it will attempt to divide the space evenly between the title and the client. Otherwise, it functions as it did before.

In order to make this work properly, some heavy changes were required to the FormUtil.computeWrapSize() method (which was made public for visibility in ExpandableComposite). It was not handling several different cases correctly. For one, it was unable to correctly determine the size when one word required more space than the width hint. To allow this case it was necessary to sometimes return a value larger than the width hint. This resulted in an incorrect height being returned since the height was calculated under the assumption that the width hint was the absolute maximum. Correcting this requires a recursive call to the method in the case that the width calculated is greater than the hint. This recursion will only be required once per call to the method and only in specific cases. The performance hit should not be too great and is required to ensure correct functionality of the method.
Comment 5 Adam Archer CLA 2007-07-09 15:50:21 EDT
Chris, can you review this patch for me?

Also, Mike has been asked to review it from the perspective of whether it breaks anything in PDE.
Comment 6 Adam Archer CLA 2007-07-09 17:09:49 EDT
Created attachment 73393 [details]
patch

I left some testing code in the last patch.
Comment 7 Chris Goldthorpe CLA 2007-07-11 11:53:55 EDT
I did some testing and saw a layout regression. I'll post more details in a follow up comment.
Comment 8 Adam Archer CLA 2007-07-11 14:05:24 EDT
Created attachment 73575 [details]
patch

The regression Chris is talking about is actually an existing inconsistency between ExpandableComposite.computeSize() and ExpandableComposite.layout() that this fix exposed. This new patch corrects the inconsistency as well.

Basically computeSize was not adding the IGAP pixels required between the toggle control and the title label. Thus, it was reporting that the required width was 4 pixels smaller than the actual required width.
Comment 9 Chris Goldthorpe CLA 2007-07-11 15:00:40 EDT
+1 for the latest patch. Will commit once Mike has given his approval also.
Comment 10 Mike Pawlowski CLA 2007-07-11 15:40:28 EDT
Comment on attachment 73575 [details]
patch

Tested well within PDE.  Tested using the Plug-in Manifest editor and Simple Cheat Sheet Editor form pages and sections.
Comment 11 Chris Goldthorpe CLA 2007-07-11 16:19:56 EDT
Patch committed to HEAD.
Comment 12 Jamie Liu CLA 2008-07-18 09:42:04 EDT
Hi, we are still seeing this issue on RedHat5. (Sled10, xp, vista all work fine)  This is being flagged as "Section 508 blocking" by our accessibility team, so could this be looked into further?
Comment 13 Jamie Liu CLA 2008-07-21 17:14:55 EDT
Correction, our accessibility team did not find it to be 508 blocking since the issue only occurs on linux and mac. Setting priority to Normal.
Comment 14 Chris Goldthorpe CLA 2008-07-24 17:29:20 EDT
We need to change the target milestone which is currently set to 3.4M1.
Comment 15 Chris Goldthorpe CLA 2011-04-20 15:51:23 EDT
This is working fine on both Windows and Linux using I20080617-2000. Closing as WORKSFORME.