User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2007121605 Minefield/3.0b3pre Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2007121605 Minefield/3.0b3pre I make themes for people with visual disabilities. Three of my themes use 28-pixel scrollbars and 32-pixel icons for menu items. There is not enough room in the dropdown Autocomplete Richlist Results richlistitems for the wide scrollbar and the 32px icons. The star icons do not appear. They vanish behind the scrollbar. Because of the way the code is written, using the splitter to widen the urlbar does not provide more room for the star icon. It simply widens the autocomplete title (.ac-title). We themers needs a way to adjust the way the space is apportioned withing the richlistitem. Reproducible: Always Steps to Reproduce: 1. Install a theme that uses 32px menu icons and 28px scrollbars (e.g. -- SpehereGnome Jumbo). 2. Click on the urlbar autocomplete dropdown button. Actual Results: In the dropdown results, for bookmarked sites the star is hidden. Expected Results: The star should be visible. I have used a variety of margin settings, all of which produce unsatisfactory results. For example, using a large negative left margin with the ellipsis can pull the star into view, but only by overlapping the right end of the autocomplete title. The result is ugly. An example (using SphereGnome_Big) can be seen here: http://img520.imageshack.us/img520/105/acdropdownfr7.png The relevant code seems to be here: http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/autocomplete.xml#1252 et seq. (note that the comment on line 1256 is actually referring to bug #399664.) As more people buy high-res displays, more will be using themes that provide icons that are more visible. So some users may not simply be those with visual disabilities.

A version of SphereGnome Jumbo that is compatible with Minefield through the 2007-12-16 nightly and shows the problem: http://www.pshrink.com/firebird/spheregnome_jumbo-2.5.3-fx.jar

this is definitely valid. martijn logged a similar bug, possibly about font sizes. this hackery needs to be fixed: http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/autocomplete.xml#1264 1260 // XXX hack 1261 // see bug #399664 comment #39 for some details 1262 // add 16px for the site image, 8 for the margin right of the site icon, 1263 // 16px for the type image, and 24px for scroll bar 1264 var pixelsUsedForNonText = 16 + 8 + 16 + 24; at the very least, we can get the width of the scrollbar we should use that (instead of 24)

Doing it this way, removes the need of calculating width of scrollbars or images, etc. It seems to work fine, although the star image is slightly higher with this patch. Also, I use a margin-right: 4px value, which is a little odd.

One more thing while you are revisiting the code: Remember that some users will be using larger text size for their fonts. Please make sure that a change in fonts size (to size 15, for example) does not shove the star out of the dropdown menu. I am looking at the 2007-12-22 Minefield build, and a large enough font size (e.g. - size 24 ) shoves the star and half of the ellipsis right off the dropdown. Size 10 is just fine.

Oh, yes: in the theme I was using for comment #4, the site icon and star are both 32-pixel images (SphereGnome Jumbo).

Ok, this is a way to fix it, basically the same as "patch?", only the margin-right: 4px rule moved to autocomplete.css. With larger font-sizes, the margin-right value has to be bigger, but that can be adjusted with css.

This is a different patch, this seems better, there is no need for the mysterious margin-right: 4px, this is just adding 2px as the margin-right value, to let the star image at least be 2px from the scrollbar. I took the liberty in this patch to vertical center the favicon image and the star image, but that is not necessary. Whatever is preferred.

Comment on attachment 294745 [details] [diff] [review] patchv1b So it seems to me that this patch might be better, although the vertical centering is probably a matter of taste, but I can change that back.

Martijn, your patch looks really promising! Your grid approach will help me remove two of my hacks (see bug #405918). This seems like is a good solution until we have bug #405913 (richlisttree), but perhaps Neil has thoughts. In your screen shot, I noticed that the bookmark star gives the middle column less space, so the ellipsis don't line up. is that by design? As for the favicon and the star centering, we should check with faaborg / beltzner. But in screen shot patchv1a screen shot, they appear higher than in the original. To make sure I understand, you needed to use transparency on the ellipsis so that it would keep its width (and so the flexed column would not take up that space and become misaligned), right? We should measure the performance impact of this change, which we can do with the test case in https://bugzilla.mozilla.org/show_bug.cgi?id=406259 (I can help measure as well.) It might be better to set the value of the ellipsis in the autocomplete-richlistitem constructor once, instead of calling: aEllipsis.value = this.ellipsis; each time we call _setUpEllipsis() and then, in our css, default the color to transparent (so it starts off transparent) and then toggle the color in _setUpEllipsis()

one more question: with normal icons (and a normal sized scrollbar) does the star/tag icon in the result item line up with the star/go icon in the url bar?

(In reply to comment #11) > one more question: with normal icons (and a normal sized scrollbar) does the > star/tag icon in the result item line up with the star/go icon in the url bar? Ah, no, they don't. Ok, that means I would need to adjust the margin-right value to match that case (which is the most common). Thanks for the comments, I'll comment on comment 10 later. (I was merely asking review to see whether this is a good approach)

Martijn, this looks like a good approach to me (using grid / flex instead of the hacks I did). myk (or who ever now owns the applications tab in the pref UI) might be interested in your approach as well.

Comment on attachment 294745 [details] [diff] [review] patchv1b I think this is the right approach, much better than my existing approach. See comment #10 for some suggestions. clearing review request as there is at least one issue (star alignment) to be resolved.

Sorry for the delay. Ok, this fixes the "ellipsis not lining up" problem in comment 10. Also, I added some margin-left to the favicon (aside from vertical centering), so it doesn't nudge as much against the side. (In reply to comment #10) > To make sure I understand, you needed to use transparency on the ellipsis so > that it would keep its width (and so the flexed column would not take up that > space and become misaligned), right? This isn't necessary anymore with this patch. This uses .collapsed. It's a more correct way to do it, but it might make things slower. > It might be better to set the value of the ellipsis in the > autocomplete-richlistitem constructor once, instead of calling: Ok, I've done that, I also moved the code from the property into the field. Thanks for the pointer for the performance testcase at bug 406259. I modified it slightly (doing more runs) for my own test. With the patch, I get 28.9s. Without the patch, I get 23.5s. So I guess I need to improve that.

martijn, is that the correct screen shot? it still appears to have the "ellipsis not lining up problem" of comment #10

Yes, that's a correct screen shot. The url and title ellipses are lining up vertically, no? Or am I misunderstanding the problem?

> The url and title ellipses are lining up vertically, no? Or am I misunderstanding the problem? You are right, I didn't notice that before (and you appears to have fixed it.) Here's the problem I was referring to: In your latest screen shot, compare the ellipsis between row 0 (which has a star) and row 1. Currently, we keep those ellipsis lined up, but with your patch, starred/tagged items are not lined up with unstarred/untagged items. Note, beltzner/faaborg might prefer what you've done (including centering the favicon), so before fixing this you might want to ping them for ux-approval.

Ah, I see, heh, I did that intentional, because I preferred it. But I can change it back if the current behavior is preferred.

Martjin, can you get some ux-approval from beltzner/mconnor/faaborg in parallel with the code review

Comment on attachment 295119 [details] [diff] [review] patchv2a Alex, can you ui review this, see last screenshot for the changes compared to current ui.

This fixes most of the performance regression with the test of bug 406259, I think. Especially removing _setUpEllipsis and using onoverflow/onunderflow events improved the result (by appr. 3s).

Alos, removing the last <column/> element seems to improve the result with 0.5s without causing negative effects.

Comment on attachment 295156 [details] [diff] [review] patchv2b Let me know, which of the optimizations you like. Another optimization could be to move a lot of the grid elements in the autocomplete-rich-result-popup binding, but that causes all kinds of selection problems, which I wasn't able to fix.

1) martijn, can you explain this part of the patch: +.bookmark { + margin-left: 3px; } 2) also, you might have some slight bit rot after a recent landing by reed of dao's patch for bug #409636 3) I don't think we need the ellipsis property anymore, I think we can just use the _ellipsis field, right?

martijn, short of a ux ruling from beltzner/mconnor/faaborg, I think we should keep things looking the same, and make your improvements spin off bugs. so we should: 1) un-vertically center the favicon 2) un-vertically center the star/tag 3) line up the ellipsis on the right, so that results with or without a star/tag line up. #3 is the more interesting one, which might require work on your end (as you can't just collapsed / uncollape the ellipsis label.)

Comment on attachment 295156 [details] [diff] [review] patchv2b clearing review request, details coming in next comment.

over email, faaborg writes: "When we have horizontal lines between rows, I think it makes sense to vertically center the favicon. however, I would like us to consider dropping the lines, and using white space to separate results to make the interface visually cleaner, in which case vertically centered favicons makes it harder to differentiate rows because you can't leverage the top edge of the favicon to detect a border. Let's keep the design the same for now, and try to move all design discussion to the mockup tracking bug: https://bugzilla.mozilla.org/show_bug.cgi?id=393508" martijn, based on that ux-review, can you take care of items #1 and #2 in comment #27? As for item #3, I suggest keeping it as is as well, but faaborg hasn't commented on it yet. (in case I haven't said so earlier, I'm very jazzed about this fix and your work so far, as it simplifies the code and removes some hacks of mine.)

Why is 'grid' used? It is very expense performance wise, and will impact Ts,Tp, and whatelse. Otherwise, love the idea of vertical center, and the 'star' only eating space when visible

Alfred, yes, that's what I'm worried about too. I used grid because that way, the overflow stuff is working automatically and then the hacky _adjustWidth method can be removed (which might be slow too). There is/should probably be a better way to get the same result without using the grid say.

(In reply to comment #26) > 1) > > martijn, can you explain this part of the patch: > > +.bookmark { > + margin-left: 3px; > } Sorry, too much fiddling with the patch, that should have been .ac-result-type-bookmark, as it is in patchv2a.

I am not sure how the grid thing is helping you here. The only thing the grid does is to ensure that the width's of the left and right side icons remain fixed...

Alfred, I don't understand, with your version and the _adjustWidth method removed, content is overflowing outside the popup, no?

> Why is 'grid' used? It is very expense performance wise, and will impact Ts,Tp, > and whatelse. I'm not 100% convinced that this grid will impact Ts. Tp (page load) should not be impacted at all, so I assume you meant Txul. These elements are in a panel that is hidden by default. From browser.xul: <panel type="autocomplete-richlistbox" chromedir="&locale.dir;" id="PopupAutoCompleteRichResult" noautofocus="true" hidden="true"/> But if there is a better way to accomplish the goal without using a grid, that would be great.

This allows removal of all the 'adjustWidth' and 'ellipse' handling, and let the 'label' element do the cropping by itself. This construct works for cropping url's, but not (yet) for the title, for some unclear reason. A refinement would be do set cropping at the longest part before/after the match part, so that when the match is at the end the cropping is at the beginning.

using labels won't work, due to RTL issues. see bug #402118, bug #399664 (search for rtl, that's a monster bug) on a related note, see: bug #404427 ("for description elements, crop only works if you use the value attribute") and bug #312156 ("implement text-overflow: ellipsis from CSS3 text")

This patch works, and using 'single-line' <description> makes sure that the parts are RTL ok. Bug 402118 has the most clear description why description is needed. bug #404427: is not relevant as the description fields are now single-line, so crop does work here bug #312156: would indeed be a much better solution for all this. Meanwhile this version has less code, has no magic numbers, is much simpler, and has less performance impact (no settimeouts and such to get and adjust widths...). Possibly for RTL users, we could move the crop/flex to the first <description>. Note that currently any RTL titles are also cropped at the right-hand side. So that needs to addressed separately, but with my solution that would be easy to add (just move the "crop" and "flex" from the third <description> to the first when the title is RTL)

Alfred, thanks for giving your version, it turns out that adding some flex attributes solved the overflow issue. This patch keeps the look as it is currently (afaict), as you suggested. I now had to toggle style visibility for the onoverflow/onunderflow event, because with .collapsed I got a weird repeating effect. This patch is still a bit rough, on some spots.

Comment on attachment 295439 [details] [diff] [review] patchv3a Sorry, never mind, I now see that Alfred's patch is better. Thanks, Alfred.

Alfred / Tim: + <xul:hbox anonid="title" pack="start" flex="1" class="ac-normal-text ac-comment"> + <xul:description style="margin:0"/> + <xul:description class="ac-emphasize-text" style="margin:0"/> + <xul:description flex="1" crop="end" style="margin:0;min-width:1px"/> </xul:hbox> Are we sure this still works properly with RTL? When the chrome direction is rtl, does this flip the order of the three elements within the hbox?

Yes, that is one of the issues to be addressed: 1. Flip order of the three desc's when the title is RTL (note: cropping will then automatically be at the right place, for RTL thus on the left). 2. When the 'match' part is not visible, we could extend this mechanism to set the widths of the first and second part more explicitly to force cropping at both ends (but that can be addressed in another bug as the current impl. also doesn't do this). Big question is now how to detect that the 'title' is RTL?

>Alex, can you ui review this, see last screenshot for the changes compared to >current ui. Only beltzner and mconnor can do ui-r, but I think we need a few iterations on the mockup tracking bug before we are sure what formating we want to land next.

See bug 407861: Use of span prevents breaking of ligatures, but only if color, backgroundcolor and/or underline is used for marking. Anything else will break ligatures. Possibly the mockup needs to determine what is actually preferred: prevent breaking of ligatures, correct cropping and/or moving the 'match' part in the visible range.

Alfred, I just tried your latest patch, but when searching in the url bar, content can overflow without getting the ellipsis: http://martijn.martijn.googlepages.com/autocomplete_alfred.gif Or maybe I applied your patch wrongly (I did get some conflicts)? I don't understand the rtl talk, that seems to already fail, no?

(In reply to comment #29) > "When we have horizontal lines between rows, I think it makes sense to > vertically center the favicon. > however, I would like us to consider dropping the lines, and using white space > to separate results to make the interface visually cleaner, in which case > vertically centered favicons makes it harder to differentiate rows because you > can't leverage the top edge of the favicon to detect a border. I have actually tried using white space between the title and URL line pairs, and observed the following: Most favicons have a transparent background with their visibile content having a height less than the total icon height, and centered vertically within the icon. If you vertically align such icons with respect to the tops of the title lines, the tops of the visible icon contents vary with respect to the visible contents of the title lines, and give the impression of sloppy programming which is yielding that "perceived" variable vertical positioning of the favicons. If instead the favicons are centered vertically with respect to both the title and URL lines, the visible icon contents are almost always also vertically centered with respect to each title and URL pair, and so the human visual system keys in on that constistency and soon does come to "perceive" the favicons as markers for related pairs of lines. Thus, if you have only white space between each pair of lines, you even moreso will be better off with the favicons aligned vertically.

Martijn, the patch is not fully operational yet. As you can see, when the match part is outside the visible range the lines are still clipped instead of cropped. I do have an idea to fix this, but that requires again a 'adjustWidth' action, so then your patch is as good.

for reference, concerning this issue: "In a result, the tag/star "eats into" the text, so the ellipsis for a title or url don't line up (between two rows, if one is starred/tagged and one is not). On the flip side, this gives more room for the text when there is no star/tag image." faaborg wrote: "I don't think this is too big of an issue since we are very used to seeing text that is not right aligned."

> Yes, that is one of the issues to be addressed: > 1. Flip order of the three desc's when the title is RTL (note: cropping will > then automatically be at the right place, for RTL thus on the left). I don't think you want to do this. I think you want to rely on the handling of rtl text (note, not rtl chrome direction) by using description. see bug #402118 for details. I suggest we do what martijn is proposing in patch v3a.

Comment on attachment 295442 [details] Even simpler version for this 'battle of patches' obsoleting because of the rtl issues.

Comment on attachment 295439 [details] [diff] [review] patchv3a martijn, can you create a screen shot using this patch?

I agree with the above comments, fixing RTL outside description is just too hard (I have an headache from thinking about it the whole weekend). Anyway the real point I wanted to make was to have less grids/boxes, and the last patch of Martijn addresses this. And with the nice trick of 'onoverflow' Martijn also got rid of the '_adjustWidth'. So, I am OK with the patch from Martijn.

This is updated to current trunk, and now I also tried to fix pinstripe (this is untested, though). Screenshot that compares current look and with this patch: http://martijn.martijn.googlepages.com/autocomplete3b_comparison.gif

> So, I am OK with the patch from Martijn. Alfred: thanks for commenting about the state of martijn's fix. I agree, RTL is/was challenging here, and as you know, we still have to figure out the bold/ligature issue (already logged in another bug) > This is updated to current trunk Martijn: excellent! The screen shot looks great to me. I'll test on pinstripe and review.

I wonder if we'll get lucky and Martijn/Alfred's fix will also fix bug #406373?

> I wonder if we'll get lucky and Martijn/Alfred's fix will also fix bug #406373? good news, we did get lucky.

martijn, playing your patch on Mac, I've noticed a few things: 1) occasionally, a result row will initially be shown as "too tall", but then it will reflow to the right height. have you ever seen that? I can't capture it in a screen shot. 2) frequently, the ellipsis will not show up. See the attached screen shot. 3) unlike the star icon, the tag icon is higher than the favicon on the left. See the attached screen shot. 4) because we are relying on reflow, the patch makes the url bar "jumpier". Have you noticed that?

while working on the original version, I remember trying to use the onoverflow / onunderflow and having this problem with ellipsis, which is how I ended up with _setUpEllipsis()

Comment on attachment 295766 [details] [diff] [review] patchv3b based on my comments, we can't check this in yet.

Regarding comment 59, I did see issue 3 (I didn't know about the existence of labels!), it should now be fixed in this patch. I haven't seen issue 1 or 2, and I don't think I have seen issue 4 either. Mac only? This patch has put back _setUpEllipsis() and accompanying code, it's not really needed to fix this bug (it would have been a nice simplification, though). I guess we should file a follow-up bug about onoverflow/onunderflow not working like on windows in this case.

Please make sure to test this against a theme with 32-pixel site icons. I have just uploaded a new build of SphereGnome Jumbo 2.5.3 compatible with the 2008-01-07 nightly Firefox build, at: http://www.pshrink.com/firebird/spheregnome_jumbo-2.5.3-fx.jar

With patchv3c on WinXP and the default icons, the margin between any stars and the scrollbar in the drop menu is larger than the margin between a star and the drop marker in the urlbar, so they do not line up horizontally.

Well, with the windowsXP theme, I already get 1px offset with normal trunk builds. This patch fixes it for the windowsXP theme, but now, with the windows2000 theme, I get a 2px offset between the star on the url bar and in the popup. This is happening because the dropmarker doesn't have a native theme anymore. Afaik, there isn't an easy way to fix this and the windowsXP theme takes precedence. Hopefully, this works correctly for the Vista theme too. (a bit off-topic: a windowsXP scrollbar is 17px wide, while a windows2000 scrollbar is 16px wide, which is causing the difference) Ed, I already tested it with large icons with one of the earlier patches, and it worked fine, afaict.

Ok, Windows Vista also returns 17px as width for the scrollar with the Vista theme, so it should be fine. No idea what Mac is doing, though.

(In reply to comment #65) > This patch fixes it for the windowsXP theme, Yes, including when I work in Seth's patch for Bug 406255 Note that I also haven't suffered the problems described on Mac in Comment 59 when exercizing any of the patches for this bug on WinXP, so those indeed may be Mac-specific. But I do see a strange hysteresis for the ellispis. When the urlbar width is reduced via the slide grip bewtween it and the searchbar, the ellispis appropriately does not get swapped into a title or URL until the star icon would overlap it. But then if you widen the urlbar, the ellipsis keeps being swapped into that title or URL util the slide grip has been moved back to the right for at least the width of the star icon (after which you can start another cycle of correct behavior and then the hysteresis). If this would be hairy to fix, I personally wouldn't consider it a blocker (it would simply need a spin-off bug).

I think I know what you mean. I think that would not be a problem if onoverflow/onunderflow events could be used. Ideally, someone with a Mac should file a bug about onoverflow/onunderflow not working correctly in this case and with a nice testcase that shows the bug.

currently testing this on mac, and so far, so good.

> 1) occasionally, a result row will initially be shown as "too tall", but then > it will reflow to the right height. have you ever seen that? I can't capture > it in a screen shot. Unforunately, I'm still seeing that issue, at least on Mac. > 2) frequently, the ellipsis will not show up. See the attached screen shot. with the new patch, I'm not seeing that anymore. > 3) unlike the star icon, the tag icon is higher than the favicon on the left. That problem also appears fixed.

(In reply to comment #70) > > 1) occasionally, a result row will initially be shown as "too tall", but then > > it will reflow to the right height. have you ever seen that? I can't capture > > it in a screen shot. > > Unforunately, I'm still seeing that issue, at least on Mac. Does it help if you remove this line in autocomplete.css? .autocomplete-richlistbox > scrollbox { overflow-x: hidden !important; } and/or overflow:-moz-hidden-unscrollable; from autocomplete-richlistitem? In any case, I can't reproduce this here on windows, so I can't really help.

Sorry for the delay, Martijn. > In any case, I can't reproduce this here on windows, so I can't really help. Unfortunately, I no longer have a Mac to test with. Perhaps Gavin or Dietrich can help out?

adding mconnor / dao, perhaps one of them can help find a new owner?

WFM. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008030904 Minefield/3.0b5pre - Build ID: 2008030904 Thank you for the fix on Windows.

Martijn: did bug 421412 "fix" this to your satisfaction, or should we look further into the grid approach? If so, it's probably best done in a new bug at this point...

Fwiw, I'm now getting scrollbars with the url bar autocomplete.

In this case, I'm not getting overflow ellipses: - Open url bar by clicking on the dropmarker - Make the url bar very small, click again on the dropmarker I think caused by the patch for bug 421412.