Closed Bug 1199496 Opened 9 years ago Closed 9 years ago

The dialog(New Folder / New Bookmark) width is increased each time it pops up

Categories

(Toolkit :: Places, defect)

40 Branch
Unspecified
Windows
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla43
Tracking Status
firefox40 + wontfix
firefox41 + verified
firefox42 + verified
firefox43 + verified
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected

People

(Reporter: alice0775, Assigned: mak)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Steps To Reproduce:
1. Right click on Bookmarks tree / menu / toolbar
2. Choose "New Folder" or "New Bookmark"
3. Repeat Step.2

Actual Results:
The dialog width is increased each time it pops up.

Expected Results:
The dialog width should be same width.
Regression since Firefox40
[Tracking Requested - why for this release]: User cannot interact with the dialog.

Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=8a151d59bdec&tochange=d84b62b367b4

REGRESSED BY: d84b62b367b4	Asaf Romano — Bug 951651 - Make bookmarkProperties, Star UI and Library info pane work with PlacesTransactions. r=mak



This is critical issue.
Because, incresed 6px each time and finally the dialog protrude from the screen (see attached). And the dialog width cannot reduce without create new profile.
Blocks: 951651
Severity: normal → critical
Flags: needinfo?(asaf)
Version: Trunk → 40 Branch
Attached image screenshot
Marco, do you have time to look into this?
Flags: needinfo?(mak77)
Alice, as usual, you impress me with this kind of bug...
Anyway, tracking. It is not a critical issue per say but it doesn't give a good impression of Firefox if you are affected by this bug...
sounds like after some time this can completely disallow the user from using the dialog, so bad enough.

Since I'm a little bit overloaded, if anyone can contribute a patch, feel free to do.
Assignee: nobody → mak77
Flags: needinfo?(mak77)
this is due to bug 90276, we should not use sizeToContent cause it's bogus (it grows the dialog at every launch due to inner/outer differences).
Flags: needinfo?(asaf)
Attached patch patch v1 (obsolete) — Splinter Review
This should also solve the flickering problem, where fields appear and then disappear. It also tries to address the problem for users whose dialog resizer has gone out of screen.
let's see if Try likes it
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cdfd587bc14
Attachment #8654920 - Flags: review?(ttaubert)
Comment on attachment 8654920 [details] [diff] [review]
patch v1

Review of attachment 8654920 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/bookmarkProperties.js
@@ +349,5 @@
>  
> +        // If the folder picker is visible, the dialog can be resized and it
> +        // will persist the size across restarts.
> +        let height = window.outerHeight;
> +        let collapsed = aEvent.newValue;

Maybe: let collapsed = !!aEvent.newValue;

Makes it clearer, I think.

::: browser/components/places/content/editBookmarkOverlay.xul
@@ +24,5 @@
>          <column flex="1" id="editBMPanel_editColumn" />
>        </columns>
>        <rows id="editBMPanel_rows">
> +        <row id="editBMPanel_nameRow"
> +             align="center" 

Nit: trailing space
Attachment #8654920 - Flags: review?(ttaubert) → review+
Attached patch patch v2Splinter Review
Sorry, when looking at your comment, it suddenly came to my mind that mutation events are deprecated, so I decided to move to a MutationObserver... unfortunately that has a very different behavior (the notifications happen at a different time) and I had to refactor the patch a little bit.

The good thing is now the code is even more contained in a single place, there are no more sizeToContent calls, more comments and users will gain the benefit to be able to resize (and persist the width) of any bookmarks dialog.

The downside is that I need a new review :(
Attachment #8654920 - Attachment is obsolete: true
Attachment #8655925 - Flags: review?(ttaubert)
Comment on attachment 8655925 [details] [diff] [review]
patch v2

Review of attachment 8655925 [details] [diff] [review]:
-----------------------------------------------------------------

Looks even better with the MutationObserver to me,
Attachment #8655925 - Flags: review?(ttaubert) → review+
https://hg.mozilla.org/mozilla-central/rev/31b80e27600d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Comment on attachment 8655925 [details] [diff] [review]
patch v2

Approval Request Comment
[Feature/regressing bug #]: bug 951651
[User impact if declined]: every time a bookmark dialog is opened it grows by 6px, until it goes out of screen.
[Describe test coverage new/current, TreeHerder]: tested locally, Nightly
[Risks and why]: nothing specific we can predict
[String/UUID change made/needed]: none
Attachment #8655925 - Flags: approval-mozilla-beta?
Attachment #8655925 - Flags: approval-mozilla-aurora?
Comment on attachment 8655925 [details] [diff] [review]
patch v2

Given the size of this patch and lack of clear risk assessment, I do not feel comfortable uplifting to Beta41 now. Let's uplift to Aurora42 to stabilize for a few days and then decide on whether to uplift to Beta41 or not.
Attachment #8655925 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Alice 0775, could you please verify this fix works as expected on 09/05 Nightly or later? Thanks in advance.
Flags: needinfo?(alice0775)
I can confirm that the problem is fixed on Latest Noghtly43.0a1.
https://hg.mozilla.org/mozilla-central/rev/e816a7a854a333bda1447ab6c5b633233f322669
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0 ID:20150905030205
Flags: needinfo?(alice0775)
(In reply to Alice0775 White from comment #18)
> I can confirm that the problem is fixed on Latest Noghtly43.0a1.
> https://hg.mozilla.org/mozilla-central/rev/
> e816a7a854a333bda1447ab6c5b633233f322669
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0
> ID:20150905030205

Thanks Alice0775 for a prompt verification.
Comment on attachment 8655925 [details] [diff] [review]
patch v2

This patch has been in Aurora for 3 days now, and was verified on Nightly build, it seems safe to uplift to Beta41.
Attachment #8655925 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
sorry had to back this out for test failures on aurora like https://treeherder.mozilla.org/logviewer.html#?job_id=1115000&repo=mozilla-aurora
Flags: needinfo?(mak77)
Flags: needinfo?(mak77)
the only difference in browser_bookmarksProperties is bug 1178709, I think it's a good idea to uplift that test-only change.
(In reply to Marco Bonardo [::mak] from comment #22)
> the only difference in browser_bookmarksProperties is bug 1178709, I think
> it's a good idea to uplift that test-only change.

did a re-checkin togehter with the test only change, lets see if this fix the problem, thanks mak
(In reply to Carsten Book [:Tomcat] from comment #23)
> (In reply to Marco Bonardo [::mak] from comment #22)
> > the only difference in browser_bookmarksProperties is bug 1178709, I think
> > it's a good idea to uplift that test-only change.
> 
> did a re-checkin togehter with the test only change, lets see if this fix
> the problem, thanks mak

indeed this fixed the test failures!
Flags: qe-verify+
I was able to reproduce this issue on Firefox 40.0.3 and Firefox 42.0a1 (2015-08-09) under Windows 7 64-bit.

Verified fixed on Firefox 43.0a1 (2015-09-08), Firefox 42.0a2 (2015-09-09) and Firefox 41 Beta 8 (20150907144446) under Windows 7 64-bit.
Depends on: 1208063
You need to log in before you can comment on or make changes to this bug.