If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Don't show entry divisor for last entry in location bar autocomplete

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Themes
--
trivial
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: Andrés Delfino, Assigned: dao)

Tracking

({fixed1.9.1, polish})

Trunk
mozilla1.9.2a1
fixed1.9.1, polish
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [polish-easy][polish-visual][polish-p1])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041909 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041909 Minefield/3.0pre

Don't show entry divisor for last entry in location bar autocomplete.

Please see screenshot.

Reproducible: Always
(Reporter)

Comment 1

10 years ago
Created attachment 316625 [details]
Windows screenshot.
(Reporter)

Updated

10 years ago
Version: unspecified → Trunk

Comment 2

10 years ago
Is this winstripe only? I don't see this on Mac.
(Reporter)

Comment 3

10 years ago
Created attachment 316998 [details]
Linux screenshot.

(In reply to comment #2)
> Is this winstripe only? I don't see this on Mac.
> 

This happens in Linux too, plus, Linux has some issues with all borders, I'll fill a bug soon for that.

See attached screenshot.
(Reporter)

Updated

10 years ago
Attachment #316625 - Attachment description: Screenshot. → Windows screenshot.
(Reporter)

Comment 4

10 years ago
(In reply to comment #3)
> This happens in Linux too, plus, Linux has some issues with all borders, I'll
> fill a bug soon for that.

Bugs are: bug 430257 and bug 430259.
Blocks: 425582
Confirmed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008042204 Minefield/3.0pre ID:2008042204
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 6

10 years ago
I tried:

.autocomplete-richlistitem:not([collapsed="true"]):last-child {
  border-bottom: none;
}

but it doesn't work as I expected it.  (It doesn't select the last item with the collapsed attribute not equal to "true").

Comment 7

10 years ago
I looked at this a few days ago, and noticed that it's not even using border-bottom:

http://mxr.mozilla.org/seamonkey/source/browser/themes/winstripe/browser/browser.css#1203

Maybe a better solution is the decrease the size of the dropdown popup by 1px (should this be done in the CSS or in the JS?), as that would also fix the problem in the case where the list is scrollable, and the extra border is coming from the last visible item, and not necessarily the last item.  But that feels a bit hackish...
Keywords: polish
Whiteboard: [polish-easy][polish-visual]
Blocks: 430259
(Assignee)

Comment 8

9 years ago
Created attachment 369061 [details] [diff] [review]
patch
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #369061 - Flags: review?(mak77)
Comment on attachment 369061 [details] [diff] [review]
patch

notice i'm not a toolkit peer so i can't give you a valid review on toolkit/content, btw i think the approach is fine.

I see some flickering of the dropdown bottom border while results are loading, maybe you should set this only on the last result when we reach maximum number of items in richlistbox or match count.

>diff --git a/toolkit/content/widgets/autocomplete.xml b/toolkit/content/widgets/autocomplete.xml
>               // set the class at the end so we can use the attributes
>               // in the xbl constructor
>               item.className = "autocomplete-richlistitem";
>               this.richlistbox.appendChild(item);
>+              existingItemsCount++;
>             }
> 
>             this._currentIndex++;
>           }
> 
>-          // yield after each batch of items so that typing the url bar is responsive
>-          setTimeout(function (self) { self._appendCurrentResult(); }, 0, this);
>+          item.setAttribute("lastvisible", "true");

item could be undefined in some situation (eg if the for cycle does not run), indeed i see some "item is undefined" error.
Attachment #369061 - Flags: review?(mak77)
to be specific i see flicker when results are loading in the hidden part of the dropdown (so 6 results are visible but new ones are being added in the scrolled out part of the dropdown)
(Assignee)

Comment 11

9 years ago
Are you sure that the flicker is caused by these changes?
yes, i don't see any flickering without those, sounds like the attribute is continuously added and removed to the last visible row (6th row)
(Assignee)

Updated

9 years ago
Component: Theme → Themes
Product: Firefox → Toolkit
QA Contact: theme → themes
(Assignee)

Comment 13

9 years ago
Created attachment 369387 [details] [diff] [review]
patch v2

OK, this can be solved entirely in CSS. Carrying over a few optimizations for _appendCurrentResult anyway.
Attachment #369061 - Attachment is obsolete: true
Attachment #369387 - Flags: review?(gavin.sharp)
Comment on attachment 369387 [details] [diff] [review]
patch v2

>diff --git a/toolkit/content/widgets/autocomplete.xml b/toolkit/content/widgets/autocomplete.xml

>             else {
>               // set the class at the end so we can use the attributes
>               // in the xbl constructor
>               item.className = "autocomplete-richlistitem";
>               this.richlistbox.appendChild(item);
>+              existingItemsCount++;

This isn't actually necessary, since the first time we hit this branch currentIndex == existingItemsCount, and it will never be < again whether or not we bother incrementing existingItemsCount (incrementing it just makes them always equal). Removing this also makes it clearer that we only care about re-using the items that are initially present in the list, so r=me with that change.
Attachment #369387 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 15

9 years ago
Created attachment 369476 [details] [diff] [review]
for check-in
Attachment #369387 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
(Assignee)

Comment 16

9 years ago
http://hg.mozilla.org/mozilla-central/rev/0030acc7b9f9
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Updated

9 years ago
Attachment #369476 - Flags: approval1.9.1?
Comment on attachment 369476 [details] [diff] [review]
for check-in

a191=beltzner
Attachment #369476 - Flags: approval1.9.1? → approval1.9.1+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
(Assignee)

Comment 18

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9792990f65e0
Keywords: checkin-needed → fixed1.9.1
Mmh, we still have a white line at the bottom of the last entry. Is that already covered by another bug or shall I file a new one?
(Assignee)

Comment 20

9 years ago
I don't understand what you're seeing. Please file a new bug.
Filed as bug 486821.
This bug's priority relative to the set of other polish bugs is:
P1 - Polish issue that appears in the main window, or is something that the user may encounter several times a day.

awesome bar results is a primary UI
Whiteboard: [polish-easy][polish-visual] → [polish-easy][polish-visual][polish-p1]
You need to log in before you can comment on or make changes to this bug.