1545700 - Bookmarks Menu item is empty at first time popup

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect, P1)

Description

[Alice0775 White]

Attachment: Screenshot with new profile

Reproducible: always

Steps To Reproduce:

1. Start Nightly with new profile
2. Right Click on title bar and Enable Menu bar
3. Click on "Bookmarks" menu

If not reproduce,
4. Restart browser
5. Click on "Bookmarks" menu

Actual Results:
Some bookmarks Menu item are empty
See attached screenshot

Expected Results:
should not empty

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=27157cc6395ef681545b5a8ae5f95fc584f38792&tochange=223a8a0c4eed19547e6e843cfd265c728f7dc7c2

Suspect :
223a8a0c4eed Brian Grinstead — Bug 1519502 - Convert menu bindings to a Custom Element r=surkov

Comment 1

[Alice0775 White]

Attachment: screenshot with a lot of bookmarks profile

Comment 2

[Alice0775 White]

Attachment: screenshot, bookmarks toolbar and its subfolder

Bookmarks folder label are missing...

Comment 3

[Brian Grinstead [:bgrins]]

Neil, my best initial guess before I can test this on Windows is that the menus are being created after this code runs https://hg.mozilla.org/integration/autoland/rev/223a8a0c4eed#l13.238. If that's the case then I think isInMenuPopup should turn into isInClosedMenuPopup so we eagerly render if it's being inserted into an already-opened popup. I don't think there's currently an attribute set on menupopups like open that I could use to update that selector. I guess I could go ahead and add it right here in this function, but I'm guessing there's a better place. Do you think this could be done on the platform side when the popup is actually opened (and removed when it's closed)?

Comment 4

[Brian Grinstead [:bgrins]]

Alice0775, I haven't been able to test this locally yet but I put together a patch that I think might fix it. I have a try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8be840e5002f879e228ddaa2dd29cb48a016714. Would you be able to test it please? It's an artifact build so the binaries should be ready in 10-20 minutes.

Comment 5

[Kazumasa Hasegawa (Kazz)]

Not only bookmarks, context menu too.
See https://bugzilla.mozilla.org/show_bug.cgi?id=1519502#c15

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Kazumasa Hasegawa (Kazz) from comment #5)

Not only bookmarks, context menu too.
See https://bugzilla.mozilla.org/show_bug.cgi?id=1519502#c15

How can I reproduce that context menu issue? Is that entry coming from an addon?

Comment 7

[Kazumasa Hasegawa (Kazz)]

How can I reproduce that context menu issue? Is that entry coming from an addon?

Yes.
That one is from my url2clipboard addon.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Kazumasa Hasegawa (Kazz) from comment #7)

How can I reproduce that context menu issue? Is that entry coming from an addon?

Yes.
That one is from my url2clipboard addon.

Thanks for the info! I can confirm the context menu issue locally without my patch applied and that it's fixed with it applied.

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: Bug 1545700 - Eagerly render <menu> inserted into already-opened menupopup

Comment 10

[Brian Grinstead [:bgrins]]

I've pushed up a patch that fixes the addon context menu issue at https://bugzilla.mozilla.org/show_bug.cgi?id=1519502#c15 which I hope is the same fix needed for this bug. Would like confirmation from Alice0775 or Kazumasa if possible that it fixes the bookmarks menu issue, but if nothing else we could land the context menu fix and follow up if it doesn't.

Comment 11

[Alice0775 White]

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

Alice0775, I haven't been able to test this locally yet but I put together a
patch that I think might fix it. I have a try push at
https://treeherder.mozilla.org/#/
jobs?repo=try&revision=d8be840e5002f879e228ddaa2dd29cb48a016714. Would you
be able to test it please? It's an artifact build so the binaries should be
ready in 10-20 minutes.

I cannot reproduce the problem on the try build(Windows 2012 x64 opt Ba). The try build fixed this problem.

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0
Built from https://hg.mozilla.org/try/rev/d8be840e5002f879e228ddaa2dd29cb48a016714

Comment 13

[Kazumasa Hasegawa (Kazz)]

Confirmed fixed on try too.

Comment 14

[Brian Grinstead [:bgrins]]

Thank you both for testing. Working to get the fix landed ASAP.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/895753bc084b
Eagerly render <menu> inserted into already-opened menupopup r=surkov

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/895753bc084b

Comment 18

[Maria Berlinger [:mberlinger], Fxa/Subplat]

I’ve tried to reproduce this issue following the str from Comment 0 on an affected build but without success.
Alice, could you please confirm this is fixed on latest beta?

Comment 19

[Alice0775 White]

(In reply to Maria Berlinger [:maria_berlinger], Release Desktop QA from comment #18)

I’ve tried to reproduce this issue following the str from Comment 0 on an
affected build but without success.
Alice, could you please confirm this is fixed on latest beta?

I can reproduce the issue on autoland 68.0a1 BuildID=20190419000651.
And I verified as fixed on Firefox68.0beta11 and Nightly69.0a1 BuildID=20190618041042.

Comment 20

[Maria Berlinger [:mberlinger], Fxa/Subplat]

Thanks, Alice!
Removing the qe+ flag.