Issue metadata
Sign in to add a comment
|
Regression: Weird behavior of sliders is seen in chrome://settings |
||||||||||||||||||||||
Issue descriptionChrome Version: 73.0.3635.0/11377.0.0 dev channel Kip, Reks, Daisy OS: Chrome OS What steps will reproduce the problem? (1)Sign into user >>Navigate to chrome://settings/manageAccessibility/tts (2)Try to click or drag any slider and observe weird behavior Actual: Weird behavior of sliders is seen in chrome://settings Expected: No such issue should be seen This is a Regression issue as same is working fine in 72.0.3626.8/11316.6.0 dev Attaching screencasts for reference...
,
Dec 10
aee@ can you take a look?
,
Dec 10
,
Dec 11
,
Dec 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8952c673cb0070f8a164146788bc75f33a3a8cc4 commit 8952c673cb0070f8a164146788bc75f33a3a8cc4 Author: Esmael El-Moslimany <aee@chromium.org> Date: Thu Dec 20 01:44:12 2018 WebUI: cr-slider, remove value-changed event in favor of cr-slider-value-changed When |value| is updated either manually or through the UI, value-changed events were being fired. This could cause two issues: momentary invalid values and feedback loops. When |value| is manually updated continuously to a valid value, a value-changed event is not necessary. The slider should accept whatever the latest updated |value| is and not update the source of the manual update with a potentially stale value. When the slider is being interacted with via the mouse, a change event is only needed when the value is in a valid state. Intermediate values between discrete values or outside the min/max bounds should be corrected and once corrected fire an event. This is the root cause of the linked bug. Bug: 913404 Change-Id: I842d1a27ae5a434ffe7d63efa7c7db3f00198cf8 Reviewed-on: https://chromium-review.googlesource.com/c/1372268 Commit-Queue: Esmael El-Moslimany <aee@chromium.org> Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org> Reviewed-by: Sam McNally <sammc@chromium.org> Cr-Commit-Position: refs/heads/master@{#618068} [modify] https://crrev.com/8952c673cb0070f8a164146788bc75f33a3a8cc4/chrome/browser/resources/settings/controls/settings_slider.html [modify] https://crrev.com/8952c673cb0070f8a164146788bc75f33a3a8cc4/chrome/browser/resources/settings/controls/settings_slider.js [modify] https://crrev.com/8952c673cb0070f8a164146788bc75f33a3a8cc4/chrome/browser/resources/settings/device_page/display.html [modify] https://crrev.com/8952c673cb0070f8a164146788bc75f33a3a8cc4/chrome/test/data/webui/cr_elements/cr_slider_test.js [modify] https://crrev.com/8952c673cb0070f8a164146788bc75f33a3a8cc4/chrome/test/data/webui/settings/device_page_tests.js [modify] https://crrev.com/8952c673cb0070f8a164146788bc75f33a3a8cc4/chrome/test/data/webui/settings/settings_slider_tests.js [modify] https://crrev.com/8952c673cb0070f8a164146788bc75f33a3a8cc4/ui/file_manager/audio_player/elements/control_panel.js [modify] https://crrev.com/8952c673cb0070f8a164146788bc75f33a3a8cc4/ui/file_manager/gallery/js/image_editor/image_editor_toolbar.js [modify] https://crrev.com/8952c673cb0070f8a164146788bc75f33a3a8cc4/ui/file_manager/video_player/js/media_controls.js [modify] https://crrev.com/8952c673cb0070f8a164146788bc75f33a3a8cc4/ui/file_manager/video_player/js/video_player.js [modify] https://crrev.com/8952c673cb0070f8a164146788bc75f33a3a8cc4/ui/webui/resources/cr_elements/cr_slider/cr_slider.js
,
Dec 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3ce569aa1b336971ef11d0ee04e6073815530417 commit 3ce569aa1b336971ef11d0ee04e6073815530417 Author: Lutz Justen <ljusten@chromium.org> Date: Thu Dec 20 13:02:04 2018 [Sheriff] Revert "WebUI: cr-slider, remove value-changed event in favor of cr-slider-value-changed" This reverts commit 8952c673cb0070f8a164146788bc75f33a3a8cc4. Reason for revert: Causes flakes in CrElementsSliderTest.All. Original change's description: > WebUI: cr-slider, remove value-changed event in favor of cr-slider-value-changed > > When |value| is updated either manually or through the UI, value-changed events > were being fired. This could cause two issues: momentary invalid values and > feedback loops. > > When |value| is manually updated continuously to a valid value, a value-changed > event is not necessary. The slider should accept whatever the latest updated > |value| is and not update the source of the manual update with a potentially > stale value. > > When the slider is being interacted with via the mouse, a change event is only > needed when the value is in a valid state. Intermediate values between > discrete values or outside the min/max bounds should be corrected and once > corrected fire an event. This is the root cause of the linked bug. > > Bug: 913404 > Change-Id: I842d1a27ae5a434ffe7d63efa7c7db3f00198cf8 > Reviewed-on: https://chromium-review.googlesource.com/c/1372268 > Commit-Queue: Esmael El-Moslimany <aee@chromium.org> > Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org> > Reviewed-by: Sam McNally <sammc@chromium.org> > Cr-Commit-Position: refs/heads/master@{#618068} TBR=dpapad@chromium.org,sammc@chromium.org,aee@chromium.org Change-Id: Ie0839f0600393bbd38f86b96695da43e1abc1cdd No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 913404 , 916888 Reviewed-on: https://chromium-review.googlesource.com/c/1386433 Reviewed-by: Lutz Justen <ljusten@chromium.org> Commit-Queue: Lutz Justen <ljusten@chromium.org> Cr-Commit-Position: refs/heads/master@{#618186} [modify] https://crrev.com/3ce569aa1b336971ef11d0ee04e6073815530417/chrome/browser/resources/settings/controls/settings_slider.html [modify] https://crrev.com/3ce569aa1b336971ef11d0ee04e6073815530417/chrome/browser/resources/settings/controls/settings_slider.js [modify] https://crrev.com/3ce569aa1b336971ef11d0ee04e6073815530417/chrome/browser/resources/settings/device_page/display.html [modify] https://crrev.com/3ce569aa1b336971ef11d0ee04e6073815530417/chrome/test/data/webui/cr_elements/cr_slider_test.js [modify] https://crrev.com/3ce569aa1b336971ef11d0ee04e6073815530417/chrome/test/data/webui/settings/device_page_tests.js [modify] https://crrev.com/3ce569aa1b336971ef11d0ee04e6073815530417/chrome/test/data/webui/settings/settings_slider_tests.js [modify] https://crrev.com/3ce569aa1b336971ef11d0ee04e6073815530417/ui/file_manager/audio_player/elements/control_panel.js [modify] https://crrev.com/3ce569aa1b336971ef11d0ee04e6073815530417/ui/file_manager/gallery/js/image_editor/image_editor_toolbar.js [modify] https://crrev.com/3ce569aa1b336971ef11d0ee04e6073815530417/ui/file_manager/video_player/js/media_controls.js [modify] https://crrev.com/3ce569aa1b336971ef11d0ee04e6073815530417/ui/file_manager/video_player/js/video_player.js [modify] https://crrev.com/3ce569aa1b336971ef11d0ee04e6073815530417/ui/webui/resources/cr_elements/cr_slider/cr_slider.js
,
Dec 21
,
Jan 2
,
Jan 2
This bug is marked fixed, but it looks like the change was reverted immediately. Is it actually fixed?
,
Jan 2
It was rereverted with the fix here https://crbug.com/916888#c5 . Maybe 916888 should be merged into this issue for easier tracking. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by kebalaji@chromium.org
, Dec 10