1012920 - Panel size not correctly calculated when filled with HTML content

Status: NEW

(Core :: XUL, defect)

Description

[Chris Tomlinson]

Attachment: panelScrollSizeMiscalculation1.png

The attached screenshots show three examples of some problems with the size of popup panels generated by my add-on[1].

I'm not sure what causes this or if there is more than one bug involved but it might be worth noting that the contents of the panel include XHTML input fields and lists.

Screenshot 1 and 2 show the panel reaching the bottom of the screen (as far as the Windows 7 taskbar); both are rendering a scroll bar that indicates the visible area is displaying almost all of the contents of the panel when in fact there is a lot of content still displayed at the bottom of the screen. Neither render the bottom arrow of the scrollbar correctly. Screenshot 2 shows the scrollbar at its lowest point (no dragging or mouse-wheeling will make it scroll more content into view).

Screenshot 3 shows that the inaccuracy is not limited to panels which reach the bottom of the screen. It also shows that the width is incorrect (cutting off the end of the word "password"), presumably because no allowance was made for the unexpected existence of the vertical scrollbar.

[1]: KeeFox - https://addons.mozilla.org/en-US/firefox/addon/keefox/

Comment 1

[Chris Tomlinson]

Attachment: panelScrollSizeMiscalculation2.png

Comment 2

[Chris Tomlinson]

Attachment: panelScrollSizeMiscalculation3.png

Comment 3

[Chris Tomlinson]

I've been hacking around in the browser toolbox and found that at least two factors contribute to the incorrect size being calculated.

1) Adding vertical margins to input button elements (maybe other element types too)

2) The height of rendered text within a div is ignored. Even specifying a fixed height for the div doesn't help (presumably because the height calculation determines that the div is empty so applies zero height)

Comment 4

[:Gijs (he/him)]

Egh, I've been meaning to look into this in more detail but I keep not finding time. Can you help me along by answering these questions:

- These are all using panelviews and PanelUI methods to show them as single panels, right? Or are they plain xul <panel>s that you create?
- Do these issues show up when the subview is inside the main menu panel?
- Are you trying to reproduce with 29 or with Nightly, and can you check in 30, 31 and 32? I believe Neil Deakin landed some fixes for the subviews recently that should help here.

Comment 5

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #4)
> Egh, I've been meaning to look into this in more detail but I keep not
> finding time. Can you help me along by answering these questions:
> 
> - These are all using panelviews and PanelUI methods to show them as single
> panels, right? Or are they plain xul <panel>s that you create?
> - Do these issues show up when the subview is inside the main menu panel?
> - Are you trying to reproduce with 29 or with Nightly, and can you check in
> 30, 31 and 32? I believe Neil Deakin landed some fixes for the subviews
> recently that should help here.

This time with needinfo. Sorry for not being much help so far, but I'll do my best to keep this on my radar and work towards a solution.

Comment 6

[Chris Tomlinson]

Yes it's all panelview elements, no panel elements.

The issues are when adding the panelview to a CustomizableUI widget view via the standard CustomizableUI API. I don't see the problem when the panelview is within the Firefox main menu.

I've managed to reduce the number of problems by setting flex="1" on the panelview and setting display:inline-flex on the input[type=button] elements.

The rough markup structure is <panelview flex=1><div>text</div><div><input type="button"/></div></panelview>.

I don't really understand why it is necessary to switch to flex display modes for the height to be calculated correctly but that might be a limitation of my CSS understanding rather than an actual bug.

After enabling this flex mode I find that most problems are gone even in 29. I think that with the current code[1] the bug fixed in #994194 causes FF30 to behave differently to 29. In 29, the behaviour of small panelviews is OK but once their contents grow, the container never shrinks again (because that height attribute has been set?). In 30, some small panelviews are not given enough room by the outer popup/overlay container so they demonstrate the problem shown in screenshot 3.

As mentioned above, not all small panelviews are affected - it particularly affects panelviews that contain text or input buttons with vertical margins (the latter problem is avoided by the use of display:inline-flex).

In 30&31 there is a new problem where the scroll bars do not disappear when the panel content shrinks. Superficially at least, this looks like the dimensions of the outer panelview container are being calculated correctly but only on the assumption that no vertical scrollbar takes up content space.

Since the existing vertical scrollbar does not get removed, the last words of some lines of text wrap onto the next line, thus requiring the existence of a vertical scrollbar in a rather mind-bending catch-22 which I'm sure has caused challenges for various parts of the Firefox UI many times in the past. The problem only seems to appear when there is text visible (unlike the similar problem above, this also happens when the text is contained within a <div><ul><li> structure).

I can't find a FF32 build at the moment but have tested 33 and see no difference compared to 31.

I'm still trying to find time to come up with a simpler repro add-on but have had to concentrate on just finding any way I can to get the add-on working for FF30 users (once AMO accept v1.4.2 the flex styling should solve the worst of the issues).

Wrapping the panelview contents in a single wrapper div seems to help initially but as soon as it becomes necessary to display scrollbars, a double set of vertical bars appears so I've stuck with the separate elements for v1.4.2.

Just to be explicit, some combination of the changes I've made and other bug fixes appear to have resolved the problems with screenshot 1 and 2, but the symptoms shown in 3 still persist.

Thanks for keeping an eye on this issue, I appreciate it's a particularly complicated mix of symptoms, changing behaviour and no doubt a fair bit of user error too!

[1] https://github.com/luckyrat/KeeFox/tree/v1.4.2/Firefox%20addon/KeeFox
https://github.com/luckyrat/KeeFox/blob/v1.4.2/Firefox%20addon/KeeFox/chrome/content/panel.js
https://addons.mozilla.org/en-US/firefox/addon/keefox/versions/1.4.2

Comment 7

[:Gijs (he/him)]

(In reply to Chris Tomlinson from comment #6)
> Yes it's all panelview elements, no panel elements.
> 
> The issues are when adding the panelview to a CustomizableUI widget view via
> the standard CustomizableUI API. I don't see the problem when the panelview
> is within the Firefox main menu.
> 
> I've managed to reduce the number of problems by setting flex="1" on the
> panelview and setting display:inline-flex on the input[type=button] elements.

Ah. So the first I would have expected. The second, not so much... 

> The rough markup structure is <panelview flex=1><div>text</div><div><input
> type="button"/></div></panelview>.
> 
> I don't really understand why it is necessary to switch to flex display
> modes for the height to be calculated correctly but that might be a
> limitation of my CSS understanding rather than an actual bug.

I don't know for sure, but all the builtin panelviews all use flex="1", too, so I would accept that part as a fact of life for now... the inline-flex shouldn't be necessary, and I'm not sure why it is.


> After enabling this flex mode I find that most problems are gone even in 29.
> I think that with the current code[1] the bug fixed in #994194 causes FF30
> to behave differently to 29. In 29, the behaviour of small panelviews is OK
> but once their contents grow, the container never shrinks again (because
> that height attribute has been set?). In 30, some small panelviews are not
> given enough room by the outer popup/overlay container so they demonstrate
> the problem shown in screenshot 3.
> 
> As mentioned above, not all small panelviews are affected - it particularly
> affects panelviews that contain text or input buttons with vertical margins
> (the latter problem is avoided by the use of display:inline-flex).

Ah. Still, that's kind of odd. :-\


> In 30&31 there is a new problem where the scroll bars do not disappear when
> the panel content shrinks. Superficially at least, this looks like the
> dimensions of the outer panelview container are being calculated correctly
> but only on the assumption that no vertical scrollbar takes up content space.

This is odd as well. AFAIK in the toolbar, we no longer set the height (that was what bug 994194 did). So I don't know why the scrollbars would stay... I think the panel would remain the same size unless you told it to resize itself, and so the flexed panelview would do the same, but if the content inside the panelview would be smaller, AFAICT that should cause the scrollbars to go away...

> Since the existing vertical scrollbar does not get removed, the last words
> of some lines of text wrap onto the next line, thus requiring the existence
> of a vertical scrollbar in a rather mind-bending catch-22 which I'm sure has
> caused challenges for various parts of the Firefox UI many times in the
> past. The problem only seems to appear when there is text visible (unlike
> the similar problem above, this also happens when the text is contained
> within a <div><ul><li> structure).
> 
> I can't find a FF32 build at the moment but have tested 33 and see no
> difference compared to 31.
> 
> I'm still trying to find time to come up with a simpler repro add-on but
> have had to concentrate on just finding any way I can to get the add-on
> working for FF30 users (once AMO accept v1.4.2 the flex styling should solve
> the worst of the issues).

Yeah, that would be helpful.

> Wrapping the panelview contents in a single wrapper div seems to help
> initially but as soon as it becomes necessary to display scrollbars, a
> double set of vertical bars appears so I've stuck with the separate elements
> for v1.4.2.
> 
> Just to be explicit, some combination of the changes I've made and other bug
> fixes appear to have resolved the problems with screenshot 1 and 2, but the
> symptoms shown in 3 still persist.

OK, so 3 is the most important. I'll try to make time to look at this either this week or the next. Setting needinfo to keep track of that...

Comment 8

[Chris Tomlinson]

Attachment: panelsizebug.xpi to help reproduce some of the panel size issues

This XPI might help identify the problem.

There are 5 widget buttons demonstrating various ways to construct panelviews, some of which have problems with scrollbars and some of which have other restrictions in order to work around the problems with scrollbars.

It doesn't demo issues related to growing and shrinking the content but I suspect that:
1) It is sensible to try to focus on one issue at a time
2) It shouldn't be too hard to make one of buttons dynamically adjust the height of some component (maybe itself) in order to investigate the problem where the scroll bars do not disappear when the panel content shrinks.

I can't replicate the need to use display:inline-flex on the buttons. I'm not sure why I found that workaround was necessary in my add-on but I'll forget about it for the time being and see if I can still reproduce it once the other issue(s) with scrollbars have been resolved.

Comment 9

[:Gijs (he/him)]

(Sigh... I'm sorry, hopefully I will be able to finally look at this tomorrow)

Comment 10

[:Gijs (he/him)]

As far as I can tell this is an issue with the popup sizing code...

Comment 11

[:Gijs (he/him)]

We no longer set the height of toolbar-based popups in Firefox front-end code (on trunk), and the fact that the popups are still not the right size points to a core issue... I'm trying to figure out some more details right now, but I'm having trouble making sense of the layout code in terms of what determines the size...

Comment 12

[:Gijs (he/him)]

So presumably this is failing in GetPrefSize which doesn't deal with HTML-in-XUL well. AFAICT it decides in the first panel that the three divs are 39.5, 39.5 and 21.5 pixels high each. The first div is actually ~67 pixels high. I don't know why it gets confused.