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

Status: RESOLVED INACTIVE

(Firefox :: Address Bar, defect)

Description

[Andrei Eftimie]

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.

Comment 1

[Andrei Eftimie]

Typo:
testcase: bug 818128 attachment 786282 [details]
screenshot: bug 818128 attachment 780292 [details]

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 3

[Andrei Eftimie]

Sure, I'll find the changeset that introduced this.

Comment 4

[Cosmin Malutan, [:cosmin-malutan]]

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

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Do we know why there is text from two different languages being rendered on top of each other?

Comment 6

[Cosmin Malutan, [:cosmin-malutan]]

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.

Comment 7

[Andrei Eftimie]

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.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

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.

Comment 10

[Dão Gottwald [:dao]]

(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 11

[Andrei Eftimie]

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%

Comment 12

[Andrei Eftimie]

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.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

(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?

Comment 14

[Dão Gottwald [:dao]]

Setting a line-height wouldn't be ideal for exotic glyphs that need more room, right?

Comment 15

[Andrei Eftimie]

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.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Should we just back out the offending patch?

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(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?

Comment 18

[Dão Gottwald [: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.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Unassigning as I haven't been working on this and I don't want to hold people up.

Comment 20

[Andreea Matei [:AndreeaMatei]]

Is there someone interested to help with this issue?

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Well, we changed the way in how our Mozmill test works to workaround this problem for now. So the test is no longer blocked.

Comment 22

[BMO Automation]

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.