Closed Bug 1143974 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: XUL, defect)

33 Branch
x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
firefox39 --- fixed
firefox-esr31 --- unaffected

People

(Reporter: u534289, Assigned: tnikkel)

References

Details

(Keywords: regression)

Attachments

(1 file)

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
this also happens for tools button in about:addons page
[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
Blocks: 467442
Status: UNCONFIRMED → NEW
Component: Untriaged → XP Toolkit/Widgets: Menus
Ever confirmed: true
Flags: needinfo?(gijskruitbosch+bugs)
Keywords: regression
OS: Linux → All
Product: Firefox → Core
Version: 36 Branch → 33 Branch
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. :-\
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(tnikkel)
I should be able to investigate soon.
Attached patch patchSplinter Review
ConvertAppUnitsRoundOut doesn't modify |this|, it returns the result.
Assignee: nobody → tnikkel
Flags: needinfo?(tnikkel)
Attachment #8579041 - Flags: review?(mats)
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.
Attachment #8579041 - Flags: review?(mats) → review+
Great ideas, I'll address them in a followup.
https://hg.mozilla.org/mozilla-central/rev/4a3c61ff20ce
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(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?
(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 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
Attachment #8579041 - Flags: approval-mozilla-beta?
Attachment #8579041 - Flags: approval-mozilla-aurora?
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-
Attachment #8579041 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8579041 [details] [diff] [review]
patch

Small patch, taking it for aurora.

Please nominate the test too once it is ready.
Attachment #8579041 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1145522
Can we hold off on uplift until we verify this doesn't cause hug 1145522 or fix that?
No longer depends on: 1145522
esr38 isn't going to branch until the end of the 38 cycle ~7wks from now
Whiteboard: [NO_UPLIFT}
(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?
Whiteboard: [NO_UPLIFT}
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?
Flags: needinfo?(ryanvm)
My process is looking at the commits in the bug, so we should be good :)
Flags: needinfo?(ryanvm)
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in before you can comment on or make changes to this bug.