Closed Bug 972359 Opened 6 years ago Closed 6 years ago

Panels/subviews always have a scrollbar

Categories

(Firefox :: Toolbars and Customization, defect)

All
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: ge3k0s, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [Australis:P2])

Attachments

(1 file, 3 obsolete files)

When you put the history widget in the navbar and open the panel with few history items a scrollbar appears for no reason since the items are not overlapping.
Whiteboard: [Australis:P3]
Duplicate of this bug: 972517
Summary: History panel always has a scrollbar → Panels/subviews always have a scrollbar
Whiteboard: [Australis:P3] → [Australis:P2]
Status: UNCONFIRMED → NEW
Ever confirmed: true
I think this is a recent regression... is that just my impression?
Regression range:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=802d87c77e76&tochange=a62bde1d6efe

I'll have a more detailed look in the morning.
Keywords: regression
I lied. Locally a backout of bug 970897 fixes this. Which means maybe my duping bug 972517 here wasn't correct?

Anyway, wonder why it doesn't work. It's probably something to do with the height calculations not actually being quite correct because of padding/border/margin on header/body/footer and such :-(
Blocks: 970897
So I'm going a little crazy looking at this... basically, the issue is we're not taking padding/margin/borders into account. So I added code to deal with this (and then there's the paddings on subviews element, negative margins on subview body stuff in the main panel... sigh). However, I'm seeing weird things. In my standalone history menu, the body height is 532px initially, but going by that and not including the border of the footer means it gets a scrollbar for exactly 1px worth of scrolling. Including the border of the footer means the body height goes to 533px the next time _syncContainerWithMainView is invoked, after which it goes to 534, 535, and so on, and because we happen to invoke the method 4 times (sigh...) we end up with the view stretched so that it ends up occupying 4-5px too much space (which ends up as dead space at the bottom). I don't understand what's going on here. The footer border is 1px wide, so including it makes sense, but then it doesn't make sense that the size of the body keeps growing and growing... maybe I'm missing a bit of padding/border/margin somewhere? Not sure how that'd help in the misreporting that the subview body does by saying it's 532 initially when it really is 533...
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(In reply to :Gijs Kruitbosch from comment #5)
> Created attachment 8377225 [details] [diff] [review]
> attempt to use margins and border
> 
> So I'm going a little crazy looking at this... basically, the issue is we're
> not taking padding/margin/borders into account. So I added code to deal with
> this (and then there's the paddings on subviews element, negative margins on
> subview body stuff in the main panel... sigh). However, I'm seeing weird
> things. In my standalone history menu, the body height is 532px initially,
> but going by that and not including the border of the footer means it gets a
> scrollbar for exactly 1px worth of scrolling. Including the border of the
> footer means the body height goes to 533px the next time
> _syncContainerWithMainView is invoked, after which it goes to 534, 535, and
> so on, and because we happen to invoke the method 4 times (sigh...) we end
> up with the view stretched so that it ends up occupying 4-5px too much space
> (which ends up as dead space at the bottom). I don't understand what's going
> on here. The footer border is 1px wide, so including it makes sense, but
> then it doesn't make sense that the size of the body keeps growing and
> growing... maybe I'm missing a bit of padding/border/margin somewhere? Not
> sure how that'd help in the misreporting that the subview body does by
> saying it's 532 initially when it really is 533...

So this is due to rounding (wheee). The problem is that MDN suggests getBoundingClientRect() as the non-rounding equivalent, but that doesn't work because it always uses the outer rect, not the inner rect which scrollHeight uses for overflow: auto elements, so then we never enlarge the menu panel anymore. :-(
This works, it seems. I removed one unnecessary call to syncContainerToMainView... we should probably investigate the whole lot again for 1.1 because this code gets run a *lot* and I suspect all the reflows aren't particularly helpful for perf, even if it's not really critical path...
Attachment #8380686 - Flags: review?(mconley)
Attachment #8377225 - Attachment is obsolete: true
IRC pointed out the previous solution was still somewhat flawed. This is better, even if still imperfect, but also has a comment explaining the dilemma here.
Attachment #8380716 - Flags: review?(mconley)
Attachment #8380686 - Attachment is obsolete: true
Attachment #8380686 - Flags: review?(mconley)
Comment on attachment 8380716 [details] [diff] [review]
Australis - use margins and border for measuring panel heights to avoid unnecessary scrollbars,

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

A few behavioural problems after I applied this patch:

Problem 1 - Panel does not get resized when customizing wide items out makes it shorter:
1) Enter customize mode
2) Remove almost everything from the menu panel except one row of icons. Make sure to remove either the zoom controls or edit controls or both.
3) Exit customize mode
4) Open the menu panel

ER: Panel should shrink-wrap the remaining icons in the menu
AR: Panel is the same size as before customization occurred.

Problem 2 - Short panels are not expanded if the zoom controls or edit controls are moved into the panel
1) Repeat the steps from Problem 1
2) Restart the browser
3) Open the menu panel - the panel should properly shrink-wrap the items in it
4) Enter customize mode
5) Choose to Restore Defaults
6) Exit customize mode
7) Open the menu panel

ER: The panel should have been expanded to fit the contents to the max height available on the screen
AR: The panel is still tiny, and all of the contents are there, but within a tiny scrollable region.


I highly suspect both problems are related to the removal of the _syncContainerWithMainView call in the popupshowing handler - putting that line back makes the problem go away. At this late stage of the game, I'd opt for putting it back. We might lose out on some perf, but as you say, opening the menu panel is not a hot path.
Attachment #8380716 - Flags: review?(mconley) → feedback+
Hrmpf. I just added that line back and tried again, and notice the following:

1. Move history to toolbar
2. Restart browser
3. open history panel --> no scrollbar
4. close history panel
5. right click, move to panel
6. open panel, right click history, move to toolbar
7. reopen history panel --> scrollbar


:'-(

I'll look at this some more tomorrow.
Needed to round in order for things to work out correctly. I also tidied up so that if we ever add a border on the subview body, that should work. This has so far managed all my testing...
Attachment #8382091 - Flags: review?(mconley)
Attachment #8380716 - Attachment is obsolete: true
Comment on attachment 8382091 [details] [diff] [review]
Australis - use margins and border for measuring panel heights to avoid unnecessary scrollbars,

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

Yes, this works much better! Thanks!
Attachment #8382091 - Flags: review?(mconley) → review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/b89988fc261d
Whiteboard: [Australis:P2] → [Australis:P2][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b89988fc261d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P2][fixed-in-fx-team] → [Australis:P2]
Target Milestone: --- → Firefox 30
I'm not sure if fix was already deployed or not but it doesn't work for me on Nightly 30. I'm using code below to scale down subviews a little bit because it's insanely huge by default.

#PanelUI-history label:first-child,
#PanelUI-helpView label:first-child,
#PanelUI-developer label:first-child,
#PanelUI-characterEncodingView label:first-child { display:none !important; }

#PanelUI-history *,
#PanelUI-bookmarks *,
#PanelUI-helpView *,
#PanelUI-developer *,
#PanelUI-characterEncodingView * {
  margin-top: 1px !important;
  margin-bottom: 1px !important;
  padding-top: 0px !important;
  padding-bottom: 0px !important;
}

Result is following:

http://fii.cz/swcsej
http://fii.cz/ebszpmh

Could you do something about scrollbar showing when using custom padding/margin and running without certain items like titles?.
(In reply to Roman Müller from comment #15)
> I'm not sure if fix was already deployed or not but it doesn't work for me
> on Nightly 30. I'm using code below to scale down subviews a little bit
> because it's insanely huge by default.
> 
> #PanelUI-history label:first-child,
> #PanelUI-helpView label:first-child,
> #PanelUI-developer label:first-child,
> #PanelUI-characterEncodingView label:first-child { display:none !important; }
> 
> #PanelUI-history *,
> #PanelUI-bookmarks *,
> #PanelUI-helpView *,
> #PanelUI-developer *,
> #PanelUI-characterEncodingView * {
>   margin-top: 1px !important;
>   margin-bottom: 1px !important;
>   padding-top: 0px !important;
>   padding-bottom: 0px !important;
> }
> 
> Result is following:
> 
> http://fii.cz/swcsej
> http://fii.cz/ebszpmh
> 
> Could you do something about scrollbar showing when using custom
> padding/margin and running without certain items like titles?.

You need to wait for today's nightly. The fix was landed yesterday, as can be seen in comment #14, but well after a nightly was created, and today's nightly isn't ready yet. I just tried your CSS additions with current fx-team tip, and they work. Please don't scare me like this again...

(and between the two of us, those universal selectors combined with a descendant selector are the slowest bit of CSS I've seen in my life as a front-end developer... you might want to think about doing something less dramatic (and more specific and performant))
(In reply to :Gijs Kruitbosch from comment #16)
> You need to wait for today's nightly. The fix was landed yesterday, as can
> be seen in comment #14, but well after a nightly was created, and today's
> nightly isn't ready yet. I just tried your CSS additions with current
> fx-team tip, and they work. Please don't scare me like this again...
> 
> (and between the two of us, those universal selectors combined with a
> descendant selector are the slowest bit of CSS I've seen in my life as a
> front-end developer... you might want to think about doing something less
> dramatic (and more specific and performant))

The code I posted is someone else's work. I don't understand to coding at all therefore I will have to stick to what I got even tho it might be the worst piece of code you have ever seen :)

Anyway thx for reply and sorry for posting unnecessary comment.
Comment on attachment 8382091 [details] [diff] [review]
Australis - use margins and border for measuring panel heights to avoid unnecessary scrollbars,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis' panel footer implementations
User impact if declined: panels always have scrollbars, looks jarring
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): reasonably low. Left to bake on m-c for a bit, no regressions reported (unlike for some of the previous work in this direction). Not sure what other options we have (short of getting rid of the footers entirely, but that doesn't seem sensible).
String or IDL/UUID changes made by this patch: none
Attachment #8382091 - Flags: approval-mozilla-aurora?
Attachment #8382091 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Contact: cornel.ionce
Verified as fixed on latest Nightly and latest Aurora builds.
Build ID Fx 29: 20140313004000
Build ID Fx 30: 20140313030202
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.