<select> dropdowns don't work in OOP popup browsers

RESOLVED FIXED in mozilla56
(NeedInfo from)

Status

()

Toolkit
WebExtensions: Frontend
P1
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: streetwolf, Assigned: kmag, NeedInfo)

Tracking

(Blocks: 1 bug)

56 Branch
mozilla56
x86_64
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 affected)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments)

(Reporter)

Description

5 months ago
Created attachment 8884673 [details]
Screenshot of Zoom Page WE menu.

Bug 1357486 has partially broken at least one WE add-on I use.  It is called Zoom Page WE. The drop-down lists on the main menu to change font size and zoom level do not display. The screen shot I provided shows the two areas, when clicked on, should produce a drop-down menu of font sizes and zoom levels.  Setting pref 'extensions.webextensions.remote = false' makes the add-on work correctly.
(Reporter)

Updated

5 months ago
Blocks: 1357486
Version: unspecified → 56 Branch
(Reporter)

Comment 1

5 months ago
Found another WE add-on of mine that doesn't produce a drop down list when I click on the down arrow in it's options in the Extensions list.  The add-on is called AutoplayStopper 0.9.8.
(Reporter)

Updated

5 months ago
Summary: Webextensions add-on 'Zoom Page WE' partially broken by Bug 1357486. → Some Webextensions add-ons partially broken by Bug 1357486.

Comment 2

5 months ago
Search Site WE popup don't shows if extensions.webextensions.remote=true. Maybe it is an same problem?
(Reporter)

Comment 3

5 months ago
(In reply to YF (Yang) from comment #2)
> Search Site WE popup don't shows if extensions.webextensions.remote=true.
> Maybe it is an same problem?

I'm sure it is as it is by the same developer
Assignee: nobody → kmaglione+bmo
Blocks: 1190679
Status: UNCONFIRMED → NEW
Component: WebExtensions: General → WebExtensions: Frontend
Ever confirmed: true
Summary: Some Webextensions add-ons partially broken by Bug 1357486. → <select> dropdowns don't work in OOP popup browsers
Add ForecastFox 4.1 to the list of broken WE extensions with drop-list.
Comment hidden (me-too)
(Reporter)

Comment 6

5 months ago
Folks, no need to spam this report.  It appears all WE add-ons with dropdown menus are broken.  Adding your particular add-on just adds to the clutter.

btw... what is the priority for this report?
Yes, as Gary said, no need to me-too this bug.

And I'd say this is P1 as well, considering it's blocking bug 1357486.
Priority: -- → P1
(Reporter)

Comment 8

5 months ago
Is the patch to fix this issue going to be released soon?  I ask as a Nighty Tester who needs some of my broken add-ons to be fully functional.  No spam intended.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

5 months ago
mozreview-review
Comment on attachment 8886417 [details]
Bug 1379508: Part 1 - Apply the correct client offsets to remote frameloaders in popup widgets.

https://reviewboard.mozilla.org/r/157216/#review162504

::: dom/ipc/TabParent.cpp:2005
(Diff revision 1)
> +  return (docWidget->GetClientOffset() -
> +          nsLayoutUtils::WidgetToWidgetOffset(docWidget, widget));

minor nit: I would find this slightly easier to read if you swapped the order of arguments to WidgetToWidgetOffset and changed the minus to a plus. That way WidgetToWidgetOffset returns a (likely positive) distance from widget to docWidget, which you are then adding to docWidget's existing client offset.
Attachment #8886417 - Flags: review?(bugmail) → review+

Comment 13

5 months ago
mozreview-review
Comment on attachment 8886418 [details]
Bug 1379508: Part 2 - Ignore transforms when calculating child process offsets.

https://reviewboard.mozilla.org/r/157218/#review162508

I'll give this an r+ if it's green on try, but I'll be honest - my understanding of this is pretty shaky. I don't know if anybody fully understands all of this though. smaug or jimm might be good bets if you want a second review.

::: dom/ipc/TabParent.cpp:1989
(Diff revision 1)
> -  nsPoint pt = nsLayoutUtils::GetEventCoordinatesRelativeTo(widget,
> -                                                            LayoutDeviceIntPoint(0, 0),
> -                                                            targetFrame);
>  
> -  return LayoutDeviceIntPoint::FromAppUnitsToNearest(
> -           pt, targetFrame->PresContext()->AppUnitsPerDevPixel());
> +  nsPresContext* presContext = targetFrame->PresContext();
> +  nsIFrame* rootFrame = presContext->PresShell()->FrameManager()->GetRootFrame();

can rootFrame be null here? Usually stuff returned by "Get" functions (GetRootFrame() in this case) is allowed to be null. If so you might need a guard.
Attachment #8886418 - Flags: review?(bugmail) → review+
(Assignee)

Comment 14

5 months ago
mozreview-review-reply
Comment on attachment 8886418 [details]
Bug 1379508: Part 2 - Ignore transforms when calculating child process offsets.

https://reviewboard.mozilla.org/r/157218/#review162508

> can rootFrame be null here? Usually stuff returned by "Get" functions (GetRootFrame() in this case) is allowed to be null. If so you might need a guard.

It shouldn't be possible for it to be null in this case, since already have a frame in the same presContext, and there needs to be a root frame for it to inherit from. I'm not opposed to adding a guard, though.
Attachment #8886418 - Flags: review?(jmathies)

Comment 15

5 months ago
mozreview-review
Comment on attachment 8886419 [details]
Bug 1379508: Part 3 - Support <select> popups in OOP popup browsers.

https://reviewboard.mozilla.org/r/157220/#review162660

::: browser/components/extensions/test/browser/browser-remote.ini:12
(Diff revision 1)
>  # whether we're running from that directory from head.js
>  install-to-subdir = test-oop-extensions
>  tags = webextensions remote-webextensions
>  skip-if = !e10s
>  
> +[browser_ext_popup_select.js]

What's the reason for this only running in OOP mode?

::: browser/components/extensions/test/browser/browser_ext_popup_select.js:8
(Diff revision 1)
> +"use strict";
> +
> +add_task(async function testPopupSelectPopup() {
> +  let extension = ExtensionTestUtils.loadExtension({
> +    background() {
> +      browser.tabs.query({active: true, currentWindow: true}, tabs => {

nit: async

::: browser/components/extensions/test/browser/browser_ext_popup_select.js:63
(Diff revision 1)
> +    });
> +
> +    let {boxObject} = browser;
> +    let popupRect = selectPopup.getOuterScreenRect();
> +
> +    is(Math.floor(boxObject.screenX + elemRect.left), popupRect.left,

Are all of these coordinates affected by `devicePixelRatio` in the same way?

::: browser/components/extensions/test/browser/browser_ext_popup_select.js:73
(Diff revision 1)
> +    clickBrowserAction(extension);
> +    let browser = await awaitExtensionPanel(extension);

This always looks like a race condition to me :/
Attachment #8886419 - Flags: review?(tomica) → review+
(Assignee)

Comment 16

5 months ago
mozreview-review-reply
Comment on attachment 8886419 [details]
Bug 1379508: Part 3 - Support <select> popups in OOP popup browsers.

https://reviewboard.mozilla.org/r/157220/#review162660

> What's the reason for this only running in OOP mode?

In-process select popups work differently. They don't use a XUL popup.

> Are all of these coordinates affected by `devicePixelRatio` in the same way?

They all use the same coordinate system, yes.

> This always looks like a race condition to me :/

It's not. The WebExtPopupLoaded event is always dispatched asynchronously.
https://hg.mozilla.org/integration/mozilla-inbound/rev/61913ebf8097a519e90c16c3a299858a462d10ab
Bug 1379508: Part 1 - Apply the correct client offsets to remote frameloaders in popup widgets. r=kats

https://hg.mozilla.org/integration/mozilla-inbound/rev/58e9c32d5e53f119a3a909b836496efaf073298b
Bug 1379508: Part 2 - Ignore transforms when calculating child process offsets. r=kats

https://hg.mozilla.org/integration/mozilla-inbound/rev/709cedd7f1bafd43c0be6e6c642d3d497e65c108
Bug 1379508: Part 3 - Support <select> popups in OOP popup browsers. r=zombie
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)
> I'll give this an r+ if it's green on try, but I'll be honest - my
> understanding of this is pretty shaky. I don't know if anybody fully
> understands all of this though. smaug or jimm might be good bets if you want
> a second review.

Landing now with just your review, since it fixes problems for nightly users, but leaving the review flag for jimm in case it needs a follow-up.
https://hg.mozilla.org/integration/mozilla-inbound/rev/41b75de6ceb270bf87f1b965052d37cdc64f511a
Bug 1379508: Follow-up: Disable select popup test on OS-X for alignment issues.
(Reporter)

Comment 20

5 months ago
Created attachment 8886826 [details]
Zoom Page WE after this patch

Getting close but no cigar as they say.  As seen in the screenshot there is a small popup 'above' the zoom percentage.  Same thing happens to the font size.

The other add-on I have does seem to work however, Textarea Cache Lite.  It doesn't having the same sort of dropdown menu but it does work now.
(Reporter)

Comment 21

5 months ago
Another WE add-on, AutoplayStopper, doesn't display it's dropdown lists at all when the down arrow is clicked.
(In reply to Gary [:streetwolf] from comment #20)
> Getting close but no cigar as they say.  As seen in the screenshot there is
> a small popup 'above' the zoom percentage.  Same thing happens to the font
> size.

That's expected. Select popups for remote frames are constrained to the viewport of the page. Feel free to file a separate bug if you think that should be changed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/43676eab0672b3e86b33dcad9d672ac07e4c562f
Bug 1379508: Follow-up: Disable select popup test on Linux for frequent intermittent failures.
(Reporter)

Comment 24

5 months ago
Kris.

It also appears your fix screws up resizing windows for the WE add-ons I use. One window just disappears when I try to drag the resize icon on the bottom right.  Another one, on it's Extensions page, makes the window one line. Is this expected behavior too?
https://hg.mozilla.org/mozilla-central/rev/61913ebf8097
https://hg.mozilla.org/mozilla-central/rev/58e9c32d5e53
https://hg.mozilla.org/mozilla-central/rev/709cedd7f1ba
https://hg.mozilla.org/mozilla-central/rev/41b75de6ceb2
https://hg.mozilla.org/mozilla-central/rev/43676eab0672
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Just tested the latest m-c win32 build with this patch and ForecastFox is still not showing drop-down.

Comment 27

5 months ago
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #26)
> Just tested the latest m-c win32 build with this patch and ForecastFox is
> still not showing drop-down.

You should try the tomorrow Nightly (m-c). Check the push date on about:buildconfig and comment 25.
(Reporter)

Comment 28

5 months ago
I'm on inbound with the patch and as I said in a previous comment things are not as they were.  The dropdown looks different in some instances or they still don't work.
(Reporter)

Updated

5 months ago
Depends on: 1381337
Duplicate of this bug: 1381344
Duplicate of this bug: 1380294

Comment 31

5 months ago
mozreview-review
Comment on attachment 8886418 [details]
Bug 1379508: Part 2 - Ignore transforms when calculating child process offsets.

https://reviewboard.mozilla.org/r/157218/#review163088
Attachment #8886418 - Flags: review?(jmathies) → review+

Comment 32

5 months ago
mozreview-review-reply
Comment on attachment 8886418 [details]
Bug 1379508: Part 2 - Ignore transforms when calculating child process offsets.

https://reviewboard.mozilla.org/r/157218/#review162508

I know very little about the transforms stuff tbh. I know about widget offsets but that's not relevant here. sorry.
Setting the status flag back to affected and reopening the bug, since there were reports of the issue still existing and there's now a new patch to land.
Status: RESOLVED → REOPENED
status-firefox56: fixed → affected
Resolution: FIXED → ---
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #33)
> Setting the status flag back to affected and reopening the bug, since there
> were reports of the issue still existing and there's now a new patch to land.

The reports were of other issues. This issue is fixed. And the follow-up has landed, in any case.
Status: REOPENED → RESOLVED
Last Resolved: 5 months ago5 months ago
Resolution: --- → FIXED

Comment 35

5 months ago
Created attachment 8888776 [details]
TooltipIssue.gif

Hello guys, 

It seems I encountered the Bug 1380294 on Firefox 56.0a1 (2017-07-20) under Win7 64-bit , please have a look at the attached video.
Flags: needinfo?(kmaglione+bmo)

Comment 36

5 months ago
Created attachment 8889494 [details]
Videos.zip

I tested some WebExtensions like: Forecastfox (fix version), Search Site WE, Zoom Page WE on Firefox 56.0a1(2017-07-24) on Wind 7 64-bit, Wind 10 32-bit, Wind 10 64-bit, Ubuntu 16.04 32-bit and Mac OS X 10.12.3.

Forecastfox (fix version) is not working on Wind 10 32-bit, Wind 10 64-bit, Ubuntu 16.04 32-bit and Mac OS X 10.12.3

At some point the extension will crash on Wind 10 32-bit 
https://crash-stats.mozilla.com/report/index/d780985d-3741-4d61-9c4d-13ceb0170724

Zoom Page WE, does not display the selected value correctly in the drop-down list after reset.

Do you have any opinion about these issues Kris?

Please see the attached videos.

Comment 37

4 months ago
(In reply to Kris Maglione [:kmag] from comment #22)
> (In reply to Gary [:streetwolf] from comment #20)
> > Getting close but no cigar as they say.  As seen in the screenshot there is
> > a small popup 'above' the zoom percentage.  Same thing happens to the font
> > size.
> 
> That's expected. Select popups for remote frames are constrained to the
> viewport of the page. Feel free to file a separate bug if you think that
> should be changed.

Kris,

I'm fairly sure that this fix does not work correctly and has introduced new problems.

Specifically, I am experiencing problems with the select drop-downs in two of my WebExtensions add-ons: Zoom Page WE and Tile Pages WE.

In the case of Zoom Page WE, the select element is inside the browserAction popup. With Firefox 54, the select drop-down is allowed to extend outside the browserAction popup window. With Nightly 56, I understand that the select drop-down should be constrained to the browserAction popup window. However, the select drop-down is NOT displayed at all, and even if I make the browserAction popup window big enough to contain the entire select drop-down, it still is NOT displayed.

In the case of Tile Pages WE, the select elements are inside the add-on Options window. With Firefox 54, the select drop-downs are displayed correctly.  With Nightly 56, the select drop-downs are NOT displayed, even though the add-on Options window is definitely big enough to contain the select drop-downs.

I cannot believe that this is the new expected behaviour.

Do I need to raise a new bug?

Comment 38

4 months ago
Kris,

Separately to the bugs reported in Comment 37, there one question I forgot to ask ... 

Is there a technical reason why select drop-downs (and popups in general) are not allowed to extend outside the parent page/window?  Or is it a user interface design decision?

Either way, is there any chance of getting this decision changed?

Comment 39

4 months ago
I've looked through a few more WebExtenions add-ons and very quickly found two more add-ons where the select drop-downs do not display in the add-ons Options window: 'LanguageTool Grammar Checker' and 'Page Translator'.
Duplicate of this bug: 1387633
You need to log in before you can comment on or make changes to this bug.