Keyword search richlistitem too tall

VERIFIED FIXED in Firefox 35

Status

()

Toolkit
Autocomplete
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: tomasz, Assigned: dao)

Tracking

(Depends on: 2 bugs, {regression})

unspecified
mozilla35
regression
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox35+ verified)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8495548 [details]
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.
Summary: Keyword search richlistitem too high. → Keyword search richlistitem too tall
(Reporter)

Updated

3 years ago
Blocks: 1040923
(Assignee)

Comment 1

3 years ago
What bug broke this?
tracking-firefox35: --- → ?
Keywords: regression, regressionwindow-wanted
(Assignee)

Comment 2

3 years ago
Also seeing this on Linux.
OS: Mac OS X → All
Hardware: x86 → All
I think it was bug 1065303.
yeah, looks like the single line entries are breaking the richlistbox rhythm, the problem is likely the hardcoded margin around ac-title-box
Blocks: 1065303
Keywords: regressionwindow-wanted
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Points: --- → 3
Flags: qe-verify+
Flags: firefox-backlog+
Duplicate of this bug: 1074405
Blocks: 995091
Duplicate of this bug: 1074715
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

Updated

3 years ago
Iteration: --- → 35.3
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1075734

Updated

3 years ago
QA Contact: alexandra.lucinet
QA Contact: alexandra.lucinet → andrei.vaida
tracking-firefox35: ? → +
Happens all the time on Mac:

https://www.dropbox.com/s/l448xlw5add6lzk/Screenshot%202014-10-06%2015.46.03.png?dl=0
Created attachment 8501679 [details] [diff] [review]
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.
Attachment #8501679 - Flags: review?(mak77)
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.
Assignee: bmcbride → nobody
Status: ASSIGNED → NEW
Iteration: 35.3 → ---

Updated

3 years ago
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 12

3 years ago
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.
Attachment #8501679 - Flags: review?(mak77) → review-
(Assignee)

Comment 13

3 years ago
I have an idea for a more reliable approach.
Assignee: nobody → dao
Status: NEW → ASSIGNED
(Assignee)

Comment 14

3 years ago
Created attachment 8501700 [details] [diff] [review]
patch

Still compiling (had to clobber), but based on my userChrome.css testing I think this should work.
Attachment #8501679 - Attachment is obsolete: true
Attachment #8501700 - Flags: review?(gijskruitbosch+bugs)
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.
(Assignee)

Comment 16

3 years ago
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.
(Assignee)

Comment 17

3 years ago
Created attachment 8501714 [details] [diff] [review]
patch v2
Attachment #8501700 - Attachment is obsolete: true
Attachment #8501700 - Flags: review?(gijskruitbosch+bugs)
Attachment #8501714 - Flags: review?(mak77)

Updated

3 years ago
Iteration: --- → 35.3
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.
Attachment #8501714 - Flags: review?(mak77) → review+
(Assignee)

Comment 19

3 years ago
(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.
(Assignee)

Comment 20

3 years ago
Created attachment 8501901 [details] [diff] [review]
patch, comments added

https://hg.mozilla.org/integration/fx-team/rev/a71600700b25
Attachment #8501714 - Attachment is obsolete: true
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8501901 [details] [diff] [review]
patch, comments added

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

Huh - nice idea!
Attachment #8501901 - Flags: feedback+
https://hg.mozilla.org/mozilla-central/rev/a71600700b25
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox35: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla35

Comment 24

3 years ago
(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).
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.
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.
Status: RESOLVED → VERIFIED
status-firefox35: fixed → verified
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.
Depends on: 1081624
Depends on: 1088000
You need to log in before you can comment on or make changes to this bug.