1663366 - [Mac] Tab-modal print UI uses old dropdown menu appearance

Status: VERIFIED FIXED

(Toolkit :: Printing, defect, P3)

Description

[Sam Johnson]

Attachment: Print UI dropdown appearance

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:82.0) Gecko/20100101 Firefox/82.0

Steps to reproduce:

1. Open the new tab-modal print UI via File > Print
2. Click to open any of the dropdown menus in the print UI.
3. Notice the menus are using the old appearance on macOS (I believe the style was changed as part of e10s). The dropdown menus appear correct when included as part of webpage content.

Actual results:

The menus are using the old appearance on macOS (I believe the style was changed as part of e10s)
Note: I am aware the appearance of the dropdown button is intentionally different; I am specifically referencing the menu of options that drops down.

Expected results:

Dropdown menus in the print UI should appear as they do throughout the rest of Firefox UI.

Comment 1

[Sam Johnson]

Attachment: Dropdown appearance in webpage content

Comment 2

[:Gijs (he/him)]

I'm guessing this is courtesy of the fact that for select elements in child processes, we proxy the display of the options to the parent. We don't do this for the select element in the print "dialog" because it's not in a child process. Moving the entire print stuff into a child process would be annoying / going in the wrong direction.

Mike, could we somehow opt the select box into using the parent/child infrastructure we use for the child process select dropdowns? I assume the main alternative is using a XUL menulist for this item in the parent, which would be... unfortunate.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs (he/him) from comment #2)

Mike, could we somehow opt the select box into using the parent/child infrastructure we use for the child process select dropdowns? I assume the main alternative is using a XUL menulist for this item in the parent, which would be... unfortunate.

Hm... support for using the e10s variation of <select> for non-e10s tabs was added in bug 1300784... it looks like it was never preffed on (dom.select_popup_in_parent.enabled) by default (bug 1331725) due to keyboard focus and accessibility issues.

Testing it now, preffed on, it appears to be broken. It looks like the SelectChild actor isn't being instantiated when the event fires in the parent process.

Comment 4

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #3)

(In reply to :Gijs (he/him) from comment #2)

Mike, could we somehow opt the select box into using the parent/child infrastructure we use for the child process select dropdowns? I assume the main alternative is using a XUL menulist for this item in the parent, which would be... unfortunate.

Hm... support for using the e10s variation of <select> for non-e10s tabs was added in bug 1300784... it looks like it was never preffed on (dom.select_popup_in_parent.enabled) by default (bug 1331725) due to keyboard focus and accessibility issues.

From a casual look at the bug, it appears the a11y issues were resolved? In which case, is there anything left to do apart from fixing the actor instantiation? I think it might just need includeParent setting ?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

includeParent is, strangely, only for JSProcessActors. The one for JSWindowActors is includeChrome - not sure why.

At any rate, I think it's a bit more complicated. Yes, we should use includeChrome so that the actor can be instantiated for in-process <browser> elements - however, from experimentation, we'll then hit bug 1596852. This means that the print.js script will likely need to add its own event listeners for mozshowdropdown, mozshowdropdown-sourcetouch and mozhidedropdown and proxy them to a manually instantiated SelectChild, via something like:

    window
      .windowGlobalChild
      .getActor("Select")
      .handleEvent(e);

There's a further wrinkle - this code throws an exception: https://searchfox.org/mozilla-central/rev/8a0745cd346f0cfb89ae71690babbf7bff706113/toolkit/actors/SelectParent.jsm#709-711

because apparently from the print preview UI browser, it's browsingContext.top.embedderElement is... null? Not entirely sure why. We'd probably need someone from DOM to help answer that. I'm actually not sure we want browsingContext.top in this case - we probably want the <xul:browser> that hosts the UI and no further. And then, we need to make sure that <xul:browser> has its selectmenulist attribute set to "ContentSelectDropdown".

And then I think it'd work.

Comment 6

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #5)

There's a further wrinkle - this code throws an exception: https://searchfox.org/mozilla-central/rev/8a0745cd346f0cfb89ae71690babbf7bff706113/toolkit/actors/SelectParent.jsm#709-711

because apparently from the print preview UI browser, it's browsingContext.top.embedderElement is... null?

Yeah, this will be because printPreviewBrowser.browsingContext.top == window.browsingContext, ie it's the browsing context for the chrome window itself, which has no embedder element.

Not entirely sure why. We'd probably need someone from DOM to help answer that. I'm actually not sure we want browsingContext.top in this case - we probably want the <xul:browser> that hosts the UI and no further.

This sounds right.

And then, we need to make sure that <xul:browser> has its selectmenulist attribute set to "ContentSelectDropdown".

And then I think it'd work.

OK, thanks for digging into this!

Comment 7

[Blake Winton (:bwinton) (:☕️)]

(Moving bugs to 86, part 1.)

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Moving things to 88, cause we're mostly on Proton these days…

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1663366 - Make content select work on the parent process (and opt-in the print settings dialog into it). r=mconley

This should do nothing until bug 1744009 lands. It would also need
bug 1596852 to be usable, but no reason to not land it now.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8203040487de
Make content select work on the parent process (and opt-in the print settings dialog into it). r=mconley

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/8203040487de

Comment 12

[Gabi Cheta [:Gabi] Desktop QA]

Verified as fixed with macOS 10.12.6 on Fx 98.0a1 and Fx 97.0b9.