Fix #256: Change popup width in overflow menu #271

biancadanforth · 2018-11-15T21:35:35Z

With CustomizableUI.jsm (via the experimental browser.customizableUI API added in #157), it is possible to detect when the browserAction toolbar button is and is not in the Overflow menu.

Because the overflow menu width varies by OS (for example, it is 377px wide on my Mac 10.13 and 425px wide on a Linux 14.04 (see screenshots below for confirmation)), this patch will detect whether or not the toolbar button is in the Overflow menu and adjust the width accordingly.

This fix has the added benefit of allowing us to constrain the non-Overflow menu popup width (which is hopefully the vast majority use case) to the UX spec of 320px where previously we had set it to a max-width of 400px to try to accommodate Overflow menu issues.

Verified issue on Linux 14.04 (v11.0.0 prior to this patch!) in Firefox Nightly 65 and Release 63 shows that the popup width exceeds our CSS specified max-width of 400px

biancadanforth · 2018-11-15T21:36:55Z

@Osmose Ready for your review!

I confirmed that the issue was that the overflow menu on Linux exceeds 400px. It was 425px, so our previous max-width of 400px was too small.

Osmose

The fix doesn't seem to work for me locally on Nightly. The overflow class isn't being applied, presumably because the API call isn't returning true when the icon is in the overflow menu.

Osmose · 2018-11-16T00:56:48Z

src/experiment_apis/customizableUI/api.js

@@ -5,6 +5,7 @@

 this.customizableUI = class extends ExtensionAPI {
  getAPI(context) {
+    const {Services} = ChromeUtils.import('resource://gre/modules/Services.jsm');


Do we not need to pass an empty object as the second argument like the other import?

Nope; I will remove the second argument from the other one. Good catch.

Osmose · 2018-11-16T00:57:24Z

src/experiment_apis/customizableUI/api.js

@@ -5,6 +5,7 @@

 this.customizableUI = class extends ExtensionAPI {
  getAPI(context) {
+    const {Services} = ChromeUtils.import('resource://gre/modules/Services.jsm');
    ChromeUtils.import('resource://gre/modules/ExtensionCommon.jsm');
    const {EventManager} = ExtensionCommon;


Not part of this PR, but this seems weirdly formatted compared to the other imports here.

Osmose · 2018-11-16T00:59:57Z

src/experiment_apis/customizableUI/schema.json

+      {
+        "name": "isWidgetInOverflow",
+        "type": "function",
+        "description": "Returns a boolean for whether or not the provided widget in the provided window is in the overflow menu or not.",


This can be more succinct if we avoid specifying that arguments are provided to the function:

"Determine whether or not a widget is in the overflow menu."

biancadanforth · 2018-11-16T20:45:04Z

@Osmose Ready for another look!

I had to add another check for the overflow menu in the case of pinning/unpinning (a "fixed" overflow menu), as it was only working on window resize (an "unfixed" overflow menu) which are apparently in two different (though proximate) areas of the chrome per jaws.

Edit: Even if this gets your approval, we probably don't want to merge until after #274 lands, I'm guessing, as a precaution?

Osmose

Works great, nice job!

Osmose · 2018-11-19T22:38:07Z

src/browser_action/index.jsx

@@ -20,6 +20,12 @@ if (extractedProductJSON) {
  appProps.extractedProduct = JSON.parse(extractedProductJSON);
 }

+(async function checkOverflow() {
+  if (await browser.customizableUI.isWidgetInOverflow('shopping-testpilot_mozilla_org-browser-action')) {
+    document.getElementById('browser-action-app').classList.add('overflow');


Hmmm, maybe we could put this into one of the React components, like in the state for BrowserActionApp? Not sure if that adds much to this.

I tried to do this just now, but <BrowserActionApp /> currently returns a <React.Fragment>, which doesn't accept a class name. Wrapping the whole thing in a <div> instead and adding the class to that is possible, but then the CSS also needs another layer instead of applying the overflow class directly to #browser-action-app:

#browser-action-app > div { width: 320px; } #browser-action-app > div.overflow { width: 100%; }

Also with the new Study Invitation screen (#274 ), I'd need that wrapper div in two places in BrowserActionApp instead of just one.

I also tried putting the class name directly on <BrowserActionApp /> in index.jsx, but ReactDOM.render didn't like the await keyword needed for browser.customizableUI.isWidgetInOverflow.

I think for simplicity, I'll just leave this as is for now. Perhaps we can refactor this later if we ever implement the view router pattern you suggested here... The options I can think of/try right now seem more confusing and more prone to bugs.

Osmose · 2018-11-19T22:40:30Z

src/experiment_apis/customizableUI/api.js

+          // First check is for the non-fixed overflow menu (e.g. widget moved by resizing window)
+          // Second is for fixed overflow menu (e.g. widget moved by (un)pinning button to overflow)
+          return (CustomizableUI.getWidget(widgetId).forWindow(browserWindow).overflowed
+            || area === 'widget-overflow-fixed-list');


This becomes slightly more readable IMO if we format this like this:

return ( CustomizableUI.getWidget(widgetId).forWindow(browserWindow).overflowed || area === 'widget-overflow-fixed-list' );

With CustomizableUI.jsm (via the experimental `browser.customizableUI` API added in mozilla#157), it is possible to detect when the browserAction toolbar button is and is not in the Overflow menu. Because the overflow menu width varies by OS (for example, it is 377px wide on my Mac 10.13 and 425px wide on a Linux 14.04), this patch will detect whether or not the toolbar button is in the Overflow menu and adjust the width accordingly. This fix has the added benefit of allowing us to constrain the non-Overflow menu popup width (which is hopefully the vast majority use case) to the UX spec of 320px where previously we had set it to a max-width of 400px to try to accomodate Overflow menu issues.

…w menu Detect toolbar button in fixed overflow menu: - Add a call to `getPlacementOfWidget` - This check covers the case when the toolbar button is explicitly pinned/unpinned to the Overflow menu by the user. Context: - The original patch only detected the toolbar button in the non-fixed overflow menu, which only covered the case when the toolbar button was moved into and out of the overflow menu by a window resize. - Per jaws, the fixed versus non-fixed Overflow menus live in two different areas of the browser chrome, where "the non-fixed overflow panel is the area that is basically an extension of the navigation-toolbar." - With both checks together, we cover both of the cases in which the toolbar button can be moved into and out of the Overflow menu.

biancadanforth requested a review from Osmose November 15, 2018 21:35

Osmose suggested changes Nov 16, 2018

View reviewed changes

biancadanforth force-pushed the 256-overflow-width branch from 29e47b1 to 37f509d Compare November 16, 2018 20:42

biancadanforth requested a review from Osmose November 16, 2018 20:43

Osmose approved these changes Nov 19, 2018

View reviewed changes

biancadanforth added 2 commits November 20, 2018 16:25

biancadanforth force-pushed the 256-overflow-width branch from 37f509d to b6c508a Compare November 21, 2018 00:29

Incorporate Osmose's feedback

c60594f

biancadanforth merged commit 5867e5e into mozilla:master Nov 21, 2018

biancadanforth deleted the 256-overflow-width branch November 21, 2018 01:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #256: Change popup width in overflow menu #271

Fix #256: Change popup width in overflow menu #271

biancadanforth commented Nov 15, 2018 •

edited

Loading

biancadanforth commented Nov 15, 2018

Osmose left a comment

Osmose Nov 16, 2018

biancadanforth Nov 16, 2018

Osmose Nov 16, 2018

Osmose Nov 16, 2018

biancadanforth commented Nov 16, 2018 •

edited

Loading

Osmose left a comment

Osmose Nov 19, 2018

biancadanforth Nov 21, 2018

Osmose Nov 19, 2018

Fix #256: Change popup width in overflow menu #271

Fix #256: Change popup width in overflow menu #271

Conversation

biancadanforth commented Nov 15, 2018 • edited Loading

biancadanforth commented Nov 15, 2018

Osmose left a comment

Choose a reason for hiding this comment

Osmose Nov 16, 2018

Choose a reason for hiding this comment

biancadanforth Nov 16, 2018

Choose a reason for hiding this comment

Osmose Nov 16, 2018

Choose a reason for hiding this comment

Osmose Nov 16, 2018

Choose a reason for hiding this comment

biancadanforth commented Nov 16, 2018 • edited Loading

Osmose left a comment

Choose a reason for hiding this comment

Osmose Nov 19, 2018

Choose a reason for hiding this comment

biancadanforth Nov 21, 2018

Choose a reason for hiding this comment

Osmose Nov 19, 2018

Choose a reason for hiding this comment

biancadanforth commented Nov 15, 2018 •

edited

Loading

biancadanforth commented Nov 16, 2018 •

edited

Loading