1612653 - On the first click, "Bookmark this page" or "Edit This Bookmark" in the "Bookmark menu" is blank or obsolete.

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Eiichi]

Attachment: Screenshot

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:74.0) Gecko/20100101 Firefox/74.0

Steps to reproduce:

1. Quit Firefox.
2. Launch Firefox.
3. Click "Bookmarks menu".

Actual results:

On the first click, "Bookmark this page" or "Edit This Bookmark" in the "Bookmarks menu" is blank.
On the second click, "Bookmark this page" or "Edit This Bookmark" in the "Bookmarks menu" is not blank.

Expected results:

"Bookmark this page" or "Edit This Bookmark" in the "Bookmarks menu" shoul not be blank.

Comment 1

[Eiichi]

Seems to be regressed by Bug 1608022.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Thank you for the report. I can't reproduce on Linux, will try on Mac next.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Is there any error in the browser console?

Comment 4

[Eiichi]

Attachment: Browser Console Log

There seems to be no error in the Browser Console.

Comment 5

[Eiichi]

Attachment: menu.png

Sometimes, the same issue occurs when Nightly has no open windows.

Comment 6

[Eiichi]

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #2)

I can't reproduce on Linux, will try on Mac next.

Can you reproduce this on Mac?
I can reproduce this issue with a fresh profile.

Comment 7

[Alexandra Martin]

Hi,

I couldn't reproduce this issue on Mac 10.14.6 or 10.13.6 using Firefox Nightly 74.0a1 (2020-02-04), Beta 73.0 or Release 72.0.2. If you can give us further information by answering the following questions, it might help in finding the problem.

What Mac version are you using?
Was the issue reproduced in the latest Firefox Nightly build? You can find it here: https://www.mozilla.org/en-US/firefox/channel/desktop/.
Could there be other steps that might help in reproducing the problem? A screencast might be helpful in this case.
Have you tried reproducing the issue using Safe Mode? (https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode)

Thank you for the report.

Comment 8

[Eiichi]

1. I'm using MacOS Version 10.13.6 (17G11023).
2. I can reproduce this issue using Firefox Nightly 74.0a174.0a1 (2020-02-05).
3. This issue occurs only on Firefox Nightly since Bug 1608022 landed.
4. I confirmed this with a new profile.

Comment 9

[Eiichi]

Attachment: Installation of Nightly with a new profile.

How to reproduce:
Step 1. Install Firefox Nightly with a new profile.
Step 2. Click "Continue"

Comment 10

[Eiichi]

Attachment: screenshot2.png

How to reproduce:
Step 3. Click "Firefox Nightly" -> "About Nightly".
Step 4. Click "Bookmarks" Menu.

Comment 11

[Alexandra Martin]

Thank you for the detailed steps.

The issue can be reproduced on Mac 10.13, 10.14 and 10.15 using the latest Nightly build (2020-02-05). Latest Release and Beta builds are not affected.

I am assigning a component, so our developers can take a look. If the selected component is not the right one, please feel free to change it.

Comment 12

[Eiichi]

Thank you for the confirmation.

Comment 13

[Eiichi]

"data-l10n-id" of menuitem id="menu_bookmarkThisPage" is not specified in browser.xhtml.
Isn't this allied to this issue?
I'm not a coder, so I may be wrong, though…

Comment 14

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

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 15

[Pascal Chevrel:pascalc (PTO until April 26)]

Marco, could you triage and prioritize this regression with regards to our upcoming releases please? Thanks

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

So, it seems that this function is never called or exits early - https://searchfox.org/mozilla-central/source/browser/base/content/browser-places.js#1705

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

I investigated it some today and this affects not just the first click and not just blank.

Basically, updateBookmarkPageMenuItem sets the menu l10n and is fired each time a user clicks on the Bookmarks menu item.

STR:

1. Open Firefox on MacOS

2. Click on Bookmarks menu

3. Notice that the item is blank

4. Close menu

5. Click on Bookmarks menu

6. Now it's translated

7. Open a page - https://www.duckduckgo.com

8. Bookmark it

9. Click on Bookmark menu

10. Notice that the item says "Bookmark this page"

11. Close menu

12. Click on Bookmark menu

13. Notice that the item now says "Edit this bookmark"

14. Unbookmark the page

15. Click on Bookmark menu

16. Notice that the item says "Edit this bookmark"

17. Close and reopen the Bookmark menu

18. Notice that now the item says "Bookmark this page"

The issue is that it seems like when its clicked, the document.l10n.setAttributes(menuItem, "menu-bookmark-this-page"); operation is not reflected in the UI until the second call happens.

I assume that there must be some additional "copying" happening where the value of a <menuitem/> which the JS operates on gets applied onto some other piece of DOM that is displayed, and because l10n is async, that changed order from:

1. User activated Bookmarks menu
2. updateBookmarkPageMenuItem is called
   2.1) it sets the label
3. Something copies the <menuitem/> with the label to the UI
4. Menu is displayed and UI has the translation
5. User clicks on Bookmarks menu again
6. ...

to:

1. User activated Bookmarks menu
2. updateBookmarkPageMenuItem is called
   2.1) it sets the data-l10n-id
3. Something copies the <menuitem/> prior to Fluent applying the translation
4. Menu is displayed with "old" translation (either blank, or previous value)
5. Fluent asynchonously applies the translation on the "shadow" element that is not visible
6. User clicks on Bookmarks menu again
7. updateBookmarkPageMenuItem is called
   7.1) it sets the data-l10n-id
8. Something copies the <menuitem/> with the label translated by Fluent in (5)
9. Menu is displayed with "new" translation

I'd like to understand when the "copying" happens and if we can delay it until the async translation is applied.

Comment 18

[Marco Bonardo [:mak]]

IIRC (but I'm not an expert of Mac bindings), menus in the main Mac menubar must be populated and updated on popupshowing, once they are shown you can't modify them anymore. Is it possible the translation tries to update the menu after popupshowing?

Comment 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

Yep, that would fit the description. So, we're currently firing onpopupshowing=setL10nId but the translation happens on the next animation frame.

I see two ways out of it:

1. We can somehow fire the update method on click and then fire popupshow once the translation is done
2. We can make the update method async, block it on translation being applied and block popupshowing on it

Not sure which one is possible.

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf]]

We could also give up on updating this string on popupshowing and instead update the l10n-id when action happens (star/unstar)...

Comment 21

[Marco Bonardo [:mak]]

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #20)

We could also give up on updating this string on popupshowing and instead update the l10n-id when action happens (star/unstar)...

That is maybe what was happening with xbl observers before it was changed? I don't recall exactly, but it sounds like plausible.

Comment 22

[Jens Stutte [:jstutte]]

Can we expect a patch here soon for 74 beta uplift? Nobody seems assigned, though there is much movement here. Thank you!

Comment 23

[Marco Bonardo [:mak]]

I can try to find some time today or tomorrow, depending on other priorities.
We could hook into handlePlacesEvents to additionally update just the main menubar entry on Mac. Though I don't know if that'll be enough to workaround the problem until I try.

Comment 24

[Zibi Braniecki [:zbraniecki][:gandalf]]

My guess is that the whole architecture would be much simpler if we just set the l10n-id in result of action.

The reason we probably didn't do that was that there was a synchronous cost to update the menubar and the person working on bookmark action wanted to avoid having to pay that.
With Fluent, the localization happens asynchronously on the next animation frame, so updating l10n-id should be very cheap and I'd recommend doing that instead of the whole "update l10n on menu open".

Comment 25

[Marco Bonardo [:mak]]

The problem is that the star is not the only way to add a bookmark to the current page, many things could do it, included Sync or import, and the string must update accordingly.
Additionally, you could have a thousand bookmarks changing at once (a large import for example), and you don't want to spend time on each single bookmark update to update a string that may never be visible. That's why it happens on popup showing, we update it when it's actually necessary. It's not much about the cost of doing an update, it's about multiplying that cost by a thousand times.
The good thing is that the Star is already listening for bookmark changes, so adding a small cost there will be acceptable. Doing all the work though may not be acceptable.

Comment 26

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ah, thank you for the explanation!

The good thing is that the Star is already listening for bookmark changes, so adding a small cost there will be acceptable. Doing all the work though may not be acceptable.

Yes, my understanding was that there is some code that reacts to the active page bookmark status change. Even if the trigger is not star itself, I expected there to be an equivalent of "OnCurrentTabBookmarkStatusChange" which would set the star status and could set the bookmark menu l10n-id.

Comment 27

[Marco Bonardo [:mak]]

yes, you are right, I can try to reuse that code.

Comment 28

[Marco Bonardo [:mak]]

Attachment: Bug 1612653 - Update 'Bookmark This Page' menuitems when the star state changes. r=standard8!

Instead of updating the Bookmark This Page / Edit Bookmark menuteitems on popupshowing,
update them when the star button is suppposed to change. This better supports MacOS behavior
where the native menubar can't be updated after being shown, and avoids many callpoints in
favor of just a few.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/373f76901953
Update 'Bookmark This Page' menuitems when the star state changes. r=Standard8

Comment 30

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/373f76901953

Comment 31

[Marco Bonardo [:mak]]

Comment on attachment 9129202 [details]
Bug 1612653 - Update 'Bookmark This Page' menuitems when the star state changes. r=standard8!

Beta/Release Uplift Approval Request

• User impact if declined: The Mac native menubar Bookmarks menu may show an empty label instead of Bookmark This Page, or the wrong label (Edit bookmark instead of Bookmark This Page, or viceversa) not reflecting the state of the current page.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: There are various points in the UI that get the string updated:

1. the bookmarks menu in the menubar (native on mac, hidden on Ubuntu/Windows)
2. the Star in the content contextual menu
3. The Bookmark PageAction icon in the urlbar meatballs menu
4. The bookmarks menu in the Library panel

all of these menus have a Bookmark This Page / Edit Bookmark menuitem, whose label changes depending on the bookmarked/starred state of the current tab page.
All of these should update to the appropriate label when the bookmarking state of the current tab page changes.

• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): This is changing the logic used to update the Bookmark This Page labels; while the new logic makes even more sense than the old one and from our internal testing it works pretty well, it would be nice if it'd have a bit more testing, QA verification may be good enough though.
  In case of emergency we could backout and live with the Mac bug for one version, or you may decide the Mac bug is not critical enough to take this small risk.
• String changes made/needed:

Comment 32

[Timea Cernea [:tbabos][inactive]]

Verified - Fixed on latest Nightly (2020-02-28) on MacOS 10.15 to ensure all the "Bookmark this Page" labels are correctly changed to Edit based on the state of the page.

Verified as requested by Marco the following:

• Bookmarks menu in the menu bar
• The star icon visual aspect and tooltip from the context menu
• The Star Bookmark icon from the urlbar
• The bookmark menu from the Library section
• "Bookmark this Page" is displayed for the first time when the menu is opened and after removing a page from bookmarks
• "Edit this Bookmark" is displayed as expected in the menu after bookmarking the page

Waiting for Uplift to Beta, if there is anything else to verify that was not mentioned please let me know.

Comment 33

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9129202 [details]
Bug 1612653 - Update 'Bookmark This Page' menuitems when the star state changes. r=standard8!

P1 and verified by QA on nightly, let's uplift to the beta branch (and get QA to verify on 74RC).

Comment 34

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8fa24b914995

Comment 36

[Timea Cernea [:tbabos][inactive]]

Verified - fixed on Firefox RC 74.0 (Build ID: 202000302184608) following the same expected results as in Comment 32 on MacOS 10.14.

Comment 37

[Jonathan]

(In reply to Timea Cernea [:tbabos][inactive] from comment #32)

Verified - Fixed on latest Nightly (2020-02-28) on MacOS 10.15 to ensure all the "Bookmark this Page" labels are correctly changed to Edit based on the state of the page.

Verified as requested by Marco the following:

• Bookmarks menu in the menu bar
• The star icon visual aspect and tooltip from the context menu
• The Star Bookmark icon from the urlbar
• The bookmark menu from the Library section
• "Bookmark this Page" is displayed for the first time when the menu is opened and after removing a page from bookmarks
• "Edit this Bookmark" is displayed as expected in the menu after bookmarking the page

Waiting for Uplift to Beta, if there is anything else to verify that was not mentioned please let me know.

Thanks for sharing this solution. As I was using a separate browser for https://nyapk.com/pikashow-apk-2/ which was showing the same issue for bookmarking. As I followed your mentioned steps which you posted here and the issue got fixed. thanks for your posting here

Comment 38

[Jesmeen Wilson]

For instance, thinking of an old friend and bumping into him later that day, or hearing a song for the first time and suddenly hearing it everywhere, or seeing certain numbers all over the place.

Mirror hours are a significant part of synchronicity, more specifically, numeric synchronicity, and indicate that the viewer is in alignment.

Seeing synchronicity of mirror hours is a sign that the universe or your Higher-Self reaches out to you to let you know that you are on the right path. visit for more: https://www.thefearlab.com/

Comment 39

[Jorge Auther]

I was planning to install the new version, but it keeps telling me to close firefox first, click on the icon and nothing happens, is gone to internet explore and found Mozilla support but nothing listed there help

Comment 40

[Jorge Auther]

A bookmark is a place holder for a web page that will allow you quick access to that page instead of having to browse to it or search for it. Instead of typing a web page in Google, clicking the bookmark will direct you to that page immediately.

Firefox – Browse to the website you wish to bookmark. In the toolbar at the top of the screen, select bookmark. Click “Bookmark This Page.”
Chrome – Browse to the website you wish to bookmark. https://thabani.co.za/uk49s-results-today/ In the toolbar at the top, click the Star next to the address bar.
Edge – Browse to the website you wish to bookmark. In the toolbar at the top, click the Star at the top right. This will open your Favorites menu.