New issue
Advanced search Search tips

Issue 798906 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 25
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocked on:
issue 820321

Blocking:
issue 790723



Sign in to add a comment

UI Request: Create specs for Display Size Setting

Project Member Reported by ovanieva@chromium.org, Jan 4 2018

Issue description


Please create specs for Display Size setting as depicted in Mocks below. 

Launch Bug: crbug/790723

PRD: go/CrosResolution

Mocks: go/DisplaySizeMocks
 

Comment 1 by osh...@chromium.org, Jan 26 2018

Components: UI>Shell>MultipleMonitor
Cc: -ovanieva@chromium.org sgabr...@chromium.org
Owner: ovanieva@chromium.org
High fidelity mocks have been aded here:
https://gallery.googleplex.com/projects/MCHbtQVoQ2HCZZ9oo1_la-_D/files/MCG17nfZaSnyAfyJc1bJy4PLwMSJbxAVNpw

Since this is only using existing components, I didn't go through the exercise or writing a spec. Please note that for the scale tooltip, we should use the same type of tooltips used further down in the slider for Night Light settings. 

Let me know if you need anything else or more details.
Malay, please use latest mirroring mocks with check box instead of toggle.
#2 The night light does not have a tool tip right now.

There is something similar for the schedule slider though. This however is not a tool tip, instead a permanent indicator that hovers above the point.

Night Light Slider.png
29.5 KB View Download
Right I think the idea is to take that, hovering over the point but only when manipulated. Called it tooltip for lack of better word but it's essentially on on-press floating label.
Cc: steve...@chromium.org
@Steven
For the above requirement, do you think its a good idea to add this to the <settings-slider> component? This way it can be used system wide consistently.
settings-slider should probably have been named pref-slider. Unless this is something that should apply to a slider represing a single pref value, I would not add it there.

If we use the behavior in more than one slider, it may make sense to create a different shared component however.

It is representing a single value. 
36pZddKujZE.png
2.8 KB View Download
Is it a single pref store value? And is the behavior something we want in other settings-slider instances?

 - Is it a single pref store value?
Yes

 - Is the behavior something we want in other settings-slider instances?
Looking at the codebase, none of the other usage for setting slider displays the slider value. So for now this will be used for this particular instance.


Before.png
5.1 KB View Download
After.png
6.8 KB View Download
I could see this behavior being useful in other instances like font size settings for example. Pretty much every time a slider doesn't display all stop-gap values.
OK, taking another look at settings-slider, there's a fair bit of customization there. Adding this feature seems reasonable.

something else that would be helpful for this: show the effective resolution: I always have to go to https://www.whatismyscreenresolution.com on any device to see the effective resolution, but macOS display settings already shows the effective resolution. I think users would appreciate this and it would help them to better understand what is going and and how it works
Cc: michae...@chromium.org
#12
Looking at settings-slider again, I am not sure how to proceed with implementing it.

I would need 2 pieces of information to make it work.
1) Detecting mouse enter and leave. paper-tooltip has some reference on how this can be done.
2) Getting the current x position of the slider knob. The slider knob is in the shodow DOM of paper-slider, a third party library. Accessing its elements would be very hacky and prone to failures if there are any upstream changes in how paper-slider is implemented.

+michaelpg as well.

Cc: hcarmona@chromium.org dpa...@chromium.org
I don't have any easy answers. +hcarmona@, +dpapad@ who might have some thoughts here, but any such changes should be tested to prevent breakage due to Polymer element updates. The paper elements are mostly comprised of behaviors so it may be easier to create a new element using the same behaviors.

This would be a good example where we should get UX to spec out how these sliders should behave, with a rule for when the tooltip-like behavior should appear (i.e. automatically based on the range, or manually by adding a custom attribute). Then we can build the element and sue it throughout Settings. Then the current settings-slider element could be built with the new element (since not all sliders represent pref values).

Cc: namratakannan@chromium.org
+namraatakannan

@malaykeshav: Peeking into the shadow DOM of paper-slider is a no-go. Parent elements should interact with child elements via properly defined APIs (like public methods, or HTML attributes, or CSS mixins).

Also note that paper-slider already has a way to display the current value (see demo at [1]). I would be hesitant of implementing a different way, while still leveraging paper-slider under the cover.

My suggestion is to have UX figure out what do we want sliders to look like, especially within the context of the upcoming MD2 updates, and implement those as part of that effort.

[1] https://www.webcomponents.org/element/PolymerElements/paper-slider/demo/demo/index.html
Components: UI>Browser>WebUI
Labels: Proj-MaterialDesign-WebUI
Blockedon: 820321
@sgabriel
Attaching a video of the implementation for Display Slider.
This includes all operations required from a slider.

Do we want to add a special indicator for the default value?

DisplaySlider.ogv
1.3 MB View Download
This is cool. Thanks Malay.
For the default state it would make sent to make it "Default:xx%" if it can be anything else than 100%.
The default value is always 100%. However this value will not always have the same position on the slider. 
A correct way to put this would be, should we change the marker on the slider for the default value? A bigger marker? Or a different color?
Recordings of the final implementation sent for review.
Updated Display Slider UI Review.webm
3.0 MB View Download
Display Slider RTL.webm
3.1 MB View Download
Project Member

Comment 23 by bugdroid1@chromium.org, Mar 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8926b625fd56c9b3f418bc41843f6e4b5711baba

commit 8926b625fd56c9b3f418bc41843f6e4b5711baba
Author: Malay Keshav <malaykeshav@chromium.org>
Date: Fri Mar 30 03:30:23 2018

Implements a custom slider for display size setting

This patch implements a custom slider that provides the basic
functionality of the paper-slider but provides an additional option to
display a label for the current slider value. The slider has the Prefs
Behavior integrated with it.

This slider has a general implemention and is ready to be used in other
locations as well.

Functionalities implemented:
 - Slider works the same way as the paper-slider in terms of mouse and
   keyboard interactions.
 - Additionally The slider displays a label on mouse hover and keyboard
   focus.
 - The slider only works for discrete values or options.

This patch also adds web ui tests for all the slider functionalities.

A video of this slider in use can be found on the bug link below.

Bug:  798906 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I55fa98eb438ed1c48b4f40c14d8c05085e7a45b4
Component: Display Slider, display settings, webui tests
Reviewed-on: https://chromium-review.googlesource.com/985127
Commit-Queue: Malay Keshav <malaykeshav@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547101}
[modify] https://crrev.com/8926b625fd56c9b3f418bc41843f6e4b5711baba/chrome/app/settings_strings.grdp
[modify] https://crrev.com/8926b625fd56c9b3f418bc41843f6e4b5711baba/chrome/browser/resources/settings/device_page/compiled_resources2.gyp
[modify] https://crrev.com/8926b625fd56c9b3f418bc41843f6e4b5711baba/chrome/browser/resources/settings/device_page/display.html
[modify] https://crrev.com/8926b625fd56c9b3f418bc41843f6e4b5711baba/chrome/browser/resources/settings/device_page/display.js
[add] https://crrev.com/8926b625fd56c9b3f418bc41843f6e4b5711baba/chrome/browser/resources/settings/device_page/display_size_slider.html
[add] https://crrev.com/8926b625fd56c9b3f418bc41843f6e4b5711baba/chrome/browser/resources/settings/device_page/display_size_slider.js
[modify] https://crrev.com/8926b625fd56c9b3f418bc41843f6e4b5711baba/chrome/browser/resources/settings/settings_resources.grd
[modify] https://crrev.com/8926b625fd56c9b3f418bc41843f6e4b5711baba/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
[modify] https://crrev.com/8926b625fd56c9b3f418bc41843f6e4b5711baba/chrome/test/data/webui/settings/cr_settings_browsertest.js
[add] https://crrev.com/8926b625fd56c9b3f418bc41843f6e4b5711baba/chrome/test/data/webui/settings/display_size_slider_test.js

Project Member

Comment 24 by bugdroid1@chromium.org, Mar 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9bb4e7ab81af5be33cc71c1127bcd757140d8a1b

commit 9bb4e7ab81af5be33cc71c1127bcd757140d8a1b
Author: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Date: Fri Mar 30 15:49:10 2018

Revert "Implements a custom slider for display size setting"

This reverts commit 8926b625fd56c9b3f418bc41843f6e4b5711baba.

Reason for revert: Possible culprit for the failing of
CrSettingsDevicePageTest.DisplayTest
CrSettingsDevicePageTest.KeyboardTest
CrSettingsDevicePageTest.PowerTest
CrSettingsDisplaySizeSliderTest.All
CrSettingsDevicePageTest.DevicePageTest
CrSettingsDevicePageTest.PointersTest
CrSettingsDevicePageTest.StylusTest

on the bots.

First build with errors:https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/linux-chromeos-dbg/builds/4954

9 builds failed in a row with those errors.

Original change's description:
> Implements a custom slider for display size setting
> 
> This patch implements a custom slider that provides the basic
> functionality of the paper-slider but provides an additional option to
> display a label for the current slider value. The slider has the Prefs
> Behavior integrated with it.
> 
> This slider has a general implemention and is ready to be used in other
> locations as well.
> 
> Functionalities implemented:
>  - Slider works the same way as the paper-slider in terms of mouse and
>    keyboard interactions.
>  - Additionally The slider displays a label on mouse hover and keyboard
>    focus.
>  - The slider only works for discrete values or options.
> 
> This patch also adds web ui tests for all the slider functionalities.
> 
> A video of this slider in use can be found on the bug link below.
> 
> Bug:  798906 
> Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
> Change-Id: I55fa98eb438ed1c48b4f40c14d8c05085e7a45b4
> Component: Display Slider, display settings, webui tests
> Reviewed-on: https://chromium-review.googlesource.com/985127
> Commit-Queue: Malay Keshav <malaykeshav@chromium.org>
> Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#547101}

TBR=stevenjb@chromium.org,malaykeshav@chromium.org

Change-Id: I456977e7b8358e6210fc72baf66bc692514f6f27
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  798906 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Reviewed-on: https://chromium-review.googlesource.com/988092
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Commit-Queue: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547172}
[modify] https://crrev.com/9bb4e7ab81af5be33cc71c1127bcd757140d8a1b/chrome/app/settings_strings.grdp
[modify] https://crrev.com/9bb4e7ab81af5be33cc71c1127bcd757140d8a1b/chrome/browser/resources/settings/device_page/compiled_resources2.gyp
[modify] https://crrev.com/9bb4e7ab81af5be33cc71c1127bcd757140d8a1b/chrome/browser/resources/settings/device_page/display.html
[modify] https://crrev.com/9bb4e7ab81af5be33cc71c1127bcd757140d8a1b/chrome/browser/resources/settings/device_page/display.js
[delete] https://crrev.com/a82bdb76536b5fce185734225128ca3ccb5a2e8c/chrome/browser/resources/settings/device_page/display_size_slider.html
[delete] https://crrev.com/a82bdb76536b5fce185734225128ca3ccb5a2e8c/chrome/browser/resources/settings/device_page/display_size_slider.js
[modify] https://crrev.com/9bb4e7ab81af5be33cc71c1127bcd757140d8a1b/chrome/browser/resources/settings/settings_resources.grd
[modify] https://crrev.com/9bb4e7ab81af5be33cc71c1127bcd757140d8a1b/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
[modify] https://crrev.com/9bb4e7ab81af5be33cc71c1127bcd757140d8a1b/chrome/test/data/webui/settings/cr_settings_browsertest.js
[delete] https://crrev.com/a82bdb76536b5fce185734225128ca3ccb5a2e8c/chrome/test/data/webui/settings/display_size_slider_test.js

Project Member

Comment 25 by bugdroid1@chromium.org, Mar 31 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f36c74c4c75c6c7121a013f220f463baffbc7aba

commit f36c74c4c75c6c7121a013f220f463baffbc7aba
Author: Malay Keshav <malaykeshav@chromium.org>
Date: Sat Mar 31 01:53:03 2018

Reland: Implements a custom slider for display size setting

This patch implements a custom slider that provides the basic
functionality of the paper-slider but provides an additional option to
display a label for the current slider value. The slider has the Prefs
Behavior integrated with it.

This slider has a general implemention and is ready to be used in other
locations as well.

Functionalities implemented:
 - Slider works the same way as the paper-slider in terms of mouse and
   keyboard interactions.
 - Additionally The slider displays a label on mouse hover and keyboard
   focus.
 - The slider only works for discrete values or options.

This patch also adds web ui tests for all the slider functionalities.

A video of this slider in use can be found on the bug link below.

Bug:  798906 
Change-Id: Ib51ec47d0d2e8ad83ea66220ddf746fe15208d5f
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Component: Display Slider, display settings, webui tests
Reverted Patch: https://chromium-review.googlesource.com/c/chromium/src/+/988092

TBR=stevenjb@chromium.org

Change-Id: Ib51ec47d0d2e8ad83ea66220ddf746fe15208d5f
Reviewed-on: https://chromium-review.googlesource.com/988823
Reviewed-by: Malay Keshav <malaykeshav@chromium.org>
Commit-Queue: Malay Keshav <malaykeshav@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547359}
[modify] https://crrev.com/f36c74c4c75c6c7121a013f220f463baffbc7aba/chrome/app/settings_strings.grdp
[modify] https://crrev.com/f36c74c4c75c6c7121a013f220f463baffbc7aba/chrome/browser/resources/settings/device_page/compiled_resources2.gyp
[modify] https://crrev.com/f36c74c4c75c6c7121a013f220f463baffbc7aba/chrome/browser/resources/settings/device_page/display.html
[modify] https://crrev.com/f36c74c4c75c6c7121a013f220f463baffbc7aba/chrome/browser/resources/settings/device_page/display.js
[add] https://crrev.com/f36c74c4c75c6c7121a013f220f463baffbc7aba/chrome/browser/resources/settings/device_page/display_size_slider.html
[add] https://crrev.com/f36c74c4c75c6c7121a013f220f463baffbc7aba/chrome/browser/resources/settings/device_page/display_size_slider.js
[modify] https://crrev.com/f36c74c4c75c6c7121a013f220f463baffbc7aba/chrome/browser/resources/settings/settings_resources.grd
[modify] https://crrev.com/f36c74c4c75c6c7121a013f220f463baffbc7aba/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
[modify] https://crrev.com/f36c74c4c75c6c7121a013f220f463baffbc7aba/chrome/test/data/webui/settings/cr_settings_browsertest.js
[add] https://crrev.com/f36c74c4c75c6c7121a013f220f463baffbc7aba/chrome/test/data/webui/settings/display_size_slider_test.js

Is this bug fixed now?
Status: Fixed (was: Assigned)
Yes. This should be marked as fixed.

Sign in to add a comment