Closed Bug 421412 Opened 17 years ago Closed 17 years ago

Get rid of _adjustWidth hack

Categories

(Toolkit :: Autocomplete, defect, P2)

defect

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)

We can use flex to push the icon to the right, but we need to make sure the ellipsis show up correctly..
you might want to take a look at bug #408621
Attached patch v1 (obsolete) (deleted) — Splinter Review
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: nobody → edilee
Status: NEW → ASSIGNED
Attachment #307837 - Flags: review?(gavin.sharp)
This will fix bug 411963. FYI, I did this patch at the top of my stack.
Blocks: 411963
Blocks: 421414
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
This fixes P2 blocking bug 396548.
Blocks: 396548
Flags: blocking1.9?
Whiteboard: [has patch][need review gavin][fixes bug 411963, bug 421414, bug 396548]
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Depends on: 408621
Attached image screenshot of v1 (deleted) —
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)
Attachment #307950 - Attachment is patch: false
Attachment #307950 - Attachment mime type: text/plain → image/png
Blocks: 407946
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..
Attached patch v1.1 (obsolete) (deleted) — Splinter Review
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)
(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 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.
Attached patch v1.2 (obsolete) (deleted) — Splinter Review
(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)
Attached image screenshot of v1.1 (deleted) —
FYI, what happens without the "- .5 px"-to-fix-rounding-issues tweak.
Attached patch v1.3 (deleted) — Splinter Review
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)
Attachment #308108 - Flags: review?(gavin.sharp) → review+
Whiteboard: [has patch][need review gavin][fixes bug 411963, bug 421414, bug 396548] → [has patch][fixes bug 411963, bug 421414, bug 396548]
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
Blocks: 406373
Whiteboard: [fixes bug 411963, bug 421414, bug 396548] → [fixes bug 411963, bug 421414, bug 396548, bug 406373]
Attachment #307950 - Flags: ui-review?(beltzner)
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.
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.

Attachment

General

Creator:
Created:
Updated:
Size: