1779695 - Add bookmark overlay is broken with long localized descriptions

Status: VERIFIED FIXED

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

Description

[Francesco Lodolo [:flod]]

Attachment: Screenshot overlay

The window is quite broken on the Italian build, apparently because of a long description that doesn't wrap.

The problem is present on release, so it's been there for a while (possibly since bug 1693139?).

Comment 1

[Marco Bonardo [:mak]]

I could reproduce this a few days ago, now I can't anymore.
I'm sure when proton was implemented it was ok, so it may actually be a regression or somehow intermittent.

Comment 2

[Marco Bonardo [:mak]]

I will try in mozregression and fresh profile.

Comment 3

[Marco Bonardo [:mak]]

I cannot reproduce with a clean profile, but I have some ideas about what this may be.
We clone this dialog here https://searchfox.org/mozilla-central/rev/7b9d23ece4835bf355e5195f30fef942d376a1c7/browser/components/places/PlacesUIUtils.sys.mjs#511-513 to allow having 2 different sizes for it, depending on whether the dialog is resizable or not (that depends on whether it contains the folder picker or not). The size of the dialog is then stored in XULStore for future reopenings of the same dialog.
That means the size can easily go otu of control, if for example we change that text or the localization.

I think we should allow the hint to wrap, to avoid the problem.

Comment 4

[Marco Bonardo [:mak]]

This can easily be reproduce using the Add bookmark for this tab context menu entry on a tab, with a localized version, like the Italian one.

Bug 1760637 is likely culprit, we probably calculate the dialog size before showing the tags field, due to awaits added in https://hg.mozilla.org/mozilla-central/rev/4e81d3826042#l3.55 and https://hg.mozilla.org/mozilla-central/rev/4e81d3826042#l3.77

I still think it would make sense to wrap the text, but we should also see if we can refactor the code to do all the showing/hiding before any await.

Comment 5

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

Set release status flags based on info from the regressing bug 1760637

Comment 6

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

:jteow, since you are the author of the regressor, bug 1760637, could you take a look?
For more information, please visit auto_nag documentation.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

I'm moderately sure this shouldn't be a problem anymore, see bug 1793355.

Comment 8

[Francesco Lodolo [:flod]]

Not only it's still there, it got much worse :-(

Comment 9

[Francesco Lodolo [:flod]]

Attachment: Nightly 108

Nightly 108.0a1 20221018094831

Comment 10

[Emilio Cobos Álvarez (:emilio)]

I'm confused, I used the Windows installer for nightly in italian and I see a working menu. Is it intermittent or something?

Comment 11

[Francesco Lodolo [:flod]]

It's not intermittent for me on macOS, and it happens on two different machines at this point. I can also reproduce on a clean profile.

I'm still getting the slightly larger size on this MBPRO14 connected to an external 27" display, the dialog above is from a MBPRO13's integrated display.

Also, just to be clear: this is the dialog you get from the context menu on a tab.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Ah, I can repro with this and see why this is:

diff --git a/browser/locales/en-US/browser/editBookmarkOverlay.ftl b/browser/locales/en-US/browser/editBookmarkOverlay.ftl
--- a/browser/locales/en-US/browser/editBookmarkOverlay.ftl
+++ b/browser/locales/en-US/browser/editBookmarkOverlay.ftl
@@ -39,11 +39,11 @@ bookmark-overlay-tags-expander =
   .tooltiptext = Show all tags
   .tooltiptextdown = { bookmark-overlay-tags-expander.tooltiptext }
   .tooltiptextup = Hide

 bookmark-overlay-keyword-2 =
   .value = Keyword
   .accesskey = K

-bookmark-overlay-tags-caption-label = Use tags to organize and search for bookmarks from the address bar
+bookmark-overlay-tags-caption-label = Use tags to organize and search for bookmarks from the address bar Use tags to organize and search for bookmarks from the address bar Use tags to organize and search for bookmarks from the address b

 bookmark-overlay-keyword-caption-label-2 = Use a single keyword to open bookmarks directly from the address bar

The issue is that the overlay-tags-caption-label is dynamically uncollapsed, so its size isn't accounted for when sizing the dialog.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1779695 - Use modern flex on in-content pages. r=Gijs,dao

This fixes the edit bookmarks dialog by properly allowing stuff to wrap once
the dialog width has been fixed.

Let's try to do this and opt-out any specific pages that may hit issues,
rather than opting-in one by one, now that's early in the cycle?

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1779695 - Await for panel initialization properly. r=mak

This fixes the bug even without the other patch, making sure that
mozSubdialogReady awaits for the panel init code to run.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1779695 - Unify editBookmark.css. r=dao

And move the rule in the previous patch from places.css to there.

Depends on D159690

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c828a3491696
Use modern flex on in-content pages. r=Gijs,dao

Comment 17

[Norisz Fay [:noriszfay]]

Backed out for causing bc failures on browser_connection_dnsoverhttps.js

Backout link

Push with failures

Failure log

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c6037523786
Use modern flex on in-content pages. r=Gijs,dao,preferences-reviewers

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d7a87dc0fe9
Unify editBookmark.css. r=dao

Comment 20

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/6c6037523786
https://hg.mozilla.org/mozilla-central/rev/0d7a87dc0fe9

Comment 21

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

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit auto_nag documentation.

Comment 22

[Francesco Lodolo [:flod]]

Dialog is displayed correctly on 108.0a1, Build ID 20221020215126

Comment 23

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

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox107 to wontfix.

For more information, please visit auto_nag documentation.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

The patch as-is can't be uplifted, it's too risky. But we need to figure out something for bug 1795901 and I might be able to create a targeted fix.

Comment 25

[Francesco Lodolo [:flod]]

Bug 1760637 landed in 102, and the dialog is indeed broken in ESR.

At this point, waiting for 108 might not be a problem, also considering I haven't seen a single duplicate of this bug.

Comment 28

[Alin Ilea, Desktop Test Engineering]

Reproduced the issue Nightly 105.0a1 using the Italian localization build.
Verified - Fixed in Beta 108.0b3 and the latest Nightly 109.0a1 (2022-11-18) on Windows 10, macOS 12 and Ubuntu 20.