Closed Bug 1108761 Opened 5 years ago Closed 5 years ago

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

Categories

(Toolkit :: XUL Widgets, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
e10s m4+ ---

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(4 files, 8 obsolete files)

23.37 KB, image/png
Details
13.10 KB, patch
Details | Diff | Splinter Review
14.48 KB, patch
enndeakin
: review+
mconley
: feedback+
Details | Diff | Splinter Review
129.52 KB, application/zip
Details
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. ¯\_(ツ)_/¯
bug 1091592 about improving stylings should be considered whichever way desktop decides to go forward.
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.
Attachment #8533326 - Flags: feedback-
Assignee: ally → mconley
Blocks: 1053981
No longer blocks: 1053981
Depends on: 1053981
Duplicate of this bug: 1108221
Attachment #8533326 - Attachment is obsolete: true
Attached image 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.
Attachment #8535018 - Attachment is obsolete: true
Attachment #8535758 - Attachment is obsolete: true
Attachment #8536851 - Attachment is obsolete: true
Comment on attachment 8537301 [details] [diff] [review]
Add class to let menulist > menupopup stylings apply to other popups. r=?

Something along these lines?
Attachment #8537301 - Flags: review?(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?
Blocks: 1010732
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?
Attachment #8544204 - Flags: feedback?(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?
(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.
visibility is inherited. You'd need to add visibility:visible on the menupopup.
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...
Attachment #8544204 - Flags: feedback?(dao) → feedback+
Attachment #8537301 - Attachment is obsolete: true
Attachment #8544204 - Attachment is obsolete: true
Attachment #8546775 - Attachment is obsolete: true
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".
Attachment #8547664 - Flags: review?(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?
(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. :/
Could I please see that patch so I can try this out myself too and double-check for mistakes?
Yeah, for sure - will upload in the next few minutes.
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. :)
Flags: needinfo?(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].
Flags: needinfo?(dao)
Attachment #8547664 - Flags: review?(enndeakin)
Attachment #8547664 - Flags: review?(dao)
Attachment #8547664 - Flags: feedback+
(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.
Attachment #8547664 - Attachment is obsolete: true
Attachment #8547664 - Flags: review?(enndeakin)
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).
Attachment #8554543 - Attachment description: Make the parent process use a menulist popup for <select>'s in the content process. r=? → Make the parent process use a menulist popup for <select>'s in the content process. feedback=dao r=?
Attachment #8554543 - Flags: review?(enndeakin)
Attachment #8554543 - Flags: feedback+
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?
Attachment #8554543 - Flags: review?(enndeakin) → review+
(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?
Flags: needinfo?(enndeakin)
https://hg.mozilla.org/mozilla-central/rev/3e382ae494a8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1128156
Flags: needinfo?(enndeakin)
You need to log in before you can comment on or make changes to this bug.