1020505 - Regression: Browser menu has width issues

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

Attachment: Screenshot_2014-06-04-11-16-18.png

Note how Request Desktop Site's checkbox hangs off the right hand side.

Quickshare backout regression?

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

What device? Could be a layout XML issue for your DPI, blah, blah, blah.

Comment 2

[Richard Newman [:rnewman]]

HTC One M7, KitKat.

Comment 3

[Aaron Train [:aaronmt]]

Oddly, on my Samsung Galaxy S5 (Android 4.4.4), I see this only on Nightly .. unless something triggers this. The checkbox on my S5 is even further pushed off-screen from the screenshot attached.

Comment 4

[Aaron Train [:aaronmt]]

Also see this on my Nexus 5 (Nightly (08/19)).

Mihai, can you help with a regression range here if you can reproduce?

Comment 5

[u421692]

2014-08-14 - good
2014-08-15 - bad
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5299864050ee&tochange=c9f8cc9ce89c

Comment 6

[u421692]

1408067296 - good
1408071493 - bad
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f9c51223ffc9&tochange=842b44860ee2

I was only able to reproduce this issue on LG Nexus 4(Android 4.4.4).

Comment 7

[Aaron Train [:aaronmt]]

Can you check fxteam?

Comment 8

[u421692]

1408045988 - good
1408047848 - bad
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=070d5351ed82&tochange=b9fc8a08a0c2

It seems that Bug 1042829 - Update Android SDK and build tools to 20 , caused this regression

Comment 9

[Lucas Rocha (:lucasr)]

Wes, any ideas?

Comment 10

[Lucas Rocha (:lucasr)]

Clearing NEEDINFO. I'm taking a look at this.

Comment 13

[Lucas Rocha (:lucasr)]

Attachment: Don't truncate the menu popup width (r=margaret)

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 8482845 [details] [diff] [review]
Don't truncate the menu popup width (r=margaret)

Not entirely sure what's causing the problem but removing the redundant MeasureSpec from the setWindowLayoutMode() call fixes. We set the width explicitly in menu_popup.xml already. Using WRAP_CONTENT is actually the right thing to do here. My guess is that PopupWindow doesn't deal very well with AT_MOST measure specs.

I can dive deeper into PopupWindow's source code but I'm happy to land this as an immediate fix.

Comment 15

[Wesley Johnston (:wesj)]

Careful. This was put in to fix bug 775272.

Comment 16

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #15)
> Careful. This was put in to fix bug 775272.

It looks like that patch also added the explicit width in menu_popup.xml. I'm not sure why that wouldn't have been enough to fix the issue, but it sounds like it wasn't in bug 775272 comment 6.

Lucas, can you make sure this patch doesn't regress the fix from bug 775272?

Comment 17

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #16)
> (In reply to Wesley Johnston (:wesj) from comment #15)
> > Careful. This was put in to fix bug 775272.
> 
> It looks like that patch also added the explicit width in menu_popup.xml.
> I'm not sure why that wouldn't have been enough to fix the issue, but it
> sounds like it wasn't in bug 775272 comment 6.
> 
> Lucas, can you make sure this patch doesn't regress the fix from bug 775272?

Tested on a phone with L and a tablet with KitKat. Works fine. It would be nice if we could test this on devices with ICS and JB just to be safe.

Aaron, do you have tablets & phones running ICS and JB to this APK? Just want to make sure this patch doesn't regress the fix from bug 775272.

https://dl.dropboxusercontent.com/u/1187037/fennec-menu-fix.apk

Comment 18

[Aaron Train [:aaronmt]]

I'll take a look, but Mihai, can you guys take a look too?

Comment 19

[u421692]

It looks to be fixed, tested with Lucas build on:
LG Nexus 4(Android 4.4.4)
Alcatel One Touch(Android 4.1.2)
Samsung Galaxy Tab 2(Android 4.2.2)
LG Optimus 4X(Android 4.1.2)

Comment 20

[:Margaret Leibovic]

Comment on attachment 8482845 [details] [diff] [review]
Don't truncate the menu popup width (r=margaret)

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

Thanks for testing. This change seems fine, maybe this part of the patch wasn't actually necessary for bug 775272.

Comment 21

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/182716b666bf

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/182716b666bf

Comment 23

[Lucas Rocha (:lucasr)]

Comment on attachment 8482845 [details] [diff] [review]
Don't truncate the menu popup width (r=margaret)

Approval Request Comment
[Feature/regressing bug #]: bug 1042829
[User impact if declined]: App menu looks broken.
[Describe test coverage new/current, TBPL]: Looks fine in Nightly.
[Risks and why]: Low, just changing the way a dialog is sized.
[String/UUID change made/needed]: n/a

Comment 24

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

Comment on attachment 8482845 [details] [diff] [review]
Don't truncate the menu popup width (r=margaret)

Aurora+

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/99043f498353

Comment 26

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in
Build: Firefox for Android 34.0a2 (2014-09-17)
Device: Nexus 4 Android 4.4.4