Affected builds: - Latest Aurora 39.0a2 - Latest Nightly 40.0a1 Affected OS`s: - Windows 7 64-bit - Windows 8.1 64-bit - Ubuntu 14.04 32-bit - Mac OS X 10.9.5 STR: 1. Start Firefox 2. Click Hello icon 3. Start a Conversation 4. Click 'Share Link' 5. Click 'Add a service' 6. Add more then 6-7 services Expected results: Make a scrollbar to display more services. Actual results: List with social services not propperly displayed. Notes: - Screenshot with the issue attached.

Comment on attachment 8594698 [details] [diff] [review] Patch v1: show a scrollbar is the social share dropdown list Review of attachment 8594698 [details] [diff] [review]: ----------------------------------------------------------------- Is it possible to avoid the horizontal scrollbar that I get with this patch?

This should fix it for you... can you check that, please?

Comment on attachment 8594777 [details] [diff] [review] Patch v2: show a scrollbar is the social share dropdown list I tried looking at this again today, in preparation for debugging with you, however it seems that we've now adjusted the vertical position of the share link button within the conversation window. So we either need to fix that, or re-adjust the patch and then try again.

This patch is significantly more complicated than the previous one, working out the following things: - Since our dropdowns are constrained not only to the dimensions of the document, but even to specific areas of a document, I added the ability to specify an element to be the source of the width and height constraints for the dropdown. - We now know when the dropdown is in overflow mode, so we can add a className to style it appropriately. - Since the scrollbar is only impacting the layout on OSes other than OSX, I added a class to the document body that depicts which OS is in use. - When the dropdown is overflowing (i.e. showing a vertical scrollbar), whilst not on OSX, we compensate for the scrollbar width - 15px - by adding padding to the vertical end of the dropdown.

One other important part of the patch I forgot to note: - I moved the conversation action buttons down to be positioned just above the context display area, so that it's always positioned at the bottom-most position. This is needed to make as much room for the shareplane dropdown as possible. Related is the removal of the `order: x` style rules, because for the flex model in use here we can rely on the element order in the DOM completely.

I've just been trying this on windows and noticed a couple of things: - when you first select the share menu after opening the window, it appears in the top-left corner and there it moves to the right place. I'm running on a VM, but given there's slow machines out there, I think it would be good to see if we avoid that somehow. - once you get the scrollbar in play, then the final 'e' of "Add a Service" overlaps the border of the hightly box slightly.

Comment on attachment 8602732 [details] [diff] [review] Patch v4: show a scrollbar in the social share dropdown list Review of attachment 8602732 [details] [diff] [review]: ----------------------------------------------------------------- I don't think we're far off, but as per the previous comment, I think there's a few thing we could just get a little better. ::: browser/components/loop/content/shared/js/mixins.js @@ +127,4 @@ > var menuNodeRect = menu.getBoundingClientRect(); > + // If the menu dimensions are constrained to a bounding element, instead of > + // the document body, find that element. > + if (this.state.boundingBoxSelector) { I think it'd be nicer if we could pass this in via the mixin definition, i.e. mixins: [DropdownMixin(".room-invitation-overlay")] that way it doesn't need to be stored in the state, and is clearer about what's happening and the expectations. Also, whatever we end up with needs adding to the jsdoc for this mixin. @@ +176,2 @@ > overflowY = true; > + } nit: trailing whitespace ::: browser/components/loop/content/shared/js/utils.js @@ +276,5 @@ > + } else if (os.indexOf("win") > -1) { > + platform = "win"; > + } > + return platform; > + } eslint says there's a missing semi-colon here.

Updated for review comments. I must say, this turned out to be quite awesome! Thanks for raising the bar here ;-) Next patch will be one without whitespace changes. For your convenience.

Comment on attachment 8603234 [details] [diff] [review] Patch v5: show a scrollbar in the social share dropdown list Review of attachment 8603234 [details] [diff] [review]: ----------------------------------------------------------------- That looks much better. r=Standard8

Verified this on latest Nightly across platforms and encountered issue bug 1163513. All new issues will be handled in their own bugs. Marking this as verified fixed.