421412 - Get rid of _adjustWidth hack

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect, P2)

Description

[Ed Lee :Mardak]

We can use flex to push the icon to the right, but we need to make sure the ellipsis show up correctly..

Comment 1

[Seth Spitzer]

you might want to take a look at bug #408621

Comment 2

[Seth Spitzer]

see also bug #405913 and bug #405918

Comment 3

[Ed Lee :Mardak]

Attachment: v1

A gavin-friendly patch ;)

Hide and show the ellipsis instead of getting the value each time. Clean up some code that gets reused. Calculate overflow by summing up the children not including the ellipsis. beltzner said the ndash is okay. But we won't actually see it until after bug 418257 lands.. but I suppose I'll have to go unbitrot that now. :p

Comment 4

[Ed Lee :Mardak]

This will fix bug 411963.

FYI, I did this patch at the top of my stack.

Comment 5

[Ed Lee :Mardak]

Comment on attachment 307837 [details] [diff] [review]
v1

>+          // Start with the parent's width and subtract off ellipsis siblings
Pretend that said..
// Start with the parent's width and subtract off its children

Comment 6

[Ed Lee :Mardak]

This fixes P2 blocking bug 396548.

Comment 7

[Ed Lee :Mardak]

Attachment: screenshot of v1

Showing off the ndash for the tooltip when it's actually needed. Also shows off the star being right aligned. Should there be a margin left of the star icon? It's okay with 0 when there's the ellipsis, but might look strange if there's text that runs into it (i.e., the text just barely didn't need ellipsis)

Comment 8

[Ed Lee :Mardak]

Attachment: non working onoverflow/onunderflow

So I tried out onoverflow and onunderflow... see patch. But it doesn't seem to help entirely.. it misses out on some overflows? At least I see results that are overflowed that don't get the ellipsis shown and no tooltip set..

Comment 9

[Ed Lee :Mardak]

Attachment: v1.1

Addresses gavin's comment about flickering ellipsis by hiding the ellipsis only when we're computing widths.

Additionally, to make sure we do hide the ellipsis quickly, use the onunderflow event.

Comment 10

[Martijn Wargers (dead)]

(In reply to comment #8)
> Created an attachment (id=308013) [details]
> non working onoverflow/onunderflow

I guess you wanted to attach a screenshot here...

> So I tried out onoverflow and onunderflow... see patch. But it doesn't seem to
> help entirely.. it misses out on some overflows? At least I see results that
> are overflowed that don't get the ellipsis shown and no tooltip set..

See bug 408621, comment 59.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 308022 [details] [diff] [review]
v1.1

>Index: toolkit/content/widgets/autocomplete.xml

>       <constructor>

>+              ellipsis = Components.classes["@mozilla.org/preferences-service;1"].
>+                getService(Components.interfaces.nsIPrefBranch).
>+                getCharValue("intl.ellipsis");

Why change to getCharValue? This is a localized pref, so this isn't going to work.

>       <method name="_adjustAcItem">

>+          // Do this for both the title and url
>+          for (let [name, val] in Iterator(vals)) {
>+            let desc = this[name];
>+            let box = this[name + "Box"];
>+            let ellipsis = this[name + "OverflowEllipsis"];
>+
>+            // Find the match and emphase it
>+            let index = val.toLowerCase().indexOf(needle);
>+            this._setUpDescription(desc, val, index, text.length);
>+
>+            // Set up overflow on a timeout because the contents of the box
>+            // might not have a width yet even though we just changed them
>+            setTimeout(this._setUpOverflow, 0, box, ellipsis);
>+          }

Can you unroll this loop and avoid the Iterator/this[] trickery? I don't think the elegance of using the loop really gains us anything, but it sure makes it harder to see what this code actually does, and hides the use of some of the members.

>+      <method name="_setUpOverflow">

>+          // Start with the parent's width and subtract off its children

Why does this method handle multiple children? There's ever only one <description> child for these boxes, right? Is this in anticipation of some other patches? Support for multiple children should be added as part of those patches, I think.

Comment 12

[Ed Lee :Mardak]

Attachment: v1.2

(In reply to comment #11)
> >+              ellipsis = Components.classes["@mozilla.org/preferences-service;1"].
> >+                getService(Components.interfaces.nsIPrefBranch).
> >+                getCharValue("intl.ellipsis");
> Why change to getCharValue? This is a localized pref, so this isn't going to
> work.
:( (It was actually supposed to be getCharPref.) But it works to get utf8 nsXPIDLCString strings it seems. Ok. Switched back to getComplexValue.

> >+          // Do this for both the title and url
> Can you unroll this loop and avoid the Iterator/this[] trickery?
Unrolled. I'll fix up later patches to duplicate the code for both title and url.

> >+      <method name="_setUpOverflow">
> >+          // Start with the parent's width and subtract off its children
> Why does this method handle multiple children?
Yeah, later patches can have multiple children. I've changed it to be a single pass instead of a loop.

Additionally, I added a little fudging on the width calculation so that subpixels that are rounded up when you do .width won't result in unnecessary ellipsis showing.

Comment 13

[Ed Lee :Mardak]

Attachment: screenshot of v1.1

FYI, what happens without the "- .5 px"-to-fix-rounding-issues tweak.

Comment 14

[Ed Lee :Mardak]

Attachment: v1.3

Restore the for loop instead of "let (i = 0) {". Fix "emphase it" typo.

Comment 15

[Ed Lee :Mardak]

Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.xml
new revision: 1.122; previous revision: 1.121
done
Checking in toolkit/themes/gnomestripe/global/autocomplete.css;
/cvsroot/mozilla/toolkit/themes/gnomestripe/global/autocomplete.css,v  <--  autocomplete.css
new revision: 1.18; previous revision: 1.17
done
Checking in toolkit/themes/pinstripe/global/autocomplete.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/global/autocomplete.css,v  <--  autocomplete.css
new revision: 1.19; previous revision: 1.18
done
Checking in toolkit/themes/winstripe/global/autocomplete.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/autocomplete.css,v  <--  autocomplete.css
new revision: 1.29; previous revision: 1.28
done

Comment 16

[Martijn Wargers (dead)]

Attachment: screenshot of ellipsis not showing on overflow

See screenshot where the ellipsis not showing up for autocomplete results in a lot of cases.
I think it happens because onoverflow/onunderflow is used, despite of the problems that were mentioned in comment 10.

Comment 17

[Ed Lee :Mardak]

We only use onunderflow, but I too have seen ellipsis not showing up sometimes. I have also noticed that setting the timeout to something bigger than 0 gets rid of the problem.