1073243 - Keyword search richlistitem too tall

Status: VERIFIED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Tomasz Kołodziejski [:tomasz]]

Attachment: Screen Shot 2014-09-25 at 3.05.45 PM.png

After recent awesomebar changes the keyword search richlistitem appears too big as in the screenshot. The last item is then half visible and the suggestion popup jumps when you scroll through it.

Comment 1

[Dão Gottwald [:dao]]

What bug broke this?

Comment 2

[Dão Gottwald [:dao]]

Also seeing this on Linux.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

I think it was bug 1065303.

Comment 4

[Marco Bonardo [:mak]]

yeah, looks like the single line entries are breaking the richlistbox rhythm, the problem is likely the hardcoded margin around ac-title-box

Comment 7

[Andrei Vaida [:avaida]]

I managed to obtain a regression range (per Bug 1074715):

(m-c):
Last good revision: c8e325eee9e1 (2014-09-19)
First bad revision: 27253887d2cc (2014-09-20)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c8e325eee9e1&tochange=27253887d2cc

(m-i):
Last good revision: 18fe3472680f
First bad revision: 206e963fb3dd
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=18fe3472680f&tochange=206e963fb3dd

Probable cause:
86a707d5ffdb - Blair McBride — Bug 1065303 - Prepare autocomplete.xml/UnifiedComplete for adding new special result types and heuristics. r=mak

Comment 9

[Richard Newman [:rnewman]]

Happens all the time on Mac:

https://www.dropbox.com/s/l448xlw5add6lzk/Screenshot%202014-10-06%2015.46.03.png?dl=0

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1 - no test

Here's a fix, but it could do with a test - I haven't had time yet to get to writing on.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

If someone else could step in to finish this, it would be appreciated - I know this bug is frustrating, but I'm stuck on stuff that needs to be higher priority.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8501679 [details] [diff] [review]
Patch v1 - no test

This still appears to make bogus assumptions about the size of the hidden text line. Font sizes are customizable on Windows and Linux.

Comment 13

[Dão Gottwald [:dao]]

I have an idea for a more reliable approach.

Comment 14

[Dão Gottwald [:dao]]

Attachment: patch

Still compiling (had to clobber), but based on my userChrome.css testing I think this should work.

Comment 15

[Marco Bonardo [:mak]]

Clever idea! It seems to be working pretty well here, though, at least on Windows, the text is not well centered.
transform: translateY(50%); seems to make it nicely centered from brief testing.

Comment 16

[Dão Gottwald [:dao]]

50% doesn't work well over here on Linux. I think the difference on Windows is due to ac-url-box having extra vertical margin there. I can make translateY take that into account directly.

Comment 17

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 8501714 [details] [diff] [review]
patch v2

Review of attachment 8501714 [details] [diff] [review]:
-----------------------------------------------------------------

Tested on Windows 8.1 and Mavericks, it looks pretty good. I didn't test on Linux, since you did already.

I'm only a little bit scared by the fact if someone touches the margin and doesn't update the translate, this will regress... do you think it may be worth to add a comment in the margin rule stating that "If you change this, you must also correct the ac-title-box translateY rule below."? (or something along those lines). It would be needed for all of the 3 themes.
Or just put the visibility rule after the translateY ones, so that the margin rule and the translateY rule are closer.

Comment 19

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #18)
> I'm only a little bit scared by the fact if someone touches the margin and
> doesn't update the translate, this will regress...

If the translateY doesn't take into account the margin, the title won't be centered vertically, but it won't affect the height of the richlistitem. So this particular bug won't regress.

Comment 20

[Dão Gottwald [:dao]]

Attachment: patch, comments added

https://hg.mozilla.org/integration/fx-team/rev/a71600700b25

Comment 21

[Marco Bonardo [:mak]]

(In reply to Dão Gottwald [:dao] from comment #19)
>  So this particular bug won't regress.

right, sorry I means "visually regress". Thanks for the patch!

I assume the needinfo to Gijs was cause he meant to take this bug, thus I'm clearing it, hope I'm not doing anything wrong.

Comment 22

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8501901 [details] [diff] [review]
patch, comments added

Review of attachment 8501901 [details] [diff] [review]:
-----------------------------------------------------------------

Huh - nice idea!

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a71600700b25

Comment 24

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #21)
> (In reply to Dão Gottwald [:dao] from comment #19)
> >  So this particular bug won't regress.
> 
> right, sorry I means "visually regress". Thanks for the patch!
> 
> I assume the needinfo to Gijs was cause he meant to take this bug, thus I'm
> clearing it, hope I'm not doing anything wrong.

Correct, and thank you!

I did think this morning, as a random brainwave... couldn't this just use line-height? I've not looked into it in detail, so maybe it's not an option, but that would seem less hacky than the transform (which has me worried about perf because... because).

Comment 25

[Marco Bonardo [:mak]]

imho, it would be hard to preserve the richlistbox rhythm with line-height. And it's a life-saver to have all of the entries the same height here.

Comment 26

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 35.0a1 (2014-10-09) using windows 7 64-bit, Ubuntu 14.04 LTS 32-bit and Mac OS X 10.9.5.

Comment 27

[Blair McBride [:Unfocused] (UNAVAILABLE)]

This shouldn't have landed without a test - we have far too little test coverage for autocomplete UI, and it's prone to regressions. Filed bug 1081624.