Closed Bug 586009 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(fennec2.0b1+)

VERIFIED FIXED
Tracking Status
fennec 2.0b1+ ---

People

(Reporter: aakashd, Assigned: wesj)

Details

Attachments

(2 files, 3 obsolete files)

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.
tracking-fennec: --- → ?
This also happens when context clicking on a nightly build ftp latest-mobile-trunk
tracking-fennec: ? → 2.0b1+
Assignee: nobody → wjohnston
Attached patch Mystery fix (obsolete) — Splinter Review
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.
Attached patch Removing sizing code (obsolete) — Splinter Review
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?
Attachment #468827 - Attachment is obsolete: true
Attachment #470105 - Flags: review?(mark.finkle)
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 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. :(
Attachment #470105 - Flags: review?(mark.finkle) → review?
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
Attachment #470105 - Flags: feedback?(21) → feedback-
Attached patch Just handle maxWidth (obsolete) — Splinter Review
I swear there has to be a CSS only method of doing this, but this works under every test case I can find. Asking
Attachment #470105 - Attachment is obsolete: true
Attachment #470950 - Flags: review?(mark.finkle)
Attachment #470105 - Flags: review?
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.
Attachment #470950 - Flags: review?(mark.finkle) → review+
Attached patch patch 2Splinter Review
Updated Wes' patch and want Vivien to review too.
Attachment #470950 - Attachment is obsolete: true
Attachment #471040 - Flags: review?(21)
(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)?
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.
pushed:
http://hg.mozilla.org/mobile-browser/rev/3f2893b0c3cf
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 471040 [details] [diff] [review]
patch 2

Removing review request since the patch has already landed
Attachment #471040 - Flags: review?(21)
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.