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)
Instantbird Graveyard
Account wizard
Tracking
(Not tracked)
RESOLVED
FIXED
1.2
People
(Reporter: florian, Assigned: wnayes)
References
Details
Attachments
(1 file, 3 obsolete files)
3.17 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
*** 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.
Reporter | ||
Comment 1•10 years ago
|
||
*** 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)
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → florian
Assignee | ||
Comment 2•10 years ago
|
||
*** 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).
Assignee | ||
Comment 3•10 years ago
|
||
*** 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)
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
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
Reporter | ||
Comment 6•10 years ago
|
||
*** 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! :)
Reporter | ||
Comment 7•10 years ago
|
||
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-
Assignee | ||
Comment 8•10 years ago
|
||
*** 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)
Assignee | ||
Comment 9•10 years ago
|
||
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
Reporter | ||
Comment 10•10 years ago
|
||
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+
Comment 11•10 years ago
|
||
*** 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.
Description
•