1539651 - Convert places-popup-base and places-popup-arrow to Custom Elements

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P3)

Description

[Brian Grinstead [:bgrins]]

https://bgrins.github.io/xbl-analysis/tree/#places-popup-base

Once we have the base popup class in place (Bug 1531870), it should be possible to take these two extended menupopups.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1539651 - Proof of concept - convert places-popup-base and places-popup-arrow to Custom Elements

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Brian Grinstead [:bgrins] from comment #0)

https://bgrins.github.io/xbl-analysis/tree/#places-popup-base

Once we have the base popup class in place (Bug 1531870), it should be possible to take these two extended menupopups.

it appears the bug is unblocked now, which makes it a good candidate to wipe out next?

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to alexander :surkov (:asurkov) from comment #2)

(In reply to Brian Grinstead [:bgrins] from comment #0)

https://bgrins.github.io/xbl-analysis/tree/#places-popup-base

Once we have the base popup class in place (Bug 1531870), it should be possible to take these two extended menupopups.

it appears the bug is unblocked now, which makes it a good candidate to wipe out next?

I have a proof of concept here, feel free to take it! If I remember correctly there are styling issues with it especially when using shadow DOM.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: Bug 1539651 - Convert places-popup-base and places-popup-arrow bindings to Custom Elements, r=bgrins

Comment 5

[alexander :surkov (:asurkov)]

Attachment: Bug 1539651 - Convert places-popup-base and places-popup-arrow bindings to Custom Elements

Comment 6

[alexander :surkov (:asurkov)]

Emilio, I logged nsMenuPopupFrame::ComputeAnchorRect as you recommended in bug 1555497 comment #6 and the results looks identical between builds. Where to look next?

ComputeAnchorRect for : <toolbarbutton id='bookmarks-menu-button'>
AnchorRect: x=0, y=0, width=1920, height=2400
relative to root AnchorRect: x=72657, y=1980, width=1920, height=2400
relative to screen AnchorRect: x=81177, y=8640, width=1920, height=2400
ComputeAnchorRect for : <toolbarbutton id='bookmarks-menu-button'>
AnchorRect: x=0, y=0, width=1920, height=2400
relative to root AnchorRect: x=72657, y=1980, width=1920, height=2400
relative to screen AnchorRect: x=81177, y=8640, width=1920, height=2400
ComputeAnchorRect for : <toolbarbutton id='bookmarks-menu-button'>
AnchorRect: x=0, y=0, width=1920, height=2400
relative to root AnchorRect: x=72657, y=1980, width=1920, height=2400
relative to screen AnchorRect: x=81177, y=8640, width=1920, height=2400
ComputeAnchorRect for : <toolbarbutton id='bookmarks-menu-button'>
AnchorRect: x=0, y=0, width=1920, height=2400
relative to root AnchorRect: x=72657, y=1980, width=1920, height=2400
relative to screen AnchorRect: x=81177, y=8640, width=1920, height=2400
ComputeAnchorRect for : <toolbarbutton id='bookmarks-menu-button'>
AnchorRect: x=0, y=0, width=1920, height=2400
relative to root AnchorRect: x=72657, y=1980, width=1920, height=2400
relative to screen AnchorRect: x=81177, y=8640, width=1920, height=2400
ComputeAnchorRect for : <toolbarbutton id='bookmarks-menu-button'>
AnchorRect: x=0, y=0, width=1920, height=2400
relative to root AnchorRect: x=72657, y=1980, width=1920, height=2400
relative to screen AnchorRect: x=81177, y=8640, width=1920, height=2400
ComputeAnchorRect for : <toolbarbutton id='bookmarks-menu-button'>
AnchorRect: x=0, y=0, width=1920, height=2400
relative to root AnchorRect: x=72657, y=1980, width=1920, height=2400
relative to screen AnchorRect: x=81177, y=8640, width=1920, height=2400

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Probably at the relevant frame's mRect then. You can call DumpFrameTree() / DumpFrameTreeLimited() and try to find the different values.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

Probably at the relevant frame's mRect then. You can call DumpFrameTree() / DumpFrameTreeLimited() and try to find the different values.

Do you refer to https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Debugging_a_Performance_Problem#Dump_the_Frame_Tree_via_GDB or something else? Is it expected work with lldb (I'm on os x)?

Also not sure I caught what relevant frame is you refer to. Do you mean popup and its anchor frames?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Yes, that :)

And yeah, I suspect MenuPopupFrame and the relevant popup / popupset frames are the things you want to look at.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #9)

Yes, that :)

And yeah, I suspect MenuPopupFrame and the relevant popup / popupset frames are the things you want to look at.

so, finally I figured out that @position attribute was missed on a menupoup, which gave it wrong positioning

Comment 11

[alexander :surkov (:asurkov)]

Emilio, if you get a chance to take a look at bookmarks menu wrong height issue (D33821 patch), it'd be super helpful. Thank you!

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: bookmarks.png

How do I repro it? This is what I see, and it looks fine?

Comment 13

[alexander :surkov (:asurkov)]

Attachment: Screen Shot 2019-07-12 at 1.15.01 PM.png

(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)

Created attachment 9077664 [details]
bookmarks.png

How do I repro it? This is what I see, and it looks fine?

you'd need to have more items bookmarked, so the popup doesn't fit on the screen.

Comment 14

[Tim Nguyen :ntim]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)

Created attachment 9077664 [details]
bookmarks.png

How do I repro it? This is what I see, and it looks fine?

In your screenshot, you're also showing the Library menu, not the Bookmarks menu (which can be found in the customize mode).

Comment 15

[Emilio Cobos Álvarez (:emilio)]

So to be clear, what is not working is the popup scrolling when it overflows the screen, right?

I was a bit confused, since I don't see the scrolling on current nightly, but that's because I'm on Wayland and it's broken there (bug 1539651). Switching to X11 I can repro that.

This is the code that takes care of that, I'll check what it's doing.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

I think this is a styling issue.

The problem is that the arrowscrollbox is not receiving the overflow event in the vertical direction. It does receive (and properly ignore) the overflow event on the horizontal direction (if any).

I think this is not a layout issue, since the scroll range is zero (scrollbox.scrollTopMax is zero). But I'll poke a bit more.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Attachment: Frame tree of the first popup layout (good)

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Attachment: Frame tree of the first popup layout (bad)

Comment 19

[Emilio Cobos Álvarez (:emilio)]

So I did a bit of digging into this, and there are a bunch of frames that just aren't there when the popup is first laid out.

I think afterwards they end up there (since you can see them!), but I think the XUL layout code is bogus and messes up something with the dynamic change.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Well, after more digging, this is not a layout bug after all. I got confused by that initial layout, but that happens only because the order of the event listeners firing is different with and without XBL, in this case. So the onpopupshowing="" in the BMB_BookmarksPopup, which is what populates the menu, is just firing after the on_popupshowing from the custom element.

But that's alright, once it's populated we relayout just fine.

Here's your fix: You were missing a flex=1, which makes the xul element inflexible and thus makes it take the whole space unconditionally.

diff --git a/browser/components/places/content/places-menupopup.js b/browser/components/places/content/places-menupopup.js
index 694348dbb266..f5cf9b166f53 100644
--- a/browser/components/places/content/places-menupopup.js
+++ b/browser/components/places/content/places-menupopup.js
@@ -638,7 +638,7 @@ class MozPlacesPopupArrow extends MozPlacesPopup {
     return `
       <html:link rel="stylesheet" href="chrome://global/content/widgets.css" />
       <html:style>${this.commonStyles}</html:style>
-      <vbox class="panel-arrowcontainer">
+      <vbox class="panel-arrowcontainer" flex="1">
         <box class="panel-arrowbox">
           <image class="panel-arrow"></image>
         </box>

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #20)

Here's your fix: You were missing a flex=1, which makes the xul element inflexible and thus makes it take the whole space unconditionally.

thank you a lot! it's so bad that such a small thing may require so disproportionately large effort.

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:surkov, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 23

[Andrew Swan [:aswan]]

Alex is on PTO, I'm sure he'll get to this when he's back so reducing his needinfo backlog...

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd7cd9d7b6ad
Convert places-popup-base and places-popup-arrow bindings to Custom Elements r=mak

Comment 25

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/bd7cd9d7b6ad