Closed Bug 901946 Opened 11 years ago Closed 6 years ago

some exotic locales (eg: zh-TW, or) builds on Linux and OSX wrongly calculate Autocomplete Popup height

Categories

(Firefox :: Address Bar, defect)

defect
Not set
normal

Tracking

()

RESOLVED INACTIVE

People

(Reporter: andrei, Unassigned)

References

Details

(Keywords: regression, Whiteboard: [qa-automation-wanted])

Attachments

(1 file)

We've seen this in our automation tests. This is reproductible on Linux (Ubuntu 32b, 13.04) with a localised zh-TW build. 22 and up is affected. I have a simplified testcase prepared in bug 818128 attachement 786282 For a screenshot see bug 818128 attachement 780292 The problem is the computed height of Autocomplete Popup is bigger than the cumulative height of the first 6 items rendered.
Can we get a regression range please for this issue? That should make it easier to fix and to find the right person to work on this bug.
Sure, I'll find the changeset that introduced this.
Hi, I found the changeset that causes this failure, because in FF we calculate the height of Autocomplete Popup, by multiplying the first item, we might have items that doesn't have the same height so function getNumberOfVisibleRows will not work correctly. The changeset that introduced this is: http://hg.mozilla.org/mozilla-central/rev/0fc382c36b4c Jared can you look in to this ? Here are the dumps with the heights >Item index: 0 -- Item height: 48px - is visible: true >Item index: 1 -- Item height: 47px - is visible: true >Item index: 2 -- Item height: 48px - is visible: true >Item index: 3 -- Item height: 48px - is visible: true >Item index: 4 -- Item height: 48px - is visible: true >Item index: 5 -- Item height: 48px - is visible: true >Item index: 6 -- Item height: 48px - is visible: true >First 6 children cumulative height: 287 >PopupAutoCompleteRichResult (parent) height: 288
Blocks: 804968
Flags: needinfo?(jaws)
Do we know why there is text from two different languages being rendered on top of each other?
Flags: needinfo?(jaws)
Jared, from what I see the title is taken from the meta-tag title of the page being suggested and the urls are latin alphabet. In minimized TC the first two suggestions are the two links visited and the other four are the default bookmarked addresses from FF, which are localized.
Jared, see bug 818128 attachment 780292 [details] The screen is an overlay of en-US vs zh-TW, but you can still see that most items contain the title and the url of a previously visited page (all in English). But the first item is "Switch to tab" which in zh-TW contains a localised version. There seems to be an actual case where the height of the Autocomplete Popup can't reliably be calculated from the height of the first item.
Flags: needinfo?(jaws)
(In reply to Andrei Eftimie from comment #7) > Jared, see bug 818128 attachment 780292 [details] > > The screen is an overlay of en-US vs zh-TW, but you can still see that most > items contain the title and the url of a previously visited page (all in > English). Thank you, it wasn't clear to me that this was two screenshots placed on top of each other.
Attached patch Patch (deleted) — Splinter Review
Sorry for the delay here. The problem looks to come from the switcht-to-tab icon only being 10px tall on Linux but on OSX and Windows we show it as 11px tall. Cosmin, can you test out this patch and let me know if it fixes the issue? Let me know if you need anything else from me.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #8346857 - Flags: feedback?(cosmin.malutan)
Flags: needinfo?(jaws)
(In reply to Jared Wein [:jaws] (Away 20 Dec to 2 Jan) from comment #9) > The problem looks to come from the switcht-to-tab icon only being 10px tall on Linux Why would this be a problem?
Comment on attachment 8346857 [details] [diff] [review] Patch Review of attachment 8346857 [details] [diff] [review]: ----------------------------------------------------------------- This does not fix the issue :( Inspecting the autocomplete elements with the DOM Inspector, I get the following heights: Regular item: Total Height 47px > ||========|| > || 6px || // padding > || |----| || > || |18px| || // height > || |----| || > || 1px || // margin-top > || |----| || > || |16px| || // height > || |----| || > || 6px || // padding > ||========|| Patched autocomplete item (tab switch item): Total Height 48px > ||========|| > || 6px || // padding > || |----| || > || |18px| || // height > || |----| || > || 1px || // margin-top > || |----| || > || |17px| || // height > || |----| || > || 6px || // padding > ||========|| If you run the testcase from bug 818128 [1] you will notice the following results: Without the patch applied: > Item index: 0 -- Item height: 48px // this is the switch-to-tab-item where the patch change applies > Item index: 1 -- Item height: 47px > Item index: 2 -- Item height: 48px > Item index: 3 -- Item height: 48px > Item index: 4 -- Item height: 48px > Item index: 5 -- Item height: 48px > Item index: 6 -- Item height: 48px > First 6 children cumulative height: 287 > PopupAutoCompleteRichResult (parent) height: 288 With the patch > Item index: 0 -- Item height: 48px // this is the switch-to-tab-item where the patch change applies > Item index: 1 -- Item height: 47px > Item index: 2 -- Item height: 48px > Item index: 3 -- Item height: 48px > Item index: 4 -- Item height: 48px > Item index: 5 -- Item height: 48px > Item index: 6 -- Item height: 48px > First 6 children cumulative height: 287 > PopupAutoCompleteRichResult (parent) height: 288 Run the testcase with a zh-TW build, and you will notice that the only element that is reporting a 47px height is the one that has all test in English. All other items have at leas 1 row of text in Chinese. Because the DOM inspector is showing me 47px height for all items, but while running the test we get 48px for all items that render exotic glyphs leads me to think that the problem is probably not with an image but maybe the rendered text. Digging deeper I've found that if we set both: > .ac-title { height: 18px; } > .ac-url { height: 16px; } The issue goes away. But for a real solution we probably shouldn't hack any fixed dimensions :) So whenever we have exotic glyphs rendered in any of these items, they render 1px taller than when only having wastern/ascii characters. [1] To run the testcase: a) Download the testcase and the latest mozmill-env from http://mozqa.com/mozmill-env for your system b) Unzip c) Activate the environment on *nix systems ( . mozmill-env/bin/activate ) or run the executable under windows d) run the command: > mozmill -t testcase.js -b %path/to/firefox/binary%
Attachment #8346857 - Flags: feedback?(cosmin.malutan) → feedback-
Interestingly I get pretty good results by setting the line-height on some of these elements. They don't appear to have it explicitly set anywhere.
(In reply to Andrei Eftimie from comment #12) > Interestingly I get pretty good results by setting the line-height on some > of these elements. > They don't appear to have it explicitly set anywhere. Dao, what do you think about setting the line-height here?
Flags: needinfo?(dao)
Setting a line-height wouldn't be ideal for exotic glyphs that need more room, right?
Flags: needinfo?(dao)
Flags: needinfo?(jaws)
Do we have any updates here? At least a direction? (In reply to Dão Gottwald [:dao] (on vacation from March 21 to 28) from comment #14) > Setting a line-height wouldn't be ideal for exotic glyphs that need more > room, right? Line-height should not influence the rendering of the glyphs themselves. They will still be drawn. It might be a problem if we have multiple lines and the line-height if way smaller then it should be. In that case the lines will overlap. But I don't see this affecting us in any way in this particular case.
Should we just back out the offending patch?
Flags: needinfo?(jaws)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16) > Should we just back out the offending patch? To whom goes this question? I assume Dao?
Flags: needinfo?(dao)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16) > Should we just back out the offending patch? Maybe. I don't know relevant bug 804968 is for the real world. Autocompleting data URIs probably isn't something that many users do. Another option might be to use background-image for .ac-action-icon. Then again, that's just this particular case, generally there's no guarantee that all autocomplete items will have the same height. So maybe the assumption made in bug 804968 was just wrong.
Flags: needinfo?(dao)
Unassigning as I haven't been working on this and I don't want to hold people up.
Assignee: jaws → nobody
Status: ASSIGNED → NEW
Is there someone interested to help with this issue?
Whiteboard: [qa-automation-blocked]
OS: Linux → All
Summary: zh-TW builds on Linux wrongly calculate Autocomplete Popup height → some exotic locales (eg: zh-TW, or) builds on Linux and OSX wrongly calculate Autocomplete Popup height
Well, we changed the way in how our Mozmill test works to workaround this problem for now. So the test is no longer blocked.
Whiteboard: [qa-automation-blocked] → [qa-automation-wanted]
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: