Closed Bug 1475033 Opened 6 years ago Closed 6 years ago

Add initial support of scrollbar-width

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: bgrins, Assigned: xidorn)

References

Details

(Keywords: dev-doc-needed)

Attachments

(9 files)

+++ This bug was initially created as a clone of Bug #1473298 +++

From https://bugzilla.mozilla.org/show_bug.cgi?id=1473298#c3:

> Yes, I agree the scrollbar should be both darker and narrower. I think the main use case for devs is to use a scroll wheel so it doesn't have to be as visible as e.g. a button.

This should be possible once scrollbar-width is implemented: https://drafts.csswg.org/css-scrollbars-1/#scrollbar-width
Component: Framework → CSS Parsing and Computation
Product: DevTools → Core
I thought the idea in the previous bug (this comment https://bugzilla.mozilla.org/show_bug.cgi?id=1473298#c4) was to keep the wide scrollbar in Windows because it matches the native scrollbars - is there something else here I'm missing?
(In reply to Victoria Wang [:victoria] from comment #1)
> I thought the idea in the previous bug (this comment
> https://bugzilla.mozilla.org/show_bug.cgi?id=1473298#c4) was to keep the
> wide scrollbar in Windows because it matches the native scrollbars - is
> there something else here I'm missing?

The way I read the discussion was that we didn't want to change the width in that bug not because we didn't want to change it in principle, but because it wasn't possible to do without falling back to styling scrollbars with XUL. For instance, "takes less space (esp. important in small panels)" is mentioned as a pro to the old implementation in https://github.com/devtools-html/rfcs/issues/27#issuecomment-344302947.
Oh I see! I guess I could go either way on this: It would help to have thinner scrollers especially with 3-pane mode (and maintaining the rectangular style would keep it looking pretty similar to native). I don't have a good sense of how important it is to have accurately native-feeling scrollers in Windows.
Priority: -- → P3
Assignee: nobody → xidorn+moz
Taking this bug as the initial step for scrollbar-width. Like scrollbar colors this support will need roll out platform by platform.
Summary: Implement scrollbar-width → Add initial support of scrollbar-width
Depends on: 1479995
Attached image screenshot
This is how scrollbar-width:thin looks like on Windows.
(Note that this patchset implemented scrollbar-width: none for all platforms, because we just need to hide scrollbars. But scrollbar-width: thin is only on Windows at the moment. The support for macOS is trivial to add and should come soon.)
Comment on attachment 8997771 [details]
Bug 1475033 part 2 - Add a ShowScrollbar enum to be used in ScrollReflowInput.

https://reviewboard.mozilla.org/r/261502/#review268502


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/generic/nsGfxScrollFrame.cpp:266
(Diff revision 1)
>    ScrollReflowInput(nsIScrollableFrame* aFrame,
> -                    const ReflowInput& aReflowInput) :
> -    mReflowInput(aReflowInput),
> +                    const ReflowInput& aReflowInput)
> +  : mReflowInput(aReflowInput)
>      // mBoxState is just used for scrollbars so we don't need to
>      // worry about the reflow depth here
> -    mBoxState(aReflowInput.mFrame->PresContext(), aReflowInput.mRenderingContext, 0),
> +  , mBoxState(aReflowInput.mFrame->PresContext(), aReflowInput.mRenderingContext, 0)

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

  , mBoxState(aReflowInput.mFrame->PresContext(), aReflowInput.mRenderingContext, 0)
                                                                                  ^~
                                                                                  nullptr
Comment on attachment 8997773 [details]
Bug 1475033 part 4 - Implement scrollbar-width: none.

https://reviewboard.mozilla.org/r/261506/#review268504


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/generic/nsGfxScrollFrame.cpp:266
(Diff revision 1)
>    ScrollReflowInput(nsIScrollableFrame* aFrame,
>                      const ReflowInput& aReflowInput)
>    : mReflowInput(aReflowInput)
>      // mBoxState is just used for scrollbars so we don't need to
>      // worry about the reflow depth here
>    , mBoxState(aReflowInput.mFrame->PresContext(), aReflowInput.mRenderingContext, 0)

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

  , mBoxState(aReflowInput.mFrame->PresContext(), aReflowInput.mRenderingContext, 0)
                                                                                  ^~
                                                                                  nullptr
Comment on attachment 8997771 [details]
Bug 1475033 part 2 - Add a ShowScrollbar enum to be used in ScrollReflowInput.

https://reviewboard.mozilla.org/r/261502/#review268802
Attachment #8997771 - Flags: review?(mstange) → review+
Comment on attachment 8997773 [details]
Bug 1475033 part 4 - Implement scrollbar-width: none.

https://reviewboard.mozilla.org/r/261506/#review268800

::: layout/generic/nsGfxScrollFrame.cpp:1106
(Diff revision 2)
> +    // root elements with "scrollbar-width: none" is already suppressed
> +    // in ScrollFrameHelper::CreateAnonymousContent.
> +    ComputedStyle* scrollbarStyle = nsLayoutUtils::StyleForScrollbar(this);
> +    auto scrollbarWidth = scrollbarStyle->StyleUIReset()->mScrollbarWidth;
> +    if (scrollbarWidth == StyleScrollbarWidth::None) {
> +      state.mVScrollbar = state.mHScrollbar = ShowScrollbar::Never;

Please use two separate statements for this.

::: layout/generic/nsGfxScrollFrame.cpp:4676
(Diff revision 2)
>    bool canHaveHorizontal;
>    bool canHaveVertical;
>    if (!mIsRoot) {
> +    if (mOuter->StyleUIReset()->mScrollbarWidth == StyleScrollbarWidth::None) {
> +      // If scrollbar-width is none, don't generate scrollbars.
> +      canHaveHorizontal = canHaveVertical = false;

Here too.
Attachment #8997773 - Flags: review?(mstange) → review+
Comment on attachment 8997771 [details]
Bug 1475033 part 2 - Add a ShowScrollbar enum to be used in ScrollReflowInput.

https://reviewboard.mozilla.org/r/261502/#review268898


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/generic/nsGfxScrollFrame.cpp:266
(Diff revision 2)
>    ScrollReflowInput(nsIScrollableFrame* aFrame,
> -                    const ReflowInput& aReflowInput) :
> -    mReflowInput(aReflowInput),
> +                    const ReflowInput& aReflowInput)
> +  : mReflowInput(aReflowInput)
>      // mBoxState is just used for scrollbars so we don't need to
>      // worry about the reflow depth here
> -    mBoxState(aReflowInput.mFrame->PresContext(), aReflowInput.mRenderingContext, 0),
> +  , mBoxState(aReflowInput.mFrame->PresContext(), aReflowInput.mRenderingContext, 0)

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

  , mBoxState(aReflowInput.mFrame->PresContext(), aReflowInput.mRenderingContext, 0)
                                                                                  ^~
                                                                                  nullptr
Comment on attachment 8997773 [details]
Bug 1475033 part 4 - Implement scrollbar-width: none.

https://reviewboard.mozilla.org/r/261506/#review268900


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: layout/generic/nsGfxScrollFrame.cpp:266
(Diff revision 2)
>    ScrollReflowInput(nsIScrollableFrame* aFrame,
>                      const ReflowInput& aReflowInput)
>    : mReflowInput(aReflowInput)
>      // mBoxState is just used for scrollbars so we don't need to
>      // worry about the reflow depth here
>    , mBoxState(aReflowInput.mFrame->PresContext(), aReflowInput.mRenderingContext, 0)

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

  , mBoxState(aReflowInput.mFrame->PresContext(), aReflowInput.mRenderingContext, 0)
                                                                                  ^~
                                                                                  nullptr
Comment on attachment 8997774 [details]
Bug 1475033 part 5 - Remove scrollbarbutton min-{width,height} rule.

https://reviewboard.mozilla.org/r/261508/#review269278
Attachment #8997774 - Flags: review?(jmathies) → review+
Comment on attachment 8997776 [details]
Bug 1475033 part 7 - Implement scrollbar-width: thin for Windows.

https://reviewboard.mozilla.org/r/261512/#review269282
Attachment #8997776 - Flags: review?(jmathies) → review+
Comment on attachment 8997775 [details]
Bug 1475033 part 6 - Have scrollbar auto colors resolve to the corresponding colors on Windows 10.

https://reviewboard.mozilla.org/r/261510/#review269280

::: widget/windows/nsNativeThemeWin.cpp:1968
(Diff revision 2)
>  }
>  
>  static nscolor
>  GetScrollbarFaceColorForAuto(ComputedStyle* aStyle)
>  {
> -  // Do we want to derive from scrollbar-track-color when possible?
> +  return NS_RGB(205, 205, 205);

Is this going to break the colors on Win7 and Win8?
Attachment #8997775 - Flags: review?(jmathies) → review-
Comment on attachment 8997775 [details]
Bug 1475033 part 6 - Have scrollbar auto colors resolve to the corresponding colors on Windows 10.

https://reviewboard.mozilla.org/r/261510/#review269280

> Is this going to break the colors on Win7 and Win8?

The color is only used when we are drawing custom scrollbars, and if we are drawing custom scrollbars, I don't think it matters too much for colors from system. Also Windows 7 uses a more complicated image as scrollbar which is already replaced by the flattened style when custom scrollbar is used.
Attachment #8997775 - Flags: review- → review?(jmathies)
Comment on attachment 8997770 [details]
Bug 1475033 part 1 - Rename nsChangeHint_CSSOverflowChange to nsChangeHint_ScrollbarChange.

https://reviewboard.mozilla.org/r/261500/#review269324

::: layout/base/nsChangeHint.h:225
(Diff revision 2)
>     * transform property set.
>     */
>    nsChangeHint_AddOrRemoveTransform = 1 << 27,
>  
>    /**
> -   * Indicates that the overflow-x and/or overflow-y property changed.
> +   * Indicates that whether the presence of scrollbar changed.

Slight wording tweak: Indicates that the presence of scrollbars might have changed.
Attachment #8997770 - Flags: review?(cam) → review+
Comment on attachment 8997772 [details]
Bug 1475033 part 3 - Add scrollbar-width property.

https://reviewboard.mozilla.org/r/261504/#review269326
Attachment #8997772 - Flags: review?(cam) → review+
Comment on attachment 8998097 [details]
Bug 1475033 part 8 - Add some basic tests for scrollbar-width.

https://reviewboard.mozilla.org/r/261604/#review269328
Attachment #8998097 - Flags: review?(cam) → review+
Blocks: 1469741
Jim, could you have another look at part 6 given the reply in comment 31? Patches here is blocking several other stuff, and it is pretty close to sunset of MozReview.
Flags: needinfo?(jmathies)
Comment on attachment 8997775 [details]
Bug 1475033 part 6 - Have scrollbar auto colors resolve to the corresponding colors on Windows 10.

https://reviewboard.mozilla.org/r/261510/#review269578

thanks for the clarification.
Attachment #8997775 - Flags: review?(jmathies) → review+
Flags: needinfo?(jmathies)
Thanks for reviewing!
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d301e82323de
part 1 - Rename nsChangeHint_CSSOverflowChange to nsChangeHint_ScrollbarChange. r=heycam
https://hg.mozilla.org/integration/autoland/rev/4e51b28e6b28
part 2 - Add a ShowScrollbar enum to be used in ScrollReflowInput. r=mstange
https://hg.mozilla.org/integration/autoland/rev/ca30b641d0ab
part 3 - Add scrollbar-width property. r=heycam
https://hg.mozilla.org/integration/autoland/rev/665ba9179f6f
part 4 - Implement scrollbar-width: none. r=mstange
https://hg.mozilla.org/integration/autoland/rev/76149510c0f0
part 5 - Remove scrollbarbutton min-{width,height} rule. r=jimm
https://hg.mozilla.org/integration/autoland/rev/1972d9de2b10
part 6 - Have scrollbar auto colors resolve to the corresponding colors on Windows 10. r=jimm
https://hg.mozilla.org/integration/autoland/rev/dbfd2f2016a4
part 7 - Implement scrollbar-width: thin for Windows. r=jimm
https://hg.mozilla.org/integration/autoland/rev/b63992dfe51d
part 8 - Add some basic tests for scrollbar-width. r=heycam
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12540 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
I have documented this property along with the Scrollbar color properties:

https://developer.mozilla.org/en-US/docs/Web/CSS/scrollbar-track-color
https://developer.mozilla.org/en-US/docs/Web/CSS/scrollbar-face-color
https://developer.mozilla.org/en-US/docs/Web/CSS/scrollbar-width

There are various bits of data waiting to be merged for the spec, and compat data. 

A review of the pages would be appreciated. Thanks!
Flags: needinfo?(xidorn+moz)
The compat data of scrollbar-width doesn't seem to be correct, specifically, Firefox 63 doesn't support it. Its support on Firefox is still behind pref layout.css.scrollbar-width.enabled. When I'm going to switch it on, I will send an intent to ship email to dev-platform, and you can update the data accordingly then.

Also I don't currently have plan to support the numeric value of scrollbar-width, and it seems operating system vendors (i.e. Microsoft and Apple) seem to be somehow strongly against that, so we may end up just have the three keyword values. That might be worth some mentioning (and maybe the compat data may want to include them).

Also please mind that there is a plan to merge the two color properties into one, see w3c/csswg-drafts#3065 and bug 1483075.

Otherwise, the pages look good to me, thanks!
Flags: needinfo?(xidorn+moz)
There is an outstanding PR waiting to be merged into MDN which includes the detail about the flag, thanks for the info regarding numeric values, I'll update that to include a note. If the two color values are merged I should find out about it so I'll flag it up in the data then. At the moment if anyone is looking at these properties they exist so I think worth have documented.
You need to log in before you can comment on or make changes to this bug.