1247824 - Persistent excess space in bookmark panel from folder/tag list

Status: RESOLVED FIXED

(Core :: XUL, defect, P4)

Description

[Justin Dolske [:Dolske]]

Attachment: 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.

Comment 1

[[e-mail]]

Can't reproduce on nightly Linux (48) & Mac developer edition (47), 
can you still reproduce the issue?

Comment 2

[Justin Dolske [: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.)

Comment 3

[Justin Dolske [:Dolske]]

See also bug 814565.

Comment 4

[YF (Yang)]

Dupe of bug 1200045?

Comment 6

[Justin Dolske [:Dolske]]

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.

Comment 7

[Dão Gottwald [:dao]]

(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.

Comment 8

[Dão Gottwald [:dao]]

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.

Comment 9

[Dão Gottwald [:dao]]

(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?

Comment 10

[Neil Deakin]

Attachment: Don't set the popup's size unless the attributes exist

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.

Comment 12

[Dão Gottwald [:dao]]

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.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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?

Comment 14

[Dão Gottwald [:dao]]

(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 ;)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/c03e4314339f
Don't set the popup's size unless the width/height attributes exist. r=masayuki

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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!

Comment 17

[Neil Deakin]

In future, please assume that I, as patch author, will request review when needed.

Comment 18

[Dão Gottwald [:dao]]

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.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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...

Comment 20

[Neil Deakin]

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.

Comment 21

[Carsten Book [:Tomcat]]

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