1108761 - Add class to let menulist > menupopup stylings apply to other popups

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

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

In XUL, we have styling and binding rules so that a menupopup under a menulist element behaves differently than when the menupopup is on its own.

On Windows, at least, a menupopup on its own looks more like a native context menu. When the menupopup is under a menulist, however, it looks more like an HTML <select> form dropdown.

In bug 1053981, we grappled with how to accomplish displaying the <select> dropdown popup in the parent process when opened from a remote tab. Our solution ended up being us applying the "chrome://global/content/bindings/popup.xml#popup-scrollbars" to our select popup.

Styling-wise, we can probably go further. I suggest that we look at trying to modify the styling and binding rules in toolkit such that anything that applies to menulist > menupopup [ > stuff] also applies for items that have a class - I suggest "menulist-menupopup".

I have an attachment where I got this started, but it's currently Windows-only. We're also going to have to do some styling for <optgroup>, and figure out what the hell we're going to do for content CSS that attempts to style <option> and <optgroup> elements... though maybe that's a separate bug. ¯\_(ツ)_/¯

Comment 1

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

Attachment: WIP patch (Windows-only at the moment)

Comment 2

[Allison Naaktgeboren :ally]

bug 1091592 about improving stylings should be considered whichever way desktop decides to go forward.

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8533326 [details] [diff] [review]
WIP patch (Windows-only at the moment)

Rather than doubling all the selectors, I think we should add the class to menupopups that are children to menulists. The best place to do this is probably the popupshowing handler in popup.xml.

Comment 5

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

Attachment: WIP: Add class to let menulist > menupopup stylings apply to other popups. r=?

Comment 6

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

Attachment: WIP patch on Windows 7

I haven't updated my patch to have my Windows changes in yet, but this is what it's looking like on my Windows box so far. Looking a bit better.

Comment 7

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

Attachment: WIP: Add class to let menulist > menupopup stylings apply to other popups. r=?

Comment 8

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

Attachment: WIP: Add class to let menulist > menupopup stylings apply to other popups. r=?

Comment 9

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

Attachment: Add class to let menulist > menupopup stylings apply to other popups. r=?

Comment 10

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

Comment on attachment 8537301 [details] [diff] [review]
Add class to let menulist > menupopup stylings apply to other popups. r=?

Something along these lines?

Comment 11

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

Gentle review ping.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8537301 [details] [diff] [review]
Add class to let menulist > menupopup stylings apply to other popups. r=?

> /* ::::: checkbox menuitem ::::: */
> 
>-menuitem[checked="true"] {
>+:not(.menulist-menupopup) > menuitem[checked="true"] {
>   -moz-appearance: checkmenuitem !important;
> }
> 
>-menuitem[type="checkbox"] {
>+:not(.menulist-menupopup) > menuitem[type="checkbox"] {
>   -moz-appearance: checkmenuitem !important;
> }
> 
> /* ::::: radio menuitem ::::: */
> 
>-menuitem[type="radio"] {
>+:not(.menulist-menupopup) > menuitem[type="radio"] {
>   -moz-appearance: radiomenuitem !important;
> }

Why these changes?

>+.menulist-menupopup > menuitem[_moz-menuactive="true"],
> menu[_moz-menuactive="true"],
> menuitem[_moz-menuactive="true"] {
>   color: -moz-mac-menutextselect;
>   background-color: Highlight;
> }

Why this change?

Comment 13

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

Attachment: Alternative solution

Comment 14

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

Comment on attachment 8544204 [details] [diff] [review]
Alternative solution

Perhaps something like this is simpler - I introduce a headless variation of menulist, and use that for the ContentSelectDropdown.

There's still some clean-up here, but I think this might be a simpler solution. I still need to test this on OSX and Windows, but this works for me on Linux.

What do you think, Dao?

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8544204 [details] [diff] [review]
Alternative solution

>+menulist[headless="true"] {
>+  -moz-binding: url("chrome://global/content/bindings/menulist.xml#menulist-headless");
>+  -moz-appearance: none !important;
>+  margin: 0 !important;
>+  height: 0 !important;
>+/** needed?
>+  padding: 0;
>+  border: 0 none;**/
>+}

Would visibility:collapse work here?

Comment 16

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

(In reply to Dão Gottwald [:dao] from comment #15)
> Comment on attachment 8544204 [details] [diff] [review]
> Alternative solution
> 
> >+menulist[headless="true"] {
> >+  -moz-binding: url("chrome://global/content/bindings/menulist.xml#menulist-headless");
> >+  -moz-appearance: none !important;
> >+  margin: 0 !important;
> >+  height: 0 !important;
> >+/** needed?
> >+  padding: 0;
> >+  border: 0 none;**/
> >+}
> 
> Would visibility:collapse work here?

No, because it seems that if a menupopup is a descendent of a hidden or collapsed node, then it is not made visible when openPopup is called on it.

Comment 17

[Dão Gottwald [:dao]]

visibility is inherited. You'd need to add visibility:visible on the menupopup.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 8544204 [details] [diff] [review]
Alternative solution

Otherwise this seems like a sensible approach, although it took me a minute to understand what you're doing. Maybe there's a better term than "headless" to express the intent...

Comment 19

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

Attachment: Make the parent process use a menulist popup for <select>'s in the content process. r=?

Comment 20

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

Attachment: Make the parent process use a menulist popup for <select>'s in the content process. r=?

Comment 21

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

Comment on attachment 8547664 [details] [diff] [review]
Make the parent process use a menulist popup for <select>'s in the content process. r=?

Cleaned things up a bit, and changed "headless" to "popuponly".

Comment 22

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [:dao] from comment #17)
> visibility is inherited. You'd need to add visibility:visible on the
> menupopup.

Have you tried this?

Comment 23

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

(In reply to Dão Gottwald [:dao] from comment #22)
> (In reply to Dão Gottwald [:dao] from comment #17)
> > visibility is inherited. You'd need to add visibility:visible on the
> > menupopup.
> 
> Have you tried this?

Ah, yes - sorry, I forgot to mention. I did indeed try this, and it still seems as if the menupopup will not appear if the menulist has visibility: collapse. :/

Comment 24

[Dão Gottwald [:dao]]

Could I please see that patch so I can try this out myself too and double-check for mistakes?

Comment 25

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

Yeah, for sure - will upload in the next few minutes.

Comment 26

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

Attachment: Patch with visibility:collapse for menulist with popuponly="true"

Comment 27

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

I'll hold on this until hear back about visibility:collapse. I couldn't get it to work, but maybe I missed something. It happens. :)

Comment 28

[Dão Gottwald [:dao]]

Comment on attachment 8547664 [details] [diff] [review]
Make the parent process use a menulist popup for <select>'s in the content process. r=?

Ok, I don't see any obvious mistake either.

Maybe Neil has some insight as to why visibility:collapse on the menulist element prevents the popup from showing.

I'm also unsure that this is really better than attachment 8537301 [details] [diff] [review].

Comment 29

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

(In reply to Dão Gottwald [:dao] from comment #28)
> Comment on attachment 8547664 [details] [diff] [review]
> Make the parent process use a menulist popup for <select>'s in the content
> process. r=?
> 
> Ok, I don't see any obvious mistake either.
> 
> Maybe Neil has some insight as to why visibility:collapse on the menulist
> element prevents the popup from showing.
> 
> I'm also unsure that this is really better than attachment 8537301 [details] [diff] [review]
> [diff] [review].

The advantage of having a menulist own the menupopup is because it seems that menupopups that want to have some notion of previous state[1] need to be owned by some kind of menu frame. That's my read on the situation, anyhow. This also keeps me from having to special-case the CSS for this particular type of menupopup, and instead just have a special type of menulist, which seems simpler to me.

[1]: I'm thinking of the _moz-menuactive state on the current selected item. With my current patch, the currently selected item gets _moz-menuactive until another item is selected / hovered, just like the non-e10s select dropdown. With the previous patch, we don't highlight the previous selection. A hack would be to apply the _moz-menuactive attribute on the currently selected item when opening the menupopup, but that attribute will not be unapplied until the item gets hovered over. That hack also seems to be a pale reinvention of the wheel. xul:menulist seems to take care of all of this for us.

Comment 30

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

Attachment: Make the parent process use a menulist popup for <select>'s in the content process. feedback=dao r=?

Comment 31

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

Comment on attachment 8554543 [details] [diff] [review]
Make the parent process use a menulist popup for <select>'s in the content process. feedback=dao r=?

Ok, this addresses some focus-related test failures I was hitting. Instead of having the ContentSelectDropdown be "pseudo-collapsed" at all times, I now have it hidden by default, and only show it just before showing the menupopup. I then re-apply the hidden attribute after the menupopup has closed. This fixes the tests, and will probably also avoid a ts_paint / tpaint regression (though I didn't test to see if there would be one).

Comment 32

[Neil Deakin]

Comment on attachment 8554543 [details] [diff] [review]
Make the parent process use a menulist popup for <select>'s in the content process. feedback=dao r=?

>-  open: function(browser, popup, rect) {
>+  open: function(browser, menulist, rect) {
>+    menulist.hidden = false;
>     currentBrowser = browser;
>-    this._registerListeners(popup);
>-    popup.hidden = false;
>+    this._registerListeners(menulist.menupopup);
> 
>     let {x, y} = browser.mapScreenCoordinatesFromContent(rect.left, rect.top + rect.height);
>-    popup.openPopupAtScreen(x, y);
>+    menulist.menupopup.openPopupAtScreen(x, y);
>+    menulist.selectedItem.scrollIntoView();

The menulist should already scroll the selected item into view for you (at nsMenuPopupFrame::EnsureMenuItemIsVisible) Do you know why this isn't happening?

Comment 33

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

(In reply to Neil Deakin from comment #32)
> Comment on attachment 8554543 [details] [diff] [review]
> Make the parent process use a menulist popup for <select>'s in the content
> process. feedback=dao r=?
> 
> >-  open: function(browser, popup, rect) {
> >+  open: function(browser, menulist, rect) {
> >+    menulist.hidden = false;
> >     currentBrowser = browser;
> >-    this._registerListeners(popup);
> >-    popup.hidden = false;
> >+    this._registerListeners(menulist.menupopup);
> > 
> >     let {x, y} = browser.mapScreenCoordinatesFromContent(rect.left, rect.top + rect.height);
> >-    popup.openPopupAtScreen(x, y);
> >+    menulist.menupopup.openPopupAtScreen(x, y);
> >+    menulist.selectedItem.scrollIntoView();
> 
> The menulist should already scroll the selected item into view for you (at
> nsMenuPopupFrame::EnsureMenuItemIsVisible) Do you know why this isn't
> happening?

Hrm, not exactly. We do hit nsMenuPopupFrame::EnsureMenuItemIsVisible, and then go into PresShell::ScrollFrameRectIntoView. We climb the ancestor chain until we hit an nsXULScrollFrame, and then call ScrollToShowRect, which calls ComputeNeedToScroll...

And we end up passing in all 0's for targetRect.y, targetRect.YMost, visibleRect.y and visibleRect.YMost. So ComputeNeedToScroll returns false, and we don't scroll.

I suspect this is because our <xul:menulist> item has no rect to speak of. Does that sound right?

Comment 34

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

Thanks for the reviews!

remote:   https://hg.mozilla.org/integration/fx-team/rev/3e382ae494a8

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3e382ae494a8

Comment 36

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

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1108761.html