Deal with what are currently popup widgets (e.g. for <select>) in widgetless content processes

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: cjones, Assigned: blassey)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b1+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

After bug 582057, navigating to a page that creates a <select> causes the content process to abort on NYI code.  We need to figure out how to deal with popups.

I think we have two approaches, roughly speaking.  One is to teach PuppetWidgets about popup widgets, and create popup PuppetWidgets.  This would entail sending the content process's "widget" hierarchy to the chrome process, because chrome would need to know the z-order of fake widgets.

The second is to add a second, non-widget implementation of the <select> frame (or something to that effect).  Then the <select> can have its own layer (or whatever), and we can reuse all the existing cross-process layer machinery and stuff will Just Work.

I personally lean towards the second option.  I'm afraid of PuppetWidgets turning into a full-fledged widget toolkit, rather than being simple stand-ins as they are now.  This also seems to me to fit into the general move towards tab-local UI, in the same spirit as tab-scoped dialog boxes.  However, I have no idea what code would break if <select>s didn't have widgets.  I also don't know what UX traditions govern <select>. 

The other factor is what the fennec frontend wants.  It appears to me that the current handling of <select>s is some relatively straightforward CSS, so that should work fine with either approach.

Thoughts?
In fennec, we stop the normal creation of a popup for the <select> and instead use our own, touch-friendly, UI to handle manipulating the selected item in a <select>.

This touch-friendly UI is created in the browser chrome, in the parent process. We have a fair amount of code that lives in the child process to send over the <option>s and other data needed to support the touch-friendly UI.

As long as we still have some way to skip creating a popup for the <select>, fennec should be fine. If the new mechanism was designed to support alternate UIs for the <select>, even better.
The reason we create a popup widget for <select>s is that we want a large select to be able to extend outside the browser window on desktop Firefox. We probably don't want to regress that.

Obviously for mobile this is not an issue. So for now, I suggest focusing on a solution that integrates cleanly with Fennec's select handling.

Beyond that, we probably should create a higher-level API that lets us send the options and optgroups over to the chrome process and have it implement the select functionality itself with the real window toolkit. Maybe we can use the Fennec API everywhere.

Note that we also have XUL menus doing similar things, although if we disable XUL menus in content processes it's probably not too bad.
OK, cool.  I'll let this bug lie for the time being.
Mark and I found out this evening that we still create popup widgets when the "ui.use_native_popup_windows" pref is flipped to false.  So we need something, probably a band-aid, in place for b1.
Blocks: 574512
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0b1+
Attachment #471605 - Flags: feedback?(roc)
(In reply to comment #4)
> Mark and I found out this evening that we still create popup widgets when the
> "ui.use_native_popup_windows" pref is flipped to false.

That's not the default value, is it? Why bother supporting this pref at all in E10S Fennec?
Comment on attachment 471605 [details] [diff] [review]
WIP patch, use native widget in content process

This works pretty well for fennec as far as I can tell, changing my feedback request to a review request.
Attachment #471605 - Flags: feedback?(roc) → review?(roc)
How about comment #6? I'm wondering why we need to support ui.use_native_popup_windows=false in E10S Fennec.
That pref doesn't mean what I thought it did.  AFAICT it's irrelevant to fennec.  It's a red herring, sorry to sidetrack this bug.
(In reply to comment #8)
> How about comment #6? I'm wondering why we need to support
> ui.use_native_popup_windows=false in E10S Fennec.

As I understand it, the speculation was that if the ui.use_native_popup_window pref was set to false then we'd avoid this code path entirely. That's not the case, so we need to fix this bug.
OK then, how does this patch work? How can we allocate a child popup widget if we don't have a real widget to make it a child of?

And why do we need to create a popup widget in the first place? Given comment #1, I thought Fennec never needed to do this.
(In reply to comment #11)
> OK then, how does this patch work? How can we allocate a child popup widget if
> we don't have a real widget to make it a child of?
> 
> And why do we need to create a popup widget in the first place? Given comment
> #1, I thought Fennec never needed to do this.

Fennec never needs to *show* a popup widget.  (AFAIK.)  For fennec, this bug encompasses the easiest, least-hacky way to keep existing <select>-related code intact in content processes, which don't have parent native widgets.

Re-reading comment 0, I think we have three options now
 (1) create native popup widgets but never use them: Brad's patch
 (2) create a hidden, not-parented PuppetWidget for popups, assert if it's ever shown
 (3) refactor the <select> code to not use popup widgets if they're not available, and instead do "something else"

Feedback appreciated :).  (1) and (2) are approximately equally easy, but pull in many unknown factors.
(In reply to comment #12)
>  (3) refactor the <select> code to not use popup widgets if they're not
> available, and instead do "something else"

We already have the "something else" code, it seems. It seems to me that hacking nsComboboxControlFrame/nsListControlFrame to not create a widget in a content process should be straightforward.
cjones: if we got a similar band-aid going with unparented puppet widgets, would that suit?
roc: yes!
Posted patch patch (obsolete) — Splinter Review
Assignee: nobody → blassey.bugs
Attachment #471605 - Attachment is obsolete: true
Attachment #473678 - Flags: review?(roc)
Attachment #471605 - Flags: review?(roc)
Attachment #473678 - Flags: review?(roc) → review-
Posted patch patchSplinter Review
Attachment #473678 - Attachment is obsolete: true
Attachment #473686 - Flags: review?(roc)
merged to m-c from cedar http://hg.mozilla.org/mozilla-central/rev/a796f404ede5
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.