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)
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)
405.37 KB,
image/png
|
Details | |
14.87 KB,
patch
|
ttaubert
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•9 years ago
|
||
Regression since Firefox40
status-firefox40:
--- → affected
status-firefox41:
--- → affected
status-firefox42:
--- → affected
status-firefox-esr31:
--- → unaffected
status-firefox-esr38:
--- → unaffected
Keywords: regression
Reporter | ||
Comment 2•9 years ago
|
||
[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
tracking-firefox40:
--- → ?
tracking-firefox41:
--- → ?
tracking-firefox42:
--- → ?
tracking-firefox43:
--- → ?
Flags: needinfo?(asaf)
Version: Trunk → 40 Branch
Reporter | ||
Comment 3•9 years ago
|
||
Comment 5•9 years ago
|
||
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...
Assignee | ||
Comment 6•9 years ago
|
||
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)
Assignee | ||
Comment 7•9 years ago
|
||
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)
Assignee | ||
Comment 8•9 years ago
|
||
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 9•9 years ago
|
||
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+
Assignee | ||
Comment 10•9 years ago
|
||
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 11•9 years ago
|
||
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+
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/31b80e27600d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Assignee | ||
Comment 14•9 years ago
|
||
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)
Reporter | ||
Comment 18•9 years ago
|
||
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+
Comment 21•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(mak77)
Assignee | ||
Comment 22•9 years ago
|
||
the only difference in browser_bookmarksProperties is bug 1178709, I think it's a good idea to uplift that test-only change.
Comment 23•9 years ago
|
||
(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
Comment 25•9 years ago
|
||
(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!
Updated•9 years ago
|
Flags: qe-verify+
Comment 27•9 years ago
|
||
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.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•