Closed Bug 1612653 Opened 6 months ago Closed 5 months ago

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

Categories

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

74 Branch
Unspecified
macOS
defect
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 75
Iteration:
75.2 - Feb 24 - Mar 8
Tracking Status
firefox-esr68 --- unaffected
firefox72 --- unaffected
firefox73 --- unaffected
firefox74 --- verified
firefox75 --- verified

People

(Reporter: suishouen, Assigned: mak)

References

(Regression)

Details

(Keywords: regression)

Attachments

(6 files)

Attached image 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.

Seems to be regressed by Bug 1608022.

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

Is there any error in the browser console?

Flags: needinfo?(suishouen)
Attached file Browser Console Log

There seems to be no error in the Browser Console.

Attachment #9123909 - Attachment mime type: application/octet-stream → text/plain
Attached image menu.png

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

Flags: needinfo?(suishouen)

(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.

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.

Flags: needinfo?(suishouen)
  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.

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

Attached image screenshot2.png

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

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.

Component: Untriaged → Bookmarks & History
Flags: needinfo?(suishouen)
OS: Unspecified → macOS
Status: UNCONFIRMED → NEW
Ever confirmed: true

Thank you for the confirmation.

Summary: "Bookmark this page" or "Edit This Bookmark" in the "Bookmark menu" is blank. → On the first click, "Bookmark this page" or "Edit This Bookmark" in the "Bookmark menu" is blank.

"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…

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

Keywords: regression

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

Flags: needinfo?(mak)
Regressed by: 1608022
Summary: On the first click, "Bookmark this page" or "Edit This Bookmark" in the "Bookmark menu" is blank. → On the first click, "Bookmark this page" or "Edit This Bookmark" in the "Bookmark menu" is blank or obsolete.

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.

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?

Flags: needinfo?(mak) → needinfo?(gandalf)
Priority: -- → P1

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.

Flags: needinfo?(gandalf)

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

(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.

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

Flags: needinfo?(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.

Flags: needinfo?(mak)

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".

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.

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.

Assignee: nobody → mak
Status: NEW → ASSIGNED
Iteration: --- → 75.2 - Feb 24 - Mar 8

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

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.

Blocks: 1600228
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/373f76901953
Update 'Bookmark This Page' menuitems when the star state changes. r=Standard8
Points: --- → 3
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 75

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:
Attachment #9129202 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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 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).

Attachment #9129202 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 1619356

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

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.