1143974 - zoom level other than 100% make the popup menu for rss service selector move away

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[u534289]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150306182708

Steps to reproduce:

download firefox frommozilla website
run it in empty profile
go to a rss feed url
zoom in or out so that the zoom is not 100%
click on button after "subscribe to this feed using"



Actual results:

the popup menu appears diagonally away from its right position that is under the clicked button


Expected results:

the popup menu should  appears  under the clicked button

Comment 1

[u534289]

this also happens for tools button in about:addons page

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]:

Steps: 
1. https://github.com/piroor/treestyletab/commits/master.atom
2. Zoom in
3. Click Select

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=47bf0d6847e4&tochange=1b9c61ae1688

Regressed by: 1b9c61ae1688	Gijs Kruitbosch — Bug 467442 - take transforms into account for popup placement, r=bz,f=tnikkel

Comment 3

[:Gijs (he/him)]

I won't have time to look at this before 37 releases because I'm occupied with a lot of other things this week.

tn, do you have more time? Otherwise, I think ideally we should try to get this fixed for 38, I guess. :-\

Comment 4

[Timothy Nikkel (:tnikkel)]

I should be able to investigate soon.

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: patch

ConvertAppUnitsRoundOut doesn't modify |this|, it returns the result.

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 8579041 [details] [diff] [review]
patch

Could we add MOZ_WARN_UNUSED_RESULT to the declaration of this method?

"Convert" in the method name here might be less than ideal since we have
a lot of other coordinate-converting functions that starts with "From"
or "To" - perhaps the name was the source of the confusion here.

Comment 7

[Timothy Nikkel (:tnikkel)]

Great ideas, I'll address them in a followup.

Comment 8

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a3c61ff20ce

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/4a3c61ff20ce

Comment 10

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #6)
> "Convert" in the method name here might be less than ideal since we have
> a lot of other coordinate-converting functions that starts with "From"
> or "To" - perhaps the name was the source of the confusion here.

Yes, not great naming. The best I could come up with is "ToDifferentAppUnits", I'm not sure if that's better or worse. Do you have any other ideas?

Comment 11

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #6)
> Could we add MOZ_WARN_UNUSED_RESULT to the declaration of this method?

Filed bug 1144951 with patch for this.

Comment 12

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8579041 [details] [diff] [review]
patch

Probably too late for beta I'm guessing.

Approval Request Comment
[Feature/regressing bug #]: bug 467442
[User impact if declined]: dropdown popup in internal RSS reader pages misplaced when zoom applied
[Describe test coverage new/current, TreeHerder]: not yet, I'll work on a test
[Risks and why]: low risk
[String/UUID change made/needed]: none

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8579041 [details] [diff] [review]
patch

Yes. Too late for Beta. (Final Beta in the 37 cycle goes to build today.) We can consider this for Aurora. Beta-

Comment 14

[Timothy Nikkel (:tnikkel)]

pushed a test
https://hg.mozilla.org/integration/mozilla-inbound/rev/22f7942307c4

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8579041 [details] [diff] [review]
patch

Small patch, taking it for aurora.

Please nominate the test too once it is ready.

Comment 16

[:Gijs (he/him)]

Can we hold off on uplift until we verify this doesn't cause hug 1145522 or fix that?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

esr38 isn't going to branch until the end of the 38 cycle ~7wks from now

Comment 18

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #16)
> Can we hold off on uplift until we verify this doesn't cause hug 1145522 or
> fix that?

I'm sorry, it turns out this didn't cause 1145522, so we could uplift, I think?

Comment 19

[Timothy Nikkel (:tnikkel)]

I just landed the test straight to inbound in a separate push a few days later, will your uplift process automatically catch that and uplift it too?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

My process is looking at the commits in the bug, so we should be good :)

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/22f7942307c4

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/93c0ea1b6b09
https://hg.mozilla.org/releases/mozilla-aurora/rev/68f723fa6438