New issue
Advanced search Search tips
Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 27
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment
link

Issue 902873: WebUI: Replace remaining paper-slider usages with cr-slider

Reported by dpa...@chromium.org, Nov 7 Project Member

Issue description

There are still a few paper-slider usages that prevent us from removing this element from third_party/polymer. Specifically in the following files:

ui/file_manager/video_player/video_player.html
ui/file_manager/gallery/gallery.html
ui/file_manager/audio_player/elements/control_panel.html
chrome/browser/resources/media_router/elements/route_controls/route_controls.html


The media_router UI is planned to be deleted soon, so no need to update it. But file_manager UI needs to be updated.

@aee: Can you list any known tasks necessary in order to replace those usages?
 

Comment 1 by aee@chromium.org, Nov 14

Status: Started (was: Available)

Comment 2 by dpa...@chromium.org, Nov 14

Owner: aee@chromium.org

Comment 3 by bugdroid1@chromium.org, Nov 22

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1c8d73b56bafcbf0df5cbc2b5361b2fe3549bf16

commit 1c8d73b56bafcbf0df5cbc2b5361b2fe3549bf16
Author: Esmael El-Moslimany <aee@chromium.org>
Date: Thu Nov 22 07:44:54 2018

Audio Player: replace paper-slider with cr-slider

The audio player's progress slider uses sliders in a different way from
other sliders in settings. The value of the slider is updated
frequently when playing. While the knob is being dragged, audio
playback is paused and is resumed when the drag completes.

Previously the audio player was using the immediate-value-changed and
value-changed events to determine if the knob was being dragged.

A couple changes were required to support the audio player progress
slider. When the |dragging| property is changed, the |dragging-changed|
event is dispatched. And the slider |value| is updated after dragging
has started and before dragging has completed. This allows the audio
player to pause playback, ignore any subsequent updates to the progress
and accept changes to progress during dragging. When dragging is done,
the audio player has access to the most up-to-date value of the slider
so it can update the time of playback then resume playing.

Inverting the direction of ArrowRight/ArrowLeft keybindings when the
direction is RTL.

Bug:  902873 
Change-Id: I898889fc882ef0b1459ceabf8d27ed5ad62267c0
Reviewed-on: https://chromium-review.googlesource.com/c/1325012
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Esmael El-Moslimany <aee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610339}
[modify] https://crrev.com/1c8d73b56bafcbf0df5cbc2b5361b2fe3549bf16/chrome/test/data/webui/cr_elements/cr_slider_test.js
[modify] https://crrev.com/1c8d73b56bafcbf0df5cbc2b5361b2fe3549bf16/ui/file_manager/audio_player/elements/BUILD.gn
[modify] https://crrev.com/1c8d73b56bafcbf0df5cbc2b5361b2fe3549bf16/ui/file_manager/audio_player/elements/audio_player.js
[modify] https://crrev.com/1c8d73b56bafcbf0df5cbc2b5361b2fe3549bf16/ui/file_manager/audio_player/elements/control_panel.css
[modify] https://crrev.com/1c8d73b56bafcbf0df5cbc2b5361b2fe3549bf16/ui/file_manager/audio_player/elements/control_panel.html
[modify] https://crrev.com/1c8d73b56bafcbf0df5cbc2b5361b2fe3549bf16/ui/file_manager/audio_player/elements/control_panel.js
[modify] https://crrev.com/1c8d73b56bafcbf0df5cbc2b5361b2fe3549bf16/ui/file_manager/audio_player/js/audio_player.js
[modify] https://crrev.com/1c8d73b56bafcbf0df5cbc2b5361b2fe3549bf16/ui/webui/resources/cr_elements/cr_slider/cr_slider.html
[modify] https://crrev.com/1c8d73b56bafcbf0df5cbc2b5361b2fe3549bf16/ui/webui/resources/cr_elements/cr_slider/cr_slider.js

Comment 4 by aee@chromium.org, Nov 29

Status: Assigned (was: Started)
Audio player is complete. Next up is video player and gallery.

Comment 5 by aee@chromium.org, Nov 30

Status: Started (was: Assigned)

Comment 6 by bugdroid1@chromium.org, Dec 4

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4635514456b10c814bce0029bb1fed1a87202af3

commit 4635514456b10c814bce0029bb1fed1a87202af3
Author: Esmael El-Moslimany <aee@chromium.org>
Date: Tue Dec 04 01:51:31 2018

Video Player WebUI: replace paper-slider with cr-slider

The video player progress needs to be updated from the slider value
only when the change is due to a user interaction. For this
reason cr-slider-value-changed-from-ui was introduced.

cr-slider getRatio() was made public since it is meaningful
representation of the slider value.

Swapped the skip direction of ArrowLeft/ArrowRight when the text
direction is RTL.

When the volume value is changed with the keyboard and the media
controls are hidden, the media controls are shown.

Bug:  902873 
Change-Id: Ide0a23783274d4c11ea83b367aba8b41a5dd8ad1
Reviewed-on: https://chromium-review.googlesource.com/c/1357216
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Esmael El-Moslimany <aee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613384}
[modify] https://crrev.com/4635514456b10c814bce0029bb1fed1a87202af3/chrome/test/data/webui/cr_elements/cr_slider_test.js
[modify] https://crrev.com/4635514456b10c814bce0029bb1fed1a87202af3/ui/file_manager/video_player/css/media_controls.css
[modify] https://crrev.com/4635514456b10c814bce0029bb1fed1a87202af3/ui/file_manager/video_player/js/BUILD.gn
[modify] https://crrev.com/4635514456b10c814bce0029bb1fed1a87202af3/ui/file_manager/video_player/js/media_controls.js
[modify] https://crrev.com/4635514456b10c814bce0029bb1fed1a87202af3/ui/file_manager/video_player/js/video_player.js
[modify] https://crrev.com/4635514456b10c814bce0029bb1fed1a87202af3/ui/file_manager/video_player/video_player.html
[modify] https://crrev.com/4635514456b10c814bce0029bb1fed1a87202af3/ui/webui/resources/cr_elements/cr_slider/cr_slider.js

Comment 7 by aee@chromium.org, Dec 4

Screenshots for https://chromium-review.googlesource.com/c/chromium/src/+/1362245.
Screenshot from 2018-12-04 14-42-08.png
38.7 KB View Download
Screenshot from 2018-12-04 14-43-49.png
63.4 KB View Download

Comment 8 by bugdroid1@chromium.org, Dec 7

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/52964c260bee1e96f746517215ac398ccd2c67ba

commit 52964c260bee1e96f746517215ac398ccd2c67ba
Author: Esmael El-Moslimany <aee@chromium.org>
Date: Fri Dec 07 01:11:05 2018

WebUI: cr-slider, simplify slider logic by removing |updateValueInstantly| property

|updateValueInstantly| is only used by the display size slider. The property
was passed through settings-slider. This CL moves the |updateValueInstantly|
logic to settings-slider.

The reasoning is that cr-slider had a property |value| that represents the
slider value unless the slider was in a dragging state and
|updateValueInstantly| was false. A new value called |immediateValue_|
was added to always have the correct value even during drag. But this
was only to ultimately stop a value-changed event from being handled
which would update |pref.value|. Instead of having two properties in
cr-slider and one property in settings-slider to store the slider value,
moving the |updateValueInstantly| logic to settings-slider significantly
simplifies the code.

Bug:  902873 
Change-Id: I5d5d8771aa8246850bfd9b4926d820180e43c211
Reviewed-on: https://chromium-review.googlesource.com/c/1363642
Commit-Queue: Esmael El-Moslimany <aee@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614530}
[modify] https://crrev.com/52964c260bee1e96f746517215ac398ccd2c67ba/chrome/browser/resources/settings/controls/settings_slider.html
[modify] https://crrev.com/52964c260bee1e96f746517215ac398ccd2c67ba/chrome/browser/resources/settings/controls/settings_slider.js
[modify] https://crrev.com/52964c260bee1e96f746517215ac398ccd2c67ba/chrome/test/data/webui/cr_elements/cr_slider_test.js
[modify] https://crrev.com/52964c260bee1e96f746517215ac398ccd2c67ba/chrome/test/data/webui/settings/settings_slider_tests.js
[modify] https://crrev.com/52964c260bee1e96f746517215ac398ccd2c67ba/ui/webui/resources/cr_elements/cr_slider/cr_slider.html
[modify] https://crrev.com/52964c260bee1e96f746517215ac398ccd2c67ba/ui/webui/resources/cr_elements/cr_slider/cr_slider.js

Comment 9 by bugdroid1@chromium.org, Dec 11

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

commit a2e98d8b5fd647a3f1c59fff562dd450ddae9bd1
Author: Esmael El-Moslimany <aee@chromium.org>
Date: Tue Dec 11 19:30:42 2018

Gallery WebUI: replace paper-slider with cr-slider in image editor

Making cr-slider tabindex=0 when enabled so it can be focused using
|slider.focus()|. When a control is selected in the image editor, the
first control in the group is given focus (for example when clicking on
the brightness/contrast button, the brightness slider should have
focus.

Added css variables to allow for a different color scheme for image
editor sliders.

Bug:  902873 
Change-Id: I4cf9c5082f81c9a8b50529c4e7f59537c4e4cfa6
Reviewed-on: https://chromium-review.googlesource.com/c/1357652
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Esmael El-Moslimany <aee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615623}
[modify] https://crrev.com/a2e98d8b5fd647a3f1c59fff562dd450ddae9bd1/chrome/test/data/webui/cr_elements/cr_slider_test.js
[modify] https://crrev.com/a2e98d8b5fd647a3f1c59fff562dd450ddae9bd1/ui/file_manager/gallery/css/gallery.css
[modify] https://crrev.com/a2e98d8b5fd647a3f1c59fff562dd450ddae9bd1/ui/file_manager/gallery/gallery.html
[modify] https://crrev.com/a2e98d8b5fd647a3f1c59fff562dd450ddae9bd1/ui/file_manager/gallery/js/image_editor/image_editor_toolbar.js
[modify] https://crrev.com/a2e98d8b5fd647a3f1c59fff562dd450ddae9bd1/ui/file_manager/integration_tests/gallery/photo_editor.js
[modify] https://crrev.com/a2e98d8b5fd647a3f1c59fff562dd450ddae9bd1/ui/webui/resources/cr_elements/cr_slider/cr_slider.html
[modify] https://crrev.com/a2e98d8b5fd647a3f1c59fff562dd450ddae9bd1/ui/webui/resources/cr_elements/cr_slider/cr_slider.js

Comment 10 by bugdroid1@chromium.org, Dec 21

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

commit f852812bdc3de2c24efde25b0f7394817561d951
Author: Esmael El-Moslimany <aee@chromium.org>
Date: Fri Dec 21 02:53:02 2018

Media Router WebUI: replace paper-slider with cr-slider

Bug:  902873 
Change-Id: I7fe0979471ba0206ff1326448e09bf4a5f7f8437
Reviewed-on: https://chromium-review.googlesource.com/c/1362245
Commit-Queue: Esmael El-Moslimany <aee@chromium.org>
Reviewed-by: Takumi Fujimoto <takumif@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618440}
[modify] https://crrev.com/f852812bdc3de2c24efde25b0f7394817561d951/chrome/browser/resources/media_router/elements/route_controls/route_controls.css
[modify] https://crrev.com/f852812bdc3de2c24efde25b0f7394817561d951/chrome/browser/resources/media_router/elements/route_controls/route_controls.html
[modify] https://crrev.com/f852812bdc3de2c24efde25b0f7394817561d951/chrome/browser/resources/media_router/elements/route_controls/route_controls.js
[modify] https://crrev.com/f852812bdc3de2c24efde25b0f7394817561d951/chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc
[modify] https://crrev.com/f852812bdc3de2c24efde25b0f7394817561d951/chrome/browser/ui/webui/media_router/media_router_webui_message_handler_unittest.cc
[modify] https://crrev.com/f852812bdc3de2c24efde25b0f7394817561d951/chrome/test/data/webui/cr_elements/cr_slider_test.js
[modify] https://crrev.com/f852812bdc3de2c24efde25b0f7394817561d951/chrome/test/data/webui/media_router/route_controls_tests.js
[modify] https://crrev.com/f852812bdc3de2c24efde25b0f7394817561d951/ui/webui/resources/cr_elements/cr_radio_group/cr_radio_group.js
[modify] https://crrev.com/f852812bdc3de2c24efde25b0f7394817561d951/ui/webui/resources/cr_elements/cr_slider/cr_slider.html
[modify] https://crrev.com/f852812bdc3de2c24efde25b0f7394817561d951/ui/webui/resources/cr_elements/cr_slider/cr_slider.js

Comment 11 by bugdroid1@chromium.org, Dec 27

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/82d643e5cda14541b31380222c58c7f61efd1705

commit 82d643e5cda14541b31380222c58c7f61efd1705
Author: Esmael El-Moslimany <aee@chromium.org>
Date: Thu Dec 27 00:42:58 2018

WebUI: removing paper-slider

Bug:  902873 
Change-Id: I2bfd69c997b5d0586eb07d0bfa120d4d0ddd80e8
Reviewed-on: https://chromium-review.googlesource.com/c/1389500
Reviewed-by: Hector Carmona <hcarmona@chromium.org>
Reviewed-by: Dan Beam <dbeam@chromium.org>
Commit-Queue: Esmael El-Moslimany <aee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618985}
[modify] https://crrev.com/82d643e5cda14541b31380222c58c7f61efd1705/chrome/browser/resources/settings/controls/settings_slider.js
[modify] https://crrev.com/82d643e5cda14541b31380222c58c7f61efd1705/chrome/browser/resources/settings/search_settings.js
[modify] https://crrev.com/82d643e5cda14541b31380222c58c7f61efd1705/chrome/test/data/webui/cr_focus_row_behavior_interactive_test.js
[modify] https://crrev.com/82d643e5cda14541b31380222c58c7f61efd1705/chrome/test/data/webui/settings/cr_settings_browsertest.js
[modify] https://crrev.com/82d643e5cda14541b31380222c58c7f61efd1705/chrome/test/data/webui/settings/cr_settings_interactive_ui_tests.js
[modify] https://crrev.com/82d643e5cda14541b31380222c58c7f61efd1705/chrome/test/data/webui/settings/device_page_tests.js
[modify] https://crrev.com/82d643e5cda14541b31380222c58c7f61efd1705/chrome/test/data/webui/settings/search_settings_test.js
[modify] https://crrev.com/82d643e5cda14541b31380222c58c7f61efd1705/chrome/test/data/webui/settings/settings_slider_tests.js
[modify] https://crrev.com/82d643e5cda14541b31380222c58c7f61efd1705/third_party/polymer/README.chromium
[modify] https://crrev.com/82d643e5cda14541b31380222c58c7f61efd1705/third_party/polymer/v1_0/bower.json
[modify] https://crrev.com/82d643e5cda14541b31380222c58c7f61efd1705/third_party/polymer/v1_0/chromium.patch
[delete] https://crrev.com/39c98a75eb0ba353cc3908ae0024f28cae85815f/third_party/polymer/v1_0/components-chromium/paper-slider/BUILD.gn
[delete] https://crrev.com/39c98a75eb0ba353cc3908ae0024f28cae85815f/third_party/polymer/v1_0/components-chromium/paper-slider/bower.json
[delete] https://crrev.com/39c98a75eb0ba353cc3908ae0024f28cae85815f/third_party/polymer/v1_0/components-chromium/paper-slider/paper-slider-extracted.js
[delete] https://crrev.com/39c98a75eb0ba353cc3908ae0024f28cae85815f/third_party/polymer/v1_0/components-chromium/paper-slider/paper-slider.html
[modify] https://crrev.com/82d643e5cda14541b31380222c58c7f61efd1705/third_party/polymer/v1_0/components_summary.txt
[modify] https://crrev.com/82d643e5cda14541b31380222c58c7f61efd1705/ui/webui/resources/polymer_resources.grdp

Comment 12 by aee@chromium.org, Dec 27

Status: Fixed (was: Started)

Sign in to add a comment