Closed Bug 1247824 Opened 4 years ago Closed 4 years ago

Persistent excess space in bookmark panel from folder/tag list

Categories

(Core :: XUL, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox47 --- affected
firefox50 --- fixed

People

(Reporter: Dolske, Assigned: enndeakin)

References

Details

Attachments

(2 files)

Attached image Screenshot
1) New profile
2) Click star to bookmark a page, show panel.
3) Expand the folder and tag lists via the [v] expander.
   (Aside: bug 1247823, where the folder list width may overflow)
4) Dismiss panel
5) Open panel again. Notice that the panel height remains as tall as it was in step 4, but the folder/tag lists are now unexpanded. This results in a large excess of empty space in the panel. The height should be reset to the default when the panel is dismissed.
Can't reproduce on nightly Linux (48) & Mac developer edition (47), 
can you still reproduce the issue?
Flags: needinfo?(dolske)
Yes, although the STR are a bit different.

1) New profile
2) Click star to bookmark a page, panel automatically opens (first time)
3) Expand Tags section, click outside panel to dismiss it.
4) Click star to show panel (second time)
5) Expand Tags section, click outside panel to dismiss it.
6) Click star to show panel (third time)

After Step 6 there is excess space in the panel.

(My earlier screenshot shows a even more space; I'd guess that expanding the Folder section has the same bugs and additively contributes to the extra space. That's more annoying to try to reproduce with, though, because bug 1247823 still cam make the expander arrows overflow outside the panel.)
Flags: needinfo?(dolske)
See also bug 814565.
Dupe of bug 1200045?
Duplicate of this bug: 1200045
From bug 1200045 comment 5:

Regression range may be https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9d2a7a8ca1c7&tochange=582d4c67b3a7

I don't see anything blindingly obvious there. If the range is good, bisecting into it will probably be needed.
(In reply to Justin Dolske [:Dolske] from comment #6)
> From bug 1200045 comment 5:
> 
> Regression range may be
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=9d2a7a8ca1c7&tochange=582d4c67b3a7
> 
> I don't see anything blindingly obvious there. If the range is good,
> bisecting into it will probably be needed.

Bug 768440 is in that range. Could somehow affect the bookmarks panel code I suppose. We animate the panel opening on Windows and OS X, not on Linux, and it looks like nobody could reproduce this on Linux yet.
Disabling the animation with animate="false" doesn't help on Windows. The problem seems to be that we persist the panel's height in the height attribute. I don't know where and why that happens. We don't seem to be doing this on Linux.
(In reply to Dão Gottwald [:dao] from comment #8)
> Disabling the animation with animate="false" doesn't help on Windows. The
> problem seems to be that we persist the panel's height in the height
> attribute. I don't know where and why that happens. We don't seem to be
> doing this on Linux.

Marco, do you know if some bookmarking code does that?

Alternatively, Enn, do you know if some XUL popup code might be responsible?
Flags: needinfo?(mak77)
Flags: needinfo?(enndeakin)
This was discussed in some other bug. The simplest fix is to just return early if the popup doesn't have width/height attributes.

The two cases where one might want to have popups resize, calling popup.sizeTo or putting a resizer in the popup set these attributes anyway.

This seems to work in my case.
Flags: needinfo?(enndeakin)
Flags: needinfo?(mak77)
Attachment #8759162 - Flags: review?(neil)
Duplicate of this bug: 1013682
Assignee: nobody → enndeakin
Component: Bookmarks & History → XP Toolkit/Widgets: Menus
Product: Firefox → Core
Comment on attachment 8759162 [details] [diff] [review]
Don't set the popup's size unless the attributes exist

Not quite sure if Neil does reviews these days.
Attachment #8759162 - Flags: review?(masayuki)
Comment on attachment 8759162 [details] [diff] [review]
Don't set the popup's size unless the attributes exist

Looks reasonable to me. But I'm not sure if this could cause regressions. How about asking Enn?
Attachment #8759162 - Flags: review?(masayuki) → review+
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) from comment #13)
> Comment on attachment 8759162 [details] [diff] [review]
> Don't set the popup's size unless the attributes exist
> 
> Looks reasonable to me. But I'm not sure if this could cause regressions.
> How about asking Enn?

He wrote the patch ;)
Attachment #8759162 - Flags: review?(neil)
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/c03e4314339f
Don't set the popup's size unless the width/height attributes exist. r=masayuki
(In reply to Dão Gottwald [:dao] from comment #14)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due
> to injured) from comment #13)
> > Comment on attachment 8759162 [details] [diff] [review]
> > Don't set the popup's size unless the attributes exist
> > 
> > Looks reasonable to me. But I'm not sure if this could cause regressions.
> > How about asking Enn?
> 
> He wrote the patch ;)

Oh, you just requested the review to me!
In future, please assume that I, as patch author, will request review when needed.
Were there any specific concerns with the patch or did you just want to give it another thought? I can't tell from comment 10. Should I back it out?

I wanted to move this bug along because it's part of a UX quality initiative regarding bookmarking that we want to wrap up this week, and requesting review (along with running mochitests on Try) seemed like the next logical step.
I believe that the approach is really right at least in technically, but some add-ons *might* depend on the bug. The reason why I worry because when I change Gecko's behavior for conforming web standards or making compatible with other browsers, I've gotten such unbelievable regression reports...
After some testing, I think the patch should be ok. The two ways one would typically resize a popup would be calling popup.sizeTo or putting a <resizer> inside the popup and both set the attributes directly.

If issues occur, one could set or clear the width/height attributes when a popup opens and/or closes as a workaround.
https://hg.mozilla.org/mozilla-central/rev/c03e4314339f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in before you can comment on or make changes to this bug.