1795901 - Edit Bookmark dialog clipped

Status: VERIFIED FIXED

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

Description

[M8R-p7]

Attachment: Edit Bookmark dialog

The Edit Bookmark dialog has become clipped in Nightly 107.0a1.

STR:
0. Create a new Profile.

1. Right-click a default bookmark in the Bookmarks Toolbar

Expected:
The fields are all in view, with equal spacing on the inline sides.

Actual Results:
The fields are clipped on the right hand side (see attachment).

mozregression reduced to this changeset containing only bug1792730.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

This is bug 1793355. Current nightly is 108, can you update and confirm it's fixed?

Comment 2

[M8R-p7]

Sorry about getting the wrong version in the report - I had seen bug1793355 and dismissed it because I was still seeing the clipping in newer Nightlies - the screenshot was taken in Nightly 107 20221016093143, and updating to Nightly 108 20221018094831 didn't resolve the issue.

However, I can't reproduce it in new profiles. I wrongly assumed that since mozregression uses new profiles that that would be enough - it was showing me the clipped panels after all - but I think that was actually bisecting bug1793355. I'll be sure to test more thoroughly in future.

Anyway - I'm still getting this on two different existing profiles, on different machines. After further investigation I think what I'm seeing is being caused by stale a xulstore.

Comment 3

[Walter K.]

I also got the cropped Edit Bookmark dialog in today's FF Dev Edition 107.0b2 (64-bit) with a preexisting profile.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

[Tracking Requested - why for this release]: Seems we should figure out what's going on here...

Comment 5

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

The bug has a release status flag that shows some version of Firefox is affected, thus it will be considered confirmed.

Comment 6

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

The bug is marked as tracked for firefox107 (beta). However, the bug still isn't assigned.

:cbellini, could you please find an assignee for this tracked bug? Given that it is a regression and we know the cause, we could also simply backout the regressor. If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit auto_nag documentation.

Comment 7

[Chris Bellini (:cbellini)]

I'm not able to repro this in 107.b2 or the latest 108 Nightly on Windows 11. Is this still being seen, perhaps on a specific platform and/or screen resolution?

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Could you please evaluate this in the Browser Console of the profile where it happens, and then on the profile where it doesn't happen?

["screenX", "screenY", "width"].map(a => Services.xulStore.getValue("chrome://browser/content/places/bookmarkProperties.xhtml", "bookmarkproperties", a))

Comment 9

[M8R-p7]

Updated to Nightly 20221020105605 and DevEd 20221018185850.

Evaluation results:

Affected:

• DevEd (Windows): [ "460", "335", "376" ]
• Nightly (Windows): [ "441", "285", "376" ]
• Nightly (Linux): [ "0", "0", "400" ]

Unaffected:

• DevEd (Linux): [ "", "", "" ]
• Release (Windows/106.0.1) : [ "460", "360", "376" ]
• ESR (Windows/102.4.0esr): [ "153", "182", "376" ]

The various other profiles I have lying around are unaffected, with [ "", "", "" ] - these are usually new profiles created to test something specific, so don't see "real" usage. The DevEds are my daily drivers. ESR has the most recent profile, dating from March 2022.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I wonder if the problem is the xulstore width along with the lack of a fix for Bug 1779695. I suspect with Bug 1779695 this will be "fixed" in the sense long text will be allowed to wrap.
Then there is some plan to change the code in initPanel to handle dom changes before any async changes for which we should file a bug.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

If I force a width on xulstore, and without Bug 1779695, I can indeed reproduce this exact bug.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

I think easiest fix would be removing the persistence of these attributes to begin with. It's a non-resizable dialog anyways?

Comment 13

[Negritas Sergiu, Desktop QA]

Hello! I have also managed to reproduce the issue on Ubuntu 22.04 with firefox 108.0a1(2022-10-21), I added myself to CC in order for QA to monitor the evolution and potential fix of this issue.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

I think easiest fix would be removing the persistence of these attributes to begin with. It's a non-resizable dialog anyways?

They used to be resizable, see https://searchfox.org/mozilla-central/source/browser/components/places/PlacesUIUtils.sys.mjs#522 but when dialogs were redesigned the last time that got lost.
The main reason to have bookmarkProperties cloned was so that dialogs with the folder picker persist a size different from dialogs without it, so people having a complex hierarchy could expand the dialog and see it better, but it was not affecting dialogs without the picker.
Also the star panel used to expand when the folder picker was opened, but that regressed recently, probably with some of your changes (didn't run mozregression yet, just guessing). I still have to file a bug for that.
Now, since most of this got lost, and I didn't hear major concerns, we could decide to simplify the whole management, remove the dialog cloning and stop persisting its size as a whole. Maybe at the same time we should increase their min-width a bit.
It's likely there's a better solution to the problem of complex hierarchies, but at the same time I don't think it affects a meaningful percentage of users (no telemetry to argument further).

Comment 15

[Donal Meehan [:dmeehan]]

:mak tracking this for 107 as requested in Comment 4.
Based on the info in Comment 14, do you plan on addressing this in a later release?
Wondering about the severity of this, it's impact on 107, and if we should still be tracking. Thanks.

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I think we can continue tracking it. If one of the buttons end up not being usable, that's a problem for the user.
Fully removing resizeable and persistence may be the way to go, at least for now. It's de-facto what we have from some time, and we didn't see major concerns.
I'd also evaluate increasing min-width to 40em (from the current value of 30) to make longer strings in these fields more usable.
Both changes seem fine for an uplift.

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Bug 1795901 - Stop persisting attributes for the bookmark properties dialog, r=dao!,emilio!

From quite some time the vast moajority of instances of this dialog are non
resizable, and persisting attributes is now creating problems with the new flex
layout.
Thus, this patch is removing attributes persistence and the dialog duplication
whose only intent was to persist different attributes depending on the dialog
contents.
As a compromise due to not being able to resize the dialog, we increase the
dialog min-width.

Comment 18

[Donal Meehan [:dmeehan]]

:mak this week is the final beta week for 107.
If this fix could be landed and get a beta uplift request when ready?

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea54536e4dc0
Stop persisting attributes for the bookmark properties dialog, r=dao,emilio

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 9300535 [details]
Bug 1795901 - Stop persisting attributes for the bookmark properties dialog, r=dao!,emilio!

Beta/Release Uplift Approval Request

• User impact if declined: The bookmarks dialog may be cut, to a point where one of the buttons is not visible
• 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: You need a profile with dialog width persisted in xulstore, that is something you must force through a script because all the dialogs are no more resizable.
  You can run this snippet in the Browser Console
  Services.xulStore.setValue("chrome://browser/content/places/bookmarkProperties.xhtml", "bookmarkproperties", "width", "200") to set the width attribute to a small value, then open the dialog and the bug should be visible.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We are removing persistence of the width attributes, that means the worst effect should be a slightly smaller or larger bookmark dialog.
• String changes made/needed:
• Is Android affected?: No

Comment 21

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/ea54536e4dc0

Comment 23

[Hani Yacoub, Desktop QA]

Verified as fixed on Windows 10 x64, macOS 11.6 and on Ubuntu 20.04 x64.

Comment 24

[Donal Meehan [:dmeehan]]

Comment on attachment 9300535 [details]
Bug 1795901 - Stop persisting attributes for the bookmark properties dialog, r=dao!,emilio!

Approved for 107.0RC1

Comment 25

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a8f9c46694a1

Comment 27

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox 107.0RC1 on Windows 10 x64, macOS 11.6 and on Ubuntu 20.04 x64.