Closed Bug 954872 Opened 10 years ago Closed 10 years ago

Typing the beginning of a protocol name listed in the top protocols should select it

Categories

(Instantbird Graveyard :: Account wizard, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: wnayes)

References

Details

Attachments

(1 file, 3 obsolete files)

*** Original post on bio 1438 at 2012-05-18 09:27:00 UTC ***

I was creating a twitter test account, and I tried to type "T" several times to select the twitter item before accepting that it wasn't going to work and I was going to have to use the arrow to select an item.
Blocks: 954826
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1438 as attmnt 1632 at 2012-06-19 10:36:00 UTC ***

The code handling this is at:
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/listbox.xml#611

See specifically:
643           // allow richlistitems to specify the string being searched for
644           var searchText = "searchLabel" in listitem ? listitem.searchLabel :
645                            listitem.getAttribute("label"); // (see also bio 250123)

And then:
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/widgets/richlistbox.xml#553

I think using the label tag instead of the description tag for the protocol name is more correct. Unfortunately this.getElementsByTagNameNS(XULNS, "label") doesn't seem to get the label elements wrapped in a vbox, so I had to override the label property in the topProtocol binding (note: this label property is also used by screen reader, so it was another bug in itself that the label property was empty for items of the top protocol list). Then I had to adapt the css to keep the same appearance.
Attachment #8353389 - Flags: review?(wnayes)
Assignee: nobody → florian
Attached image CSS Margin changes from the patch (obsolete) —
*** Original post on bio 1438 as attmnt 1636 at 2012-06-19 17:56:00 UTC ***

On my build, the 4px margin fixes the height issue, but there also seems to be an issue with the left padding (see attachment). The keyboard selection of the list items works great though!

The margin-start-value of 6px on the label seems to be the issue (removing it aligns the two lines of text).
Attached patch Added fixed padding and margins (obsolete) — Splinter Review
*** Original post on bio 1438 as attmnt 1637 at 2012-06-20 00:23:00 UTC ***

Added an (arbitrary) margin and padding to the two CSS classes used by the label and description elements in the Top Protocols items.
Attachment #8353394 - Flags: review?(florian)
Comment on attachment 8353389 [details] [diff] [review]
Patch

*** Original change on bio 1438 attmnt 1632 at 2012-06-20 00:23:15 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353389 - Attachment is obsolete: true
Attachment #8353389 - Flags: review?(wnayes)
Comment on attachment 8353393 [details]
CSS Margin changes from the patch

*** Original change on bio 1438 attmnt 1636 at 2012-06-20 00:23:15 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353393 - Attachment is obsolete: true
*** Original post on bio 1438 at 2012-06-20 09:49:16 UTC ***

Comment on attachment 8353394 [details] [diff] [review] (bio-attmnt 1637)
Added fixed padding and margins


> .top-proto-name {
>+  margin: 2px 4px;
>+  padding: 0px;
>   font-size: larger;
> }
> 
> .top-proto-description {
>+  margin: 2px 4px;
>+  padding: 0px;
>   font-size: smaller;
>   opacity: 0.85;
> }

I think I would prefer if the margin/padding rules weren't duplicated, I guess I prefer duplicating the selectors:
.top-proto-name, .top-proto-description {
/* spacing rules */
}

I tried the patch, and the 4px margin seems a little bit too much. The icon already has a 4px spacing on both sides, so the most logical value would have been a start margin of 0px for the labels, but visually it doesn't look really right. After trying a few things, I think I prefer margin: 2px 2px;

Thanks for taking this over! :)
Comment on attachment 8353394 [details] [diff] [review]
Added fixed padding and margins

*** Original change on bio 1438 attmnt 1637 at 2012-06-20 11:12:17 UTC ***

r- per comment 4, as I would like a new patch before checking this in, but it looks good :).
Attachment #8353394 - Flags: review?(florian) → review-
*** Original post on bio 1438 as attmnt 1646 at 2012-06-20 17:32:00 UTC ***

Here's a patch with those changes. I think I had tried margin: 2px 2px; at one point, but changed it to 4px as that's what it was previously (at least on Linux). Either works for me though. :)
Attachment #8353403 - Flags: review?(florian)
Comment on attachment 8353394 [details] [diff] [review]
Added fixed padding and margins

*** Original change on bio 1438 attmnt 1637 at 2012-06-20 17:32:14 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353394 - Attachment is obsolete: true
Comment on attachment 8353403 [details] [diff] [review]
CSS grouping of margin and padding rules

*** Original change on bio 1438 attmnt 1646 at 2012-06-20 23:10:15 UTC ***

Thanks!
Attachment #8353403 - Flags: review?(florian) → review+
*** Original post on bio 1438 at 2012-06-21 00:30:55 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/27f653d6c7b9 Thanks! :)
Assignee: florian → wnayes
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.