586009 - Context menu with a long link opens the tab sidebar when instanced

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aakash Desai [:aakashd]]

Build Id:
Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:2.0b4pre) Gecko/20100810 Namoroka/4.0b4pre Fennec/2.0a1pre

and

Mozilla/5.0 (Android; U; Linux armv71; en-US; rv:2.0b4pre) Gecko/20100810 Namoroka/4.0b4pre Fennec/2.0a1pre

Steps to Reproduce:
1. Go to addons.mozilla.org/mobile
2. Context click on the "Add to mobile" button for Reporter for Fennec

Actual Results:
The tab sidebar is opened in the background when the context menu pops up.

Expected Results:
The tab sidebar should not be opened in the background when the context menu pops up.

Comment 1

[Aakash Desai [:aakashd]]

This also happens when context clicking on a nightly build ftp latest-mobile-trunk

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Mystery fix

Adding a min-width with percentage units to the container fixes this... but it probably shouldn't work.

I'm assuming the problem comes because we call getClientBoundingRect on the popup, which does reflow with the unrestricted size popup, and then we restrict it to be smaller later. I'm guessing the rule changes how the reflow plays out.

I'll probably come back to this later, but if someone knows more about why this works I'd love to hear it.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Removing sizing code

Here's another possible fix for this. This removes all the sizing code, and just lets CSS do its magic. I'm not sure why the sizing code was there to begin with. Do we need it?

Comment 4

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

Comment on attachment 470105 [details] [diff] [review]
Removing sizing code

Looks like you are removing the MenuListHelperUI.sizeToContent. I would have guessed, ContextHelper.sizeToContent

> -  padding: 64px;
> +  padding: 12.5%;

Is this the change that makes it work?

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 470105 [details] [diff] [review]
Removing sizing code

You know what. I tested this a dozen times on Friday to make sure that the bug appeared and disappeared with and without but it, but those same tests don't work now.

I'm removing the review request. I'll be happy to take feedback from vingtetun, but I've got to play with this a bit more too I guess. :(

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 470105 [details] [diff] [review]
Removing sizing code

I would like to see sizeToContent remove in both places (ContextHelper && MenuListHelperUI) but we still need to keep them in case of a very very long url or title. Otherwise the right side of the popup will be outside the window.

I will be very happy if you're able to remove the sizeToContent code and have the UI result we want: * popup in the middle
* flex working (crop)
* margin on each side (btw, I think using a percent could definitively help here!)
* resize enabled
* Number of items aware: If the context menu hold two items and only one in the next instance we want the size of the popup to change and reflect that

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Just handle maxWidth

I swear there has to be a CSS only method of doing this, but this works under every test case I can find. Asking

Comment 8

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

Comment on attachment 470950 [details] [diff] [review]
Just handle maxWidth

I added:

this._popup.maxHeight = window.innerHeight * 0.75;

To the changes since the code being removed also handled height. This seems to work. I tested the contextmenu UI and the menulist UI. Both seemed to work OK.

I'd love to have more rigorous testing. Especially with large lists.

r+, but I am attaching my patch for Vivien to look over too.

Comment 9

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

Attachment: patch 2

Updated Wes' patch and want Vivien to review too.

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #9)
> Created attachment 471040 [details] [diff] [review]
> patch 2
> 
> Updated Wes' patch and want Vivien to review too.

It still stays a small bug to fix when resizing the window many times (see the bottom part of the screenshot) but this works pretty well, I'm kind of confused of the code now.

Mark, have you not spoke of adding a flex somewhere yesterday (on the context commands list I think)?

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Screenshoot list height bug

gasp! I've forgot the screenshot.

Comment 12

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

Ugh. Turns out the line I added:

this._popup.maxHeight = window.innerHeight * 0.75;

caused the bug Vivien spotted.

Yay! Turns out removing the line I added makes the bug go away _and_ we didn't need it anyway. The height works just fine. Resizing the window works fine now.

Comment 13

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

pushed:
http://hg.mozilla.org/mobile-browser/rev/3f2893b0c3cf

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 471040 [details] [diff] [review]
patch 2

Removing review request since the patch has already landed

Comment 15

[Aakash Desai [:aakashd]]

verified FIXED on builds:
Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:2.0b6pre) Gecko/20100907 Namoroka/4.0b6pre Fennec/2.0b1pre

and

Mozilla/5.0 (Android; Linux armv71; Nokia N900; en-US; rv:2.0b6pre) Gecko/20100907 Namoroka/4.0b6pre Fennec/2.0b1pre