1390445 - HTML <select> drop-downs are incorrectly positioned in WebExtensions add-on Options page

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P2)

Description

[dw-dev]

Attachment: Print Edit WE add-on - Options page - screenshot of incorrectly positioned select dropdown

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170608105825

Steps to reproduce:

Opened the Options page for several WebExtensions add-ons:

- Print Edit WE 19.0 (one of my add-ons)
- Tile Pages WE 1.0 (beta) (one of my add-ons) (first version not yet released)
- Language Tool Grammar Checker 1.0.13
- Page Translator 1.0.0

In each case, clicked on a <select> element in the add-on's Options page to open the drop-down menu.

Tested with both Firefox 56.0b2 and Nightly 57.0a1.



Actual results:

The <select> drop-downs in all of the add-on Options pages are displayed, but are INCORRECTLY POSITIONED.

The drop-downs are displayed some distance below the <select> elements, and quite often are at the bottom of the browser window.

I have attached a couple of screenshots showing this for 'Print Edit WE' and 'LanguageTool - Grammar and Style Checker'.



Expected results:

The <select> drop-downs in all of the add-on Options pages should have opened immediately below (or above) the <select> elements.

Comment 1

[dw-dev]

Attachment: LanuguageTool - Grammar and Style Checker add-on - Options page - screenshot of incorectly positioned select dropdown

Comment 2

[Alice0775 White]

setting extensions.webextensions.remote = false fixes the problem.

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

I've investigated this issue a bit and it seems that it is related to the value of `rect` object computed in the child process by the SelectContentHelper.jsm (and then sent to the main process to open the actual popup frame):

- http://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a4131452459241010701e0/toolkit/modules/SelectContentHelper.jsm#112

I've reproduced the issue locally and it seems the moving the window that contains the about:addons tab fixes the popup positioning (until the addon details page is reloaded).

I collected some logs from the code used to compute the screen position for the select popup frame in BrowserUtils.jsm [1] and it seems that the value of window.mozInnerScreenY is the one that changes, apparently the initial value is wrong and SelectContentHelper.jsm asks the parent process to show the popup at the wrong position, after moving the window on the screen the value seems to be recomputed and becomes the right one and the parent process is able to show the popup at the expected screen position).

[1]: http://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a4131452459241010701e0/toolkit/modules/BrowserUtils.jsm#227-228

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Follows some additional details:

The initial window.mozInnerScreenY value inside the options_ui remote browser is the one related to the screen position of the browser when it has been created and added to the document (which is likely to be out of the visible part of the addon details page), when the user scrolls the addon details page to make the options_ui visible, the window.mozInnerScreenY doesn't change accordingly and the select will be positioned where the select element was on the screen before the user has scrolled the "about:addons" page content.

Moving the entire window on the screen seems to force the window.mozInnerScreenY value to be updated to the correct one
(but it will become wrong again as soon as the scrolling position of the addon details page has been changed again).

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1390445 - Fix select popup positioning for oop extensions options_ui pages.

Review commit: https://reviewboard.mozilla.org/r/181854/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181854/

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8910404 [details]
Bug 1390445 - Fix select popup positioning for oop extensions options_ui pages.

https://reviewboard.mozilla.org/r/181854/#review187222

::: toolkit/mozapps/extensions/content/extensions.js:4166
(Diff revision 1)
> +    if (updateWindowPosTimeout) {
> +      clearTimeout(updateWindowPosTimeout);
> +      updateWindowPosTimeout = null;
> +    }

Let's just use a DeferredTask for this.

::: toolkit/mozapps/extensions/content/extensions.js:4174
(Diff revision 1)
> +    }
> +
> +    updateWindowPosTimeout = setTimeout(() => {
> +      updateWindowPosTimeout = null;
> +
> +      const browser = document.querySelector("browser#addon-options");

Please use `getElementById` instead. This is much more expensive.

::: toolkit/mozapps/extensions/content/extensions.js:4176
(Diff revision 1)
> +    updateWindowPosTimeout = setTimeout(() => {
> +      updateWindowPosTimeout = null;
> +
> +      const browser = document.querySelector("browser#addon-options");
> +
> +      if (browser && browser.getAttribute("remote") === "true") {

Nit: `browser.isRemoteBrowser`

::: toolkit/mozapps/extensions/content/extensions.js:4181
(Diff revision 1)
> +      if (browser && browser.getAttribute("remote") === "true") {
> +        // If an options_ui remote browser exists, dispatch a MozUpdateWindowPos
> +        // event, which is going to be processed by TabParent and it forces
> +        // window.mozInnerScreenX and window.mozInnerScreenY attributes
> +        // to be recomputed.
> +        window.dispatchEvent(new window.CustomEvent("MozUpdateWindowPos", {

This is going to cause us to update the position of every TabParent in the browser. We should probably just add a scriptable method to the FrameLoader binding to recompute its position.

Although, really, it would be nice if TabParent handled this itself.

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8910404 [details]
Bug 1390445 - Fix select popup positioning for oop extensions options_ui pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181854/diff/1-2/

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8910404 [details]
Bug 1390445 - Fix select popup positioning for oop extensions options_ui pages.

https://reviewboard.mozilla.org/r/181854/#review187222

> This is going to cause us to update the position of every TabParent in the browser. We should probably just add a scriptable method to the FrameLoader binding to recompute its position.
> 
> Although, really, it would be nice if TabParent handled this itself.

I'm investigating the other proposed solution ("let TabParent to handle this itself"),
but in the meantime I've updated the patch to use the first proposed solution ("a scriptable method added to the FrameLoader binding").

Comment 9

[dw-dev]

When I reported this bug, I forgot to mention that, as well as the drop-down menu appearing in the wrong position, it is also not possible to style the drop-down menu using CSS.

Specifically, it is not possible to change the font size of the text in the <option> elements.

The default font size is much too large if you have and add-on with a significant number of options.

Comment 10

[dw-dev]

Regarding Comment 9, I forgot to mention that changing the font size using CSS in the options page is possible with Firefox 55.

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to dw-dev from comment #10)
> Regarding Comment 9, I forgot to mention that changing the font size using
> CSS in the options page is possible with Firefox 55.

Hi dw-dev,
it sounds like an additional "oop extensions"-related issue (which has been enabled by default on Windows in Firefox 56), 
do you mind to create a separate bugzilla issue and CC me on it?

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8910404 [details]
Bug 1390445 - Fix select popup positioning for oop extensions options_ui pages.

https://reviewboard.mozilla.org/r/181854/#review188488

r=me with corrections, but the FrameLoader changes will also need review by a DOM peer.

::: dom/base/nsFrameLoader.cpp:3772
(Diff revision 2)
> +NS_IMETHODIMP
> +nsFrameLoader::RequestUpdatePosition()
> +{

No need for this. There aren't any existing native callers to worry about. Just fold it into the WebIDL version.

::: dom/base/nsFrameLoader.cpp:3775
(Diff revision 2)
> +  auto* tabParent = TabParent::GetFrom(GetRemoteBrowser());
> +
> +  if (tabParent) {

Nit: `if (auto tabParent = TabParent:...)`

::: dom/base/nsIFrameLoader.idl:161
(Diff revision 2)
>    /**
> +   * Force a remote browser to recompute its dimension and screen position.
> +   */
> +  void requestUpdatePosition();

No need for this.

::: toolkit/mozapps/extensions/content/extensions.js:4176
(Diff revision 2)
> +    if (updatePositionTask) {
> +      updatePositionTask.disarm();
> +    }

No need for this.

::: toolkit/mozapps/extensions/content/extensions.js:4180
(Diff revision 2)
> +    if (!getRemoteBrowser()) {
> +      return;
> +    }

No need for this either. Just arm the task. It can check for a remote browser when it gets dispatched. Much less overhead that way.

::: toolkit/mozapps/extensions/content/extensions.js:4184
(Diff revision 2)
> +    updatePositionTask = new DeferredTask(() => {
> +      const browser = getRemoteBrowser();
> +      if (!browser) {
> +        return;
> +      }
> +
> +      browser.frameLoader.requestUpdatePosition();
> +    }, UPDATE_POSITION_DELAY);

We need to create this once, and arm it for each scroll event, rather than creating a new one each time. You should probably just create it when you declare the `updatePositionTask` variable.

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8910404 [details]
Bug 1390445 - Fix select popup positioning for oop extensions options_ui pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181854/diff/2-3/

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8910404 [details]
Bug 1390445 - Fix select popup positioning for oop extensions options_ui pages.

https://reviewboard.mozilla.org/r/181854/#review188488

> We need to create this once, and arm it for each scroll event, rather than creating a new one each time. You should probably just create it when you declare the `updatePositionTask` variable.

ah, I see now, Thanks!

and sorry, it was the first time that I was using the DeferredTask and I'm pretty sure that I looked at some existent code that was using it wrong, this is way better.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8910404 [details]
Bug 1390445 - Fix select popup positioning for oop extensions options_ui pages.

https://reviewboard.mozilla.org/r/181854/#review190246

::: toolkit/mozapps/extensions/content/extensions.js:4171
(Diff revision 3)
> +    if (browser && browser.isRemoteBrowser) {
> +      browser.frameLoader.requestUpdatePosition();
> +    }
> +  }, UPDATE_POSITION_DELAY);
> +
> +  window.addEventListener("scroll", () => {

Is there any reason we can't do this here:
http://searchfox.org/mozilla-central/rev/f2b181af9497af258d96213f2f0cfccc08740a86/dom/ipc/TabParent.cpp#324

Then this could all be in C++. I guess the event listener wouldn't be on a delay, but hopefully this isn't too expensive.

I'm also going to send this to kats since this is really his area of expertise.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8910404 [details]
Bug 1390445 - Fix select popup positioning for oop extensions options_ui pages.

https://reviewboard.mozilla.org/r/181854/#review190578

It's not really clear to me why this affects OOP addons but not regular web content. I'm applying the patch now to investigate the code flow a bit more but if anybody can explain what is so special about the addon options page that would help.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8910404 [details]
Bug 1390445 - Fix select popup positioning for oop extensions options_ui pages.

https://reviewboard.mozilla.org/r/181854/#review190620

Ok, this seems reasonable enough for now. For posterity, the issue here is that there is a remote browser (<browser id="addon-options" ... remote="true" ...>) embedded inside a xul:scrollbox on the about:addons page. I don't think we have any other scenario where we have a remote browser embedded inside a scrolling thing. So what this patch is doing is picking up any scrolling on the about:addons page, and forcing the remote browser to update it's positioning information. That way it when the dropdown is spawned inside the remote browser it gets positioned in the right place.

I also verified that any attempts to scroll the about:addons page while the dropdown is shown hides the dropdown, so we don't have a problem with the positioning needing to be updated while the dropdown is visible.

With respect to billm's comment, if we put the code directly in TabParent.cpp I don't know if we'd be able to register the listener just on about:addons. Instead it would run on every TabParent which would cause a lot of unnecessary positioning updates. It might be possible to do if we do URL checks and so on but the approach in this patch also seems ok to me.

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #17)

Thanks a lot for reviewing this patch :kats

I confirm that your analysis from comment 17 is complete and right (my apologies for not being able to reply comment 16 before and provide these details, I just got back from pto).

I share the same concerns related to moving this into TabParent.cpp (which I did locally to compare the two approaches):
it doesn't seem that we have any other scenario where we have a scrollable area with a remote browser inside and so I was not completely convinced if it is really worth to move it to TabParent.cpp (especially if we decide to restrict the behavior to just "about:addons" from inside the TabParent.cpp, e.g. by not subscribing the event if the window root url is not about:addons).

Comment 19

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8910404 [details]
Bug 1390445 - Fix select popup positioning for oop extensions options_ui pages.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181854/diff/3-4/

Comment 20

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 5 changes to 5 files
remote: 
remote: WebIDL file dom/webidl/FrameLoader.webidl altered in changeset 1886de5bf8b4 without DOM peer review
remote: 
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: 
remote: Changes to WebIDL files in this repo require review from a DOM peer in the form of r=...
remote: This is to ensure that we behave responsibly with exposing new Web APIs. We appreciate your understanding..
remote: 
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.d_webidl hook failed
abort: push failed on remote

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8910404 [details]
Bug 1390445 - Fix select popup positioning for oop extensions options_ui pages.

https://reviewboard.mozilla.org/r/181854/#review193726

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/acdd6207bc52
Fix select popup positioning for oop extensions options_ui pages. r=billm,kats,kmag

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/acdd6207bc52

Comment 24

[CosminB]

Attachment: Animation.gif

This issue is verified as fixed on Firefox 58.0a1 (20171013100112) under Wind 7 64-bit and Ubuntu 16.04 32-bit.

The extensions that have <select> drop-down in the options page are displayed below the <select> elements.

Please see the attached video.

Comment 26

[Julien Cristau [:jcristau] (back August 18)]

Do we need to uplift this to 57?

Comment 27

[Luca Greco [:rpl] [:luca] [:lgreco]]

The patch related to this fix contains changes to an idl and its related cpp file, and we are not currently uplifting other fixes related to a WebExtension options_ui page running in oop mode inside the "about:addons" page (e.g. See Bug 1385548 Comment 33).

Unlike Bug 1385548 this bug doesn't prevent the options_ui page running in oop mode to work, it just fixes the select positioning (also, the select would be positioned correctly if the options_ui page is opened in a new tab).

For the above reasons I don't think that this issue meets the requirements for an uplift to 57.