Closed Bug 1393603 Opened 7 years ago Closed 7 years ago

stylo: Handle overflow:scroll for viewport units

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

enhancement

Tracking

()

RESOLVED WONTFIX

People

(Reporter: manishearth, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

These affect the viewport unit calculation (see CalcViewportUnitsScale in nsRuleNode.cpp)
Comment on attachment 8900908 [details] Bug 1393603 - Handle overflow:scroll for viewport units; https://reviewboard.mozilla.org/r/172342/#review177640 The reason I didn't do this when I implemented vh / vw was because I found no trivial way to do this. This is the trivial way, but `CalcViewportUnitsScale` calls `QueryFrame` into the root frame and all that stuff, so probably it's a no-go. We can probably change that queryframe to be a static cast, and assert only on the main thread. Even with that, we need to figure out how to this in a thread-safe way: http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/layout/generic/nsGfxScrollFrame.cpp#3860, which sounds even harder. So we need to somehow get and cache that state on the main thread and store it in the pres context or something. It seems to me that having state that depends on the frame tree for styling is quite bogus on its own (since changing the height of something with viewport units may alter the scrollbar styles!). But if we want to do this, this is definitely not the way.
Attachment #8900908 - Flags: review-
I'd really want to blame the Gecko code, and see which spec mandates this, and what's its state. I'd prefer not doing this tbh.
emilio says he has Plans for this
Assignee: manishearth → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(emilio+bugs)
Stylo behaviour matches Chromium and Safari, so I doubt this is going to be an issue. This was implemented in bug 811403, there are links to the CSSWG discussions there. However nobody else has implemented this, and I'm hesitant to do so. Gecko relies on the overflow on the root reframing the whole thing in order to do proper restyling on dynamic changes to the overflow property. I believe this may be bogus since bug 1344398. I'm still not sure how can you know the actual correct initial containing block width without layout information (or at least the width of the scrollbars). Also, I doubt WebKit and Blink will ever fix this, since they moved to a model where styling and layout is completely separated as well. So overall I think I'd be ok with regressing this... Somewhat unfortunate to be honest, but... David, Simon, since you were involved in the discussions related to this bug, do you think this is acceptable? I'll file an issue against the CSSWG to reconsider this decision.
(Or, alternatively, David, do you know a way to compute the scrollbar widths without relying on the frame tree? That way we can fix this properly too)
I guess the root frame always exists since the initialization of the pres shell, so we could rely on it... But still sounds like something we need to compute on the main thread or do painful thread-safety auditing, and then I want to make sure we handle dynamic changes for it properly (I'm trying to write some tests now). It'd be nice to keep this behaviour, but I'm also not sure how worth it is to spend a lot of cycles on it, given in practice it's not going to be an issue at all, and seems kinda gross... Having a good way to get the scrollbar horizontal / vertical sizes would make it easier, but having to get them from the XUL scrollbar boxes is kind of annoying :(
Comment on attachment 8900908 [details] Bug 1393603 - Handle overflow:scroll for viewport units; https://reviewboard.mozilla.org/r/172342/#review178242 Sounds like no need to review then.
Attachment #8900908 - Flags: review?(xidorn+moz)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #6) > (Or, alternatively, David, do you know a way to compute the scrollbar widths > without relying on the frame tree? That way we can fix this properly too) You could probably do it reasonably accurately with a combination of: LookAndFeel::GetInt(eIntID_UseOverlayScrollbars) and nsNativeTheme::GetMinimumWidgetSize(null, null, NS_THEME_SCROLLBAR_{HORIZONTAL,VERTICAL}, &result, &isOverridable); though you'd need to check that null for the first two parameters is OK on all platforms. That said, it's probably worth filing an issue in the csswg-drafts repo asking if any other implementers are planning to implement this.
Flags: needinfo?(dbaron)
Priority: -- → P3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Flags: needinfo?(simon.sapin)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: