Closed
Bug 421412
Opened 17 years ago
Closed 17 years ago
Get rid of _adjustWidth hack
Categories
(Toolkit :: Autocomplete, defect, P2)
Toolkit
Autocomplete
Tracking
()
RESOLVED
FIXED
mozilla1.9beta5
People
(Reporter: Mardak, Assigned: Mardak)
References
Details
(Whiteboard: [fixes bug 411963, bug 421414, bug 396548, bug 406373])
Attachments
(5 files, 3 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/gif
|
Details |
We can use flex to push the icon to the right, but we need to make sure the ellipsis show up correctly..
Comment 1•17 years ago
|
||
you might want to take a look at bug #408621
Comment 2•17 years ago
|
||
see also bug #405913 and bug #405918
Assignee | ||
Comment 3•17 years ago
|
||
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
Assignee | ||
Comment 4•17 years ago
|
||
This will fix bug 411963.
FYI, I did this patch at the top of my stack.
Blocks: 411963
Assignee | ||
Comment 5•17 years ago
|
||
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
Assignee | ||
Comment 6•17 years ago
|
||
This fixes P2 blocking bug 396548.
Blocks: 396548
Flags: blocking1.9?
Whiteboard: [has patch][need review gavin][fixes bug 411963, bug 421414, bug 396548]
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Assignee | ||
Comment 7•17 years ago
|
||
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)
Attachment #307950 -
Flags: ui-review?(beltzner)
Assignee | ||
Updated•17 years ago
|
Attachment #307950 -
Attachment is patch: false
Attachment #307950 -
Attachment mime type: text/plain → image/png
Assignee | ||
Comment 8•17 years ago
|
||
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..
Assignee | ||
Comment 9•17 years ago
|
||
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.
Attachment #307837 -
Attachment is obsolete: true
Attachment #308022 -
Flags: review?(gavin.sharp)
Attachment #307837 -
Flags: review?(gavin.sharp)
Comment 10•17 years ago
|
||
(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•17 years ago
|
||
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.
Assignee | ||
Comment 12•17 years ago
|
||
(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.
Attachment #308022 -
Attachment is obsolete: true
Attachment #308102 -
Flags: review?(gavin.sharp)
Attachment #308022 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 13•17 years ago
|
||
FYI, what happens without the "- .5 px"-to-fix-rounding-issues tweak.
Assignee | ||
Comment 14•17 years ago
|
||
Restore the for loop instead of "let (i = 0) {". Fix "emphase it" typo.
Attachment #308102 -
Attachment is obsolete: true
Attachment #308108 -
Flags: review?(gavin.sharp)
Attachment #308102 -
Flags: review?(gavin.sharp)
Updated•17 years ago
|
Attachment #308108 -
Flags: review?(gavin.sharp) → review+
Updated•17 years ago
|
Whiteboard: [has patch][need review gavin][fixes bug 411963, bug 421414, bug 396548] → [has patch][fixes bug 411963, bug 421414, bug 396548]
Assignee | ||
Comment 15•17 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch][fixes bug 411963, bug 421414, bug 396548] → [fixes bug 411963, bug 421414, bug 396548]
Target Milestone: --- → mozilla1.9beta5
Assignee | ||
Updated•17 years ago
|
Whiteboard: [fixes bug 411963, bug 421414, bug 396548] → [fixes bug 411963, bug 421414, bug 396548, bug 406373]
Assignee | ||
Updated•17 years ago
|
Attachment #307950 -
Flags: ui-review?(beltzner)
Comment 16•17 years ago
|
||
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.
Assignee | ||
Comment 17•17 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•