1799613 - edit bookmark dialog still clipped in 107 release

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Markus]

Attachment: Clipped edit bookmark dialog in v107

https://bugzilla.mozilla.org/show_bug.cgi?id=1795901
did not fix it properly.

The window is now a bit wider, but still clips a full button

Comment 1

[Markus]

Compare also to my screenshot from 107 beta in https://bugzilla.mozilla.org/show_bug.cgi?id=1799030

Comment 2

[Mark Banner (:standard8)]

Please can you confirm the build id of the version of Firefox you are using? You can get that by entering about:support in the address bar.

Comment 3

[Markus]

20221107173030

Comment 4

[Francesco Lodolo [:flod]]

I can confirm. Beta channel is now on RC (20221107173030), and I see the same problem on macOS with Italian.

Comment 5

[Francesco Lodolo [:flod]]

Oh, wait. This is a different bug :-(

The dialog opened from the context menu has been broken for a while (102+), and it was fixed in bug 1779695 (but for 108, not 107, because the patch is too invasive).

Sorry for missing that bit of information when you filed bug 1799030.

Comment 6

[Markus]

Are you sure about that "since 102+"?
I'm "swimming the beta lane" of releases and I'm using this dialog quite often, but never noticed this issue until 107b9.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

There was also a beta regression that should be fixed as well (bug 1795901)... If the latest beta is affected, maybe there's something borked with the uplift?

Actually... I think the fix for bug 1795901 is incomplete. We should've also removed the persisted attributes in a migration. Otherwise old persistent attributes might still apply. Or at least I don't see what wouldn't make them apply, this code doesn't check for ShouldPersistAttribute.

Comment 8

[Francesco Lodolo [:flod]]

(In reply to Markus from comment #6)

Are you sure about that "since 102+"?

Yes. I reported that bug in July when nightly was 104, and it's still broken in 102.4.0esr (at least for Italian).

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Reporter, if you open the browser console and paste this, does the problem get fixed?

for (let attr of ["width", "screenX", "screenY"]) {
  try {
    Services.xulStore.removeValue("chrome://browser/content/places/bookmarkProperties.xhtml", "bookmarkproperties", attr);
  } catch (ex) {
    Cu.reportError(`Error removing bookmarkproperties ${attr}: ` + ex);
  }
}

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Ah, https://hg.mozilla.org/releases/mozilla-beta/rev/a8f9c46694a1 landed only in rc1, can you update to that and see if it's fixed there first?

Comment 11

[Francesco Lodolo [:flod]]

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

Ah, https://hg.mozilla.org/releases/mozilla-beta/rev/a8f9c46694a1 landed only in rc1, can you update to that and see if it's fixed there first?

20221107173030 is already RC1.

Comment 12

[Emilio Cobos Álvarez [:emilio]]

Oh, ok I had indeed missed comment 5, yeah, this was broken for a while and fix is bug 1779695.

Comment 13

[Marco Bonardo [:mak]]

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

Actually... I think the fix for bug 1795901 is incomplete. We should've also removed the persisted attributes in a migration. Otherwise old persistent attributes might still apply. Or at least I don't see what wouldn't make them apply, this code doesn't check for ShouldPersistAttribute.

I thought the code would not persist attributes if they are not in persist="", not sure why it didn't happen in my testing, nor in QA.... I wonder if we ended up testing bookmarkProperties2 and not the remaining one?
That sounds like a bug in xulpersist... wdyt? Otherwise we'd need a ui migration, but I'd prefer avoiding the same issue in the future.
ShouldPersistAttribute is likely not the right thing to check though, we should rather get a list from the persist attributes (like here), and apply the attr only if it appears in the list. That requires some additional string find though, would we be ok with paying that price?

Comment 14

[Markus]

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

Reporter, if you open the browser console and paste this, does the problem get fixed?

for (let attr of ["width", "screenX", "screenY"]) {
  try {
    Services.xulStore.removeValue("chrome://browser/content/places/bookmarkProperties.xhtml", "bookmarkproperties", attr);
  } catch (ex) {
    Cu.reportError(`Error removing bookmarkproperties ${attr}: ` + ex);
  }
}

Uncaught ReferenceError: Cu is not defined

Changing the "Cu" part to a console.log() says

Error removing bookmarkproperties width: ReferenceError: Services is not defined

So, I'm not sure if I'm in the right console?

Comment 15

[Emilio Cobos Álvarez [:emilio]]

So I looked into this, and yeah I also thought that the right fix would be to check the persist attribute (and it's probably worth the cost). However we have a variety of code that relies on the current behavior (for once, all the callers to xulStore.persist() like these...)...

Comment 16

[Emilio Cobos Álvarez [:emilio]]

(In reply to Markus from comment #14)

So, I'm not sure if I'm in the right console?

Nope, you're not, see https://firefox-source-docs.mozilla.org/devtools-user/browser_console/index.html, sorry for not dropping that link earlier!

Comment 17

[Marco Bonardo [:mak]]

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

So I looked into this, and yeah I also thought that the right fix would be to check the persist attribute (and it's probably worth the cost). However we have a variety of code that relies on the current behavior (for once, all the callers to xulStore.persist() like these...)...

makes sense, so the ui migration is the best path forward. I think we can use this bug for it. let's fix in 108.

Comment 18

[Markus]

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

(In reply to Markus from comment #14)

So, I'm not sure if I'm in the right console?

Nope, you're not, see https://firefox-source-docs.mozilla.org/devtools-user/browser_console/index.html, sorry for not dropping that link earlier!

Ah thanks. Been there, but "Browser Console command line" is the key in that link.

Confirmed. This fixed it!

Comment 19

[Emilio Cobos Álvarez [:emilio]]

I see. So we should probably fix this in 107. The migration should be trivial for uplift.

Comment 20

[Marco Bonardo [:mak]]

Attachment: Bug 1799613 - Remove no longer persisted bookmark dialog attributes from xulstore. r=emilio

Comment 21

[Marco Bonardo [:mak]]

uplift is feasible, but I must play with migration numbers if we want to do it... and have separate patches for nightly and rc

Comment 22

[Marco Bonardo [:mak]]

I'll make a separate patch for uplift.

Comment 23

[Marco Bonardo [:mak]]

Attachment: Bug 1799613 - Remove no longer persisted bookmark dialog attributes from xulstore. UPLIFT TO 107 PATCH. r=emilio

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/308141e3ded9
Remove no longer persisted bookmark dialog attributes from xulstore. r=emilio

Comment 25

[Marco Bonardo [:mak]]

Comment on attachment 9302492 [details]
Bug 1799613 - Remove no longer persisted bookmark dialog attributes from xulstore. UPLIFT TO 107 PATCH. r=emilio

Beta/Release Uplift Approval Request

• User impact if declined: We are persisting width on bookmark dialogs yet, that may cause the dialog to be cut and hide or partially hide one of the buttons

IMPORTANT: only the UPLIFT TO 107 PATCH must be landed in 107, not the patch that landed in Nightly 108, they differ.

• 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: must test migrating a profile with persisted attributes from 106 to 107, though that requires running scripts in the Browser Console for the most part, to set attributes before, and check they are gone later.
  in 106 Services.xulStore.setValue("chrome://browser/content/places/bookmarkProperties.xhtml", "bookmarkproperties", "width", "200")
  in 107 Services.xulStore.getValue("chrome://browser/content/places/bookmarkProperties.xhtml", "bookmarkproperties", "width") must return an empty string and not "200"
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): it is just removing values from xulstore, it's a trivial ui migration.
• String changes made/needed:
• Is Android affected?: No

Comment 26

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/308141e3ded9

Comment 27

[Monica Chiorean]

Reproduce the issue on Mac 10.13 using Firefox build 106.0->upgraded to 107.0 and using the steps from comment #25.
Could NOT reproduce and confirm the fix on Nightly. Is there a way to reproduce/confirm the issue on Nightly build?

Also if I try to add a new bookmark from Library on localized builds (de), the Add new bookmark dialog does not display the buttons entirely, should I log a separate issue for this? Thank you so much.

Comment 28

[Monica Chiorean]

Hello Markus,

Can you please confirm the fix on Nightly build? (link to Nightly build https://archive.mozilla.org/pub/firefox/nightly/2022/11/2022-11-08-21-36-02-mozilla-central-l10n/). Thank you so much.

Comment 29

[Markus]

Sorry, I do not run the nightly version. I fixed my profile on 107 already with the script posted above.

But I can confirm that the clipping issue is present for the add bookmark dialog as well. :(

Comment 30

[Markus]

Attachment: add bookmark from library.jpg

Comment 31

[Marco Bonardo [:mak]]

The Add Bookmark for this page from the Library window History section is an interesting case, it's not using the new dialog type, I filed bug 1799888 for that, we should convert it to the new dialog type if possible (since it is opened in the browser window, not in the Library) and allow the tip label to wrap also in the old dialog type.

Regarding the steps to reproduce, I forgot that in Nightly the bug is harder to reproduce due to bug 1779695 being fixed. That bug fix is missing in 107. The only thing we can check in Nightly is that xulstore values are being removed. So run in the Browser Console
Services.xulStore.setValue("chrome://browser/content/places/bookmarkProperties.xhtml", "bookmarkproperties", "width", "200") in a build before this fix, then Services.xulStore.getValue("chrome://browser/content/places/bookmarkProperties.xhtml", "bookmarkproperties", "width") in a build after the fix should return an empty string. We can't check much more in Nightly.

In 107 one can reproduce the bug installing the german locale, then running in the Browser Console Services.xulStore.setValue("chrome://browser/content/places/bookmarkProperties.xhtml", "bookmarkproperties", "width", "200") and opening the edit bookmark dialog on a bookmark. Running Services.xulStore.removeValue("chrome://browser/content/places/bookmarkProperties.xhtml", "bookmarkproperties", "width") in the console (that is what this patch will do) fixes the problem

Comment 32

[Monica Chiorean]

I was able to verify the issue on Mac10.13/Ubuntu20.04/Win10 using localized DE Nightly build 107.0a1->upgraded to 108.0a1(20221109164728), after following the steps from comment#31 and getting result "" on Browser Console.
Also verified on US on WIn10 suing same settings as above.

Comment 33

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

The patch landed in nightly and beta is affected.
:mak, 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 34

[Donal Meehan [:dmeehan]]

Comment on attachment 9302492 [details]
Bug 1799613 - Remove no longer persisted bookmark dialog attributes from xulstore. UPLIFT TO 107 PATCH. r=emilio

Approved for 107.0RC2

Comment 35

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-release/rev/79f955e181b7

Comment 36

[Monica Chiorean]

I was able to verify the issue on Mac10.13/Ubuntu20.04/Win10 using localized DE Beta build 106.0->upgraded to 107.0 (20221110173214), after following the steps from comment#25 and getting result "" on Browser Console.
Also verified on US on WIn10 suing same settings as above.