Skip to content

Commit

Permalink
Reland: Implemented fluent style for select multiple (https://crrev.c…
Browse files Browse the repository at this point in the history
…om/c/1715553)

Revert: https://crrev.com/c/1735548
Issue leading to revert: https://crbug.com/990672

The original change introduced use of uninitialized memory.
This change adds the missing initialization.
The update is limited to native_theme_base.cc which can be seen in patchset 2.

TBR'ing the original reviewers except Elly (owner for the fix under native_theme).

Bug: 987292
Change-Id: Ia091807203df5a17a240238dd4968dc031aa1476

TBR=chrishtr@chromium.org,masonfreed@chromium.org,haraken@chromium.org,tkent@chromium.org

Change-Id: Ia091807203df5a17a240238dd4968dc031aa1476
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1747855
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Bo Cupp <pcupp@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#686175}
  • Loading branch information
BoCupp-Microsoft authored and Commit Bot committed Aug 12, 2019
1 parent 432b5e0 commit 2e9e096
Show file tree
Hide file tree
Showing 33 changed files with 361 additions and 50 deletions.
12 changes: 7 additions & 5 deletions chrome/browser/ui/libgtkui/native_theme_gtk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -421,11 +421,13 @@ SkColor NativeThemeGtk::GetSystemColor(ColorId color_id,
return color;
}

void NativeThemeGtk::PaintArrowButton(cc::PaintCanvas* canvas,
const gfx::Rect& rect,
Part direction,
State state,
ColorScheme color_scheme) const {
void NativeThemeGtk::PaintArrowButton(
cc::PaintCanvas* canvas,
const gfx::Rect& rect,
Part direction,
State state,
ColorScheme color_scheme,
const ScrollbarArrowExtraParams& arrow) const {
auto context = GetStyleContextFromCss(
GtkVersionCheck(3, 20)
? "GtkScrollbar#scrollbar #contents GtkButton#button"
Expand Down
3 changes: 2 additions & 1 deletion chrome/browser/ui/libgtkui/native_theme_gtk.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ class NativeThemeGtk : public ui::NativeThemeBase {
const gfx::Rect& rect,
Part direction,
State state,
ColorScheme color_scheme) const override;
ColorScheme color_scheme,
const ScrollbarArrowExtraParams& arrow) const override;
void PaintScrollbarTrack(cc::PaintCanvas* canvas,
Part part,
State state,
Expand Down
9 changes: 9 additions & 0 deletions content/child/webthemeengine_impl_default.cc
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,15 @@ static void GetNativeThemeExtraParams(
NativeThemeScrollbarOverlayColorTheme(
extra_params->scrollbar_thumb.scrollbar_theme);
break;
case WebThemeEngine::kPartScrollbarDownArrow:
case WebThemeEngine::kPartScrollbarLeftArrow:
case WebThemeEngine::kPartScrollbarRightArrow:
case WebThemeEngine::kPartScrollbarUpArrow:
native_theme_extra_params->scrollbar_arrow.zoom =
extra_params->scrollbar_button.zoom;
native_theme_extra_params->scrollbar_arrow.right_to_left =
extra_params->scrollbar_button.right_to_left;
break;
default:
break; // Parts that have no extra params get here.
}
Expand Down
6 changes: 6 additions & 0 deletions third_party/blink/public/platform/web_theme_engine.h
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,11 @@ class WebThemeEngine {
WebScrollbarOverlayColorTheme scrollbar_theme;
};

struct ScrollbarButtonExtraParams {
float zoom;
bool right_to_left;
};

union ExtraParams {
ScrollbarTrackExtraParams scrollbar_track;
ButtonExtraParams button;
Expand All @@ -157,6 +162,7 @@ class WebThemeEngine {
InnerSpinButtonExtraParams inner_spin;
ProgressBarExtraParams progress_bar;
ScrollbarThumbExtraParams scrollbar_thumb;
ScrollbarButtonExtraParams scrollbar_button;
};

virtual ~WebThemeEngine() {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@

input,
select,
select:-internal-list-box,
textarea {
background-color: #ffffff;
border-color: #cecece;
}

input:hover,
select:hover,
select:-internal-list-box:hover,
textarea:hover {
border-color: #9d9d9d;
}
Expand Down Expand Up @@ -103,3 +105,55 @@ meter::-webkit-meter-even-less-good-value {
background: #d83b01;
border-radius: 1px 0px 0px 1px;
}

/* -internal-list-box is how we specify select[multiple] */
/* select[multiple] is an exception to the prohibition on sizes here
because it is one of the few controls with borders that grow on zoom
(to solve a nasty scrollbar location problem) */
select:-internal-list-box {
border-radius: 2px;
}

/* These options only apply to in-page 'multiple' select elements.
The options in the popup are handled in listPicker.css */
select:-internal-list-box option, select:-internal-list-box optgroup {
padding: 0 3px;
}

select:-internal-list-box option {
border-radius: 2px;
}

select:-internal-list-box option:hover {
background-color: #f3f3f3;
}

/* option selected */
select:-internal-list-box:focus option:checked,
select:-internal-list-box:focus option:checked:hover,
select:-internal-list-box option:checked,
select:-internal-list-box option:checked:hover {
background-color: #cecece !important;
color: #101010 !important;
}

/* option disabled */
select:-internal-list-box option:disabled,
select:-internal-list-box option:disabled:hover,
select:-internal-list-box:disabled option,
select:-internal-list-box:disabled option:hover {
background-color: #ffffff !important;
}

/* option both disabled and selected */
option:checked:disabled,
option:checked:disabled:hover,
select:-internal-list-box:focus option:checked:disabled,
select:-internal-list-box:focus option:checked:disabled:hover,
select:-internal-list-box:disabled option:checked,
select:-internal-list-box:disabled option:checked:hover,
select:-internal-list-box option:checked:disabled,
select:-internal-list-box option:checked:disabled:hover {
background-color: #f0f0f0 !important;
color: #c5c5c5 !important;
}
13 changes: 6 additions & 7 deletions third_party/blink/renderer/core/layout/layout_scrollbar.cc
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,9 @@ LayoutScrollbar::LayoutScrollbar(ScrollableArea* scrollable_area,
: Scrollbar(scrollable_area,
orientation,
kRegularScrollbar,
style_source,
nullptr,
LayoutScrollbarTheme::GetLayoutScrollbarTheme()),
style_source_(style_source) {
LayoutScrollbarTheme::GetLayoutScrollbarTheme()) {
DCHECK(style_source);

// FIXME: We need to do this because LayoutScrollbar::styleChanged is called
Expand Down Expand Up @@ -104,7 +104,6 @@ int LayoutScrollbar::HypotheticalScrollbarThickness(
}

void LayoutScrollbar::Trace(blink::Visitor* visitor) {
visitor->Trace(style_source_);
Scrollbar::Trace(visitor);
}

Expand Down Expand Up @@ -153,11 +152,11 @@ void LayoutScrollbar::SetPressedPart(ScrollbarPart part,
scoped_refptr<ComputedStyle> LayoutScrollbar::GetScrollbarPseudoStyle(
ScrollbarPart part_type,
PseudoId pseudo_id) {
if (!style_source_->GetLayoutObject())
if (!StyleSource()->GetLayoutObject())
return nullptr;
return style_source_->StyleForPseudoElement(
return StyleSource()->StyleForPseudoElement(
PseudoStyleRequest(pseudo_id, this, part_type),
style_source_->GetLayoutObject()->Style());
StyleSource()->GetLayoutObject()->Style());
}

void LayoutScrollbar::UpdateScrollbarParts(bool destroy) {
Expand Down Expand Up @@ -280,7 +279,7 @@ void LayoutScrollbar::UpdateScrollbarPart(ScrollbarPart part_type,
LayoutScrollbarPart* part_layout_object = parts_.at(part_type);
if (!part_layout_object && need_layout_object && scrollable_area_) {
part_layout_object = LayoutScrollbarPart::CreateAnonymous(
&style_source_->GetDocument(), scrollable_area_, this, part_type);
&StyleSource()->GetDocument(), scrollable_area_, this, part_type);
parts_.Set(part_type, part_layout_object);
SetNeedsPaintInvalidation(part_type);
} else if (part_layout_object && !need_layout_object) {
Expand Down
8 changes: 0 additions & 8 deletions third_party/blink/renderer/core/layout/layout_scrollbar.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,6 @@ class LayoutScrollbar final : public Scrollbar {
const LayoutBox& enclosing_box,
const LayoutObject& style_source);

// The Element that supplies our style information. If the scrollbar is
// for a document, this is either the <body> or <html> element. Otherwise, it
// is the element that owns our PaintLayerScrollableArea.
Element* StyleSource() const { return style_source_.Get(); }

IntRect ButtonRect(ScrollbarPart) const;
IntRect TrackRect(int start_length, int end_length) const;
IntRect TrackPieceRectWithMargins(ScrollbarPart, const IntRect&) const;
Expand Down Expand Up @@ -99,9 +94,6 @@ class LayoutScrollbar final : public Scrollbar {
scoped_refptr<ComputedStyle> GetScrollbarPseudoStyle(ScrollbarPart, PseudoId);
void UpdateScrollbarPart(ScrollbarPart, bool destroy = false);

// The element that supplies our style information.
Member<Element> style_source_;

HashMap<unsigned, LayoutScrollbarPart*> parts_;
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2444,13 +2444,17 @@ Scrollbar* PaintLayerScrollableArea::ScrollbarManager::CreateScrollbar(
scrollbar_size = LayoutTheme::GetTheme().ScrollbarControlSizeForPart(
style_source.StyleRef().EffectiveAppearance());
}
scrollbar = MakeGarbageCollected<Scrollbar>(ScrollableArea(), orientation,
scrollbar_size,
&ScrollableArea()
->GetLayoutBox()
->GetFrame()
->GetPage()
->GetChromeClient());
Element* style_source_element = nullptr;
if (RuntimeEnabledFeatures::FormControlsRefreshEnabled()) {
style_source_element = DynamicTo<Element>(style_source.GetNode());
}
scrollbar = MakeGarbageCollected<Scrollbar>(
ScrollableArea(), orientation, scrollbar_size, style_source_element,
&ScrollableArea()
->GetLayoutBox()
->GetFrame()
->GetPage()
->GetChromeClient());
}
ScrollableArea()->GetLayoutBox()->GetDocument().View()->AddScrollbar(
scrollbar);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,9 @@ TEST_F(ScrollableAreaTest, ScrollbarGraphicsLayerInvalidation) {
EXPECT_CALL(*scrollable_area, LayerForHorizontalScrollbar())
.WillRepeatedly(Return(&graphics_layer));

auto* scrollbar = MakeGarbageCollected<Scrollbar>(
scrollable_area, kHorizontalScrollbar, kRegularScrollbar, nullptr);
auto* scrollbar =
MakeGarbageCollected<Scrollbar>(scrollable_area, kHorizontalScrollbar,
kRegularScrollbar, nullptr, nullptr);
graphics_layer.ResetTrackedRasterInvalidations();
scrollbar->SetNeedsPaintInvalidation(kNoPart);
EXPECT_TRUE(graphics_layer.HasTrackedRasterInvalidations());
Expand Down
25 changes: 24 additions & 1 deletion third_party/blink/renderer/core/scroll/scrollbar.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,18 +31,22 @@
#include "third_party/blink/public/platform/web_gesture_event.h"
#include "third_party/blink/public/platform/web_mouse_event.h"
#include "third_party/blink/public/platform/web_scrollbar_overlay_color_theme.h"
#include "third_party/blink/renderer/core/dom/element.h"
#include "third_party/blink/renderer/core/layout/layout_object.h"
#include "third_party/blink/renderer/core/page/chrome_client.h"
#include "third_party/blink/renderer/core/scroll/scroll_animator_base.h"
#include "third_party/blink/renderer/core/scroll/scrollable_area.h"
#include "third_party/blink/renderer/core/scroll/scrollbar_theme.h"
#include "third_party/blink/renderer/platform/geometry/float_rect.h"
#include "third_party/blink/renderer/platform/graphics/paint/cull_rect.h"
#include "third_party/blink/renderer/platform/text/text_direction.h"

namespace blink {

Scrollbar::Scrollbar(ScrollableArea* scrollable_area,
ScrollbarOrientation orientation,
ScrollbarControlSize control_size,
Element* style_source,
ChromeClient* chrome_client,
ScrollbarTheme* theme)
: scrollable_area_(scrollable_area),
Expand All @@ -67,7 +71,8 @@ Scrollbar::Scrollbar(ScrollableArea* scrollable_area,
elastic_overscroll_(0),
track_needs_repaint_(true),
thumb_needs_repaint_(true),
injected_gesture_scroll_begin_(false) {
injected_gesture_scroll_begin_(false),
style_source_(style_source) {
theme_.RegisterScrollbar(*this);

// FIXME: This is ugly and would not be necessary if we fix cross-platform
Expand All @@ -88,6 +93,7 @@ Scrollbar::~Scrollbar() =default;
void Scrollbar::Trace(blink::Visitor* visitor) {
visitor->Trace(scrollable_area_);
visitor->Trace(chrome_client_);
visitor->Trace(style_source_);
}

void Scrollbar::SetFrameRect(const IntRect& frame_rect) {
Expand Down Expand Up @@ -784,6 +790,23 @@ CompositorElementId Scrollbar::GetElementId() {
return scrollable_area_->GetScrollbarElementId(orientation_);
}

float Scrollbar::EffectiveZoom() const {
if (RuntimeEnabledFeatures::FormControlsRefreshEnabled() && style_source_ &&
style_source_->GetLayoutObject()) {
return style_source_->GetLayoutObject()->Style()->EffectiveZoom();
}
return 1.0;
}

bool Scrollbar::ContainerIsRightToLeft() const {
if (RuntimeEnabledFeatures::FormControlsRefreshEnabled() && style_source_ &&
style_source_->GetLayoutObject()) {
TextDirection dir = style_source_->GetLayoutObject()->Style()->Direction();
return IsRtl(dir);
}
return false;
}

STATIC_ASSERT_ENUM(kWebScrollbarOverlayColorThemeDark,
kScrollbarOverlayColorThemeDark);
STATIC_ASSERT_ENUM(kWebScrollbarOverlayColorThemeLight,
Expand Down
13 changes: 12 additions & 1 deletion third_party/blink/renderer/core/scroll/scrollbar.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
namespace blink {

class CullRect;
class Element;
class GraphicsContext;
class IntRect;
class ChromeClient;
Expand All @@ -55,12 +56,13 @@ class CORE_EXPORT Scrollbar : public GarbageCollectedFinalized<Scrollbar>,
ScrollbarControlSize size,
ScrollbarTheme* theme) {
return MakeGarbageCollected<Scrollbar>(scrollable_area, orientation, size,
nullptr, theme);
nullptr, nullptr, theme);
}

Scrollbar(ScrollableArea*,
ScrollbarOrientation,
ScrollbarControlSize,
Element* style_source,
ChromeClient* = nullptr,
ScrollbarTheme* = nullptr);
~Scrollbar() override;
Expand Down Expand Up @@ -189,6 +191,14 @@ class CORE_EXPORT Scrollbar : public GarbageCollectedFinalized<Scrollbar>,

CompositorElementId GetElementId();

float EffectiveZoom() const;
bool ContainerIsRightToLeft() const;

// The Element that supplies our style information. If the scrollbar is
// for a document, this is either the <body> or <html> element. Otherwise, it
// is the element that owns our PaintLayerScrollableArea.
Element* StyleSource() const { return style_source_.Get(); }

virtual void Trace(blink::Visitor*);

protected:
Expand Down Expand Up @@ -242,6 +252,7 @@ class CORE_EXPORT Scrollbar : public GarbageCollectedFinalized<Scrollbar>,
bool injected_gesture_scroll_begin_;
IntRect visual_rect_;
IntRect frame_rect_;
Member<Element> style_source_;
};

} // namespace blink
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -281,8 +281,13 @@ void ScrollbarThemeAura::PaintButton(GraphicsContext& gc,
if (!params.should_paint)
return;
DrawingRecorder recorder(gc, scrollbar, display_item_type);

WebThemeEngine::ExtraParams extra_params;
extra_params.scrollbar_button.zoom = scrollbar.EffectiveZoom();
extra_params.scrollbar_button.right_to_left =
scrollbar.ContainerIsRightToLeft();
Platform::Current()->ThemeEngine()->Paint(
gc.Canvas(), params.part, params.state, WebRect(rect), nullptr,
gc.Canvas(), params.part, params.state, WebRect(rect), &extra_params,
WebColorScheme::
kLight /* TODO(futhark): pass color scheme to scrollbar parts */);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -551,3 +551,6 @@ crbug.com/981749 virtual/threaded/fast/scrolling/scroll-chaining-from-position-f


crbug.com/989673 svg/foreignObject/mask.html [ Failure ]

# These tests are failing due to subpixel differences but can't be rebaselined.
Bug(none) virtual/controls-refresh/fast/forms/controls-new-ui/select/select-multiple-appearance-basic.html [ Failure ]
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.

0 comments on commit 2e9e096

Please sign in to comment.