Closed Bug 1463687 Opened 6 years ago Closed 6 years ago

Have viewport uses scrollbar color properties on root element

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(1 file)

Bug 1463687 - Apply scrollbar style of root element to scrollbars of viewport.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
Based on feedback from https://github.com/devtools-html/rfcs/issues/27#issuecomment-391251478 we need a way to style scrollbars of the viewport.

The normal way is to propagate the properties from root element to the viewport, similar to what we do for overflow properties (although we probably don't want to propagate <body> to viewport like overflow).
Agreed and spec updated accordingly. Thanks for raising/filing!
Priority: -- → P3
Assignee: nobody → xidorn+moz
Comment on attachment 8980193 [details]
Bug 1463687 - Apply scrollbar style of root element to scrollbars of viewport.

https://reviewboard.mozilla.org/r/246358/#review252456

::: layout/base/nsLayoutUtils.h:3117
(Diff revision 1)
> +   * Generally it returns the computed style of the scrollable frame
> +   * which contains the given part frame. If the scrollable frame is

"scroll frame"

::: layout/base/nsLayoutUtils.cpp:10194
(Diff revision 1)
> +  nsIFrame* scrollableFrame = nullptr;
> +  for (nsIFrame* f = aScrollbarPart; f; f = f->GetParent()) {
> +    if (f->IsScrollFrame()) {
> +      scrollableFrame = f;
> +      break;
> +    }
> +  }

This can be:

nsIFrame* scrollableFrame = GetClosestFrameOfType(aFrame, LayoutFrameType::Scroll);

I would probably call it "scrollFrame" rather than "scrollableFrame", since "scrollable" sounds to me like it might be referring to the child of the scroll frame.

::: layout/base/nsLayoutUtils.cpp:10211
(Diff revision 1)
> +      // from the element. Dropping the strong reference is fine because
> +      // the style should be hold strongly by the element.

"held"

::: layout/base/nsLayoutUtils.cpp:10213
(Diff revision 1)
> +        return frameOfRoot->Style();
> +      }
> +      // If the root doesn't have primary frame, get the computed style
> +      // from the element. Dropping the strong reference is fine because
> +      // the style should be hold strongly by the element.
> +      RefPtr<ComputedStyle> style = pc->StyleSet()->ResolveServoStyle(root);

It would be nice if we could assert that the refcount is greater than 1 here, but since it would need adding a new FFI function, it's maybe too much effort.
Attachment #8980193 - Flags: review?(cam) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ac922efad9fd
Apply scrollbar style of root element to scrollbars of viewport. r=heycam
Comment on attachment 8980193 [details]
Bug 1463687 - Apply scrollbar style of root element to scrollbars of viewport.

Probably worth another look.
Flags: needinfo?(xidorn+moz)
Attachment #8980193 - Flags: review+ → review?(cam)
Comment on attachment 8980193 [details]
Bug 1463687 - Apply scrollbar style of root element to scrollbars of viewport.

https://reviewboard.mozilla.org/r/246358/#review252778

::: layout/base/nsLayoutUtils.cpp:10201
(Diff revision 3)
> +  while (content && content->IsInNativeAnonymousSubtree()) {
> +    content = content->GetParent();
> +  }

In theory we could have an XBL binding attached to NAC element, and that binding creates a scrollable element.  It's unlikely we have cases like this where we want to control the scrollbar colors from inside the binding, but might be worth mentioning in the comment.

::: layout/base/nsLayoutUtils.cpp:10219
(Diff revision 3)
> +  RefPtr<ComputedStyle> style = aScrollbarPart->
> +    PresContext()->StyleSet()->ResolveServoStyle(content->AsElement());

Can you add an assertion here that content is the document's root element?
Attachment #8980193 - Flags: review?(cam) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a3adfabc2b66
Apply scrollbar style of root element to scrollbars of viewport. r=heycam
https://hg.mozilla.org/mozilla-central/rev/a3adfabc2b66
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Blocks: 1464744
Depends on: 1490037
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: