Issue metadata
Sign in to add a comment
|
Holding brightness-down key causes brightness to be increased from 0 |
||||||||||||||||||||
Issue descriptionThere have been a bunch of reports that holding the brightness-down key makes the backlight brightness drop to zero, but eventually makes the backlight turn on again at its minimum-visible level. The powerd logs look like this: [0310/184200:INFO:internal_backlight_controller.cc(442)] Got user-triggered request to set brightness to 0% [0310/184200:INFO:internal_backlight_controller.cc(676)] Setting brightness to 0 (0%) over 200 ms [0310/184200:INFO:daemon.cc(1384)] Saw user activity [0310/184200:INFO:internal_backlight_controller.cc(676)] Setting brightness to 1 (6.25%) over 200 ms [0310/184200:INFO:display_power_setter.cc(72)] Asking Chrome to turn all displays on [0310/184200:INFO:internal_backlight_controller.cc(442)] Got user-triggered request to set brightness to 0% [0310/184200:INFO:internal_backlight_controller.cc(676)] Setting brightness to 0 (0%) over 200 ms powerd contains code to intentionally increase the backlight brightness to the minimum-visible level in response to user activity after it's been set to 0. This is present to keep the user from getting into a state where try to interact with their Chromebook after forgetting that they've turned the display off. Chrome and powerd also try to work around the case where the brightness-down key (or other special keys, such as the volume keys) triggers this code and increases the brightness (which obviously isn't the user's intent when pressing brightness-down). This code used to work but it appears to be broken now. There's much discussion at http://crosbug.com/p/48210, but the brief summary is that the Chrome code in ui/chromeos/user_activity_power_manager_notifier.cc expects to see e.g. keycodes 216 (BRIGHTNESS_UP) and 217 (BRIGHTNESS_DOWN) but instead sees 117 (F6) and 118 (F7), so it resports these events as regular user activity instead of saying that they're brightness keys. It sounds like chromeos::EventRewriter may be running *after* UserActivityDetector sees these key events instead of before it. Presumably it used to run before (and should be updated to do this again).
,
May 13 2016
(The peppy where I'm seeing this has had some touchpad jankiness issues recently, so it's also possible that bogus events are being generated and reported as user activity.)
,
May 13 2016
event-rewriter is expected to trigger *before* the other event-handlers. If that is not happening, then that does sound like a bug. Unfortunately, I am not a good owner to investigate this right now. Perhaps abodenha@ can find an owner?
,
May 14 2016
I started tracing through the code today. I'm still fuzzy on the difference between EventSource and PlatformEventSource. I probably won't be able to do much debugging until week after next, so if Albert has someone with cycles to learn more about Chromium input, that'd be great. :-P
,
May 18 2016
,
Jul 1 2016
The CBC topic seems unrelated to this bug. I've filed https://github.com/dnschneid/crouton/issues/2661 to track what I think is the issue there.
,
Sep 7 2016
Sammie, have you looked into this at all?
,
Sep 7 2016
Hi derat@, sorry, fell of my radar. I can look at this at the end of the week.
,
Sep 13 2016
Sammie looked into this and thinks it might've been caused by https://codereview.chromium.org/1074543002/ (afakhry@, April 2015, issue 462735 ). As I understand it, UserActivityDetector was switched from being an EventHandler to a PlatformEventObserver to ensure that it would continue to see events while menus are open. If platform events don't get rewritten (maybe this is intentional), then UAD no longer sees the rewritten events. One easy way to work around this (if we can't rewrite platform events) might be restoring the original code so that UAD is both an EventHandler *and* a PlatformEventObserver. This will result in it seeing events twice, but that may be harmless (or there may be some way for it to de-dupe them). The rewritten event will hopefully come first (as the platform events are observed via DidProcessEvent()), in which case UAD will report the rewritten event to powerd instead of the original one (it only reports once every five seconds). What do people more familiar with events think?
,
Sep 21 2016
derat@, my CL in #10 is indeed why the UAD is not getting the rewritten events. I agree it would be nicer if the platform event can be rewritten, but I'm not sure if we can (or want) to do that. It seems it will require a lot of plumbing. A lot has changed in the menus code lately, but we could consider making the UserActivityDetector an EventHandler (only) again, and making sure that it always remains the very first PreTargetHandler (Even before the MenuPreTargetHandler when the menu is present). That way menus don't block events from reaching the UAD, and UAD gets the rewritten events.
,
Sep 21 2016
I'm fine with UserActivityDetector being the first pre-target handler, especially if it needs to process these events while menus are open. My slight concern with this is making sure that we don't encounter a similar problem going forward. EventTarget::PrependPreTargetHandler is currently used in 9 non test locations. And if we have to support transient handlers like the MenuPreTargetHandler, it becomes difficult to guarantee what UAD wants.
,
Sep 22 2016
Thanks! I'm taking a look at this. I'm also pretty scared by how EventTarget::PrependPreTargetHandler() is called from all over the place. It was easier to see and enforce ordering dependencies when handlers were just (I think) added from ash::Shell::Init(), but now there are a bunch of different places where classes (often from their c'tors) are saying "I want to be first". I can think of a couple of things we could try: a) Move these to AddPreTargetHandler() calls in Shell::Init() where possible to make the ordering clearer. b) Add some tests that wait for everything to be initialized and verify that certain ordering constraints are upheld (e.g. UAD is first). Transient handlers won't be covered, but maybe they could just have their own tests that verify this. :-/ c) Add methods that can be used to enforce ordering when adding a handler (e.g. specify that you should be inserted before or after another already-added handler). This could get quite tricky, though, and handlers may not hold pointers to the other handlers that they need to come before or after.
,
Sep 22 2016
How about a priority queue of event handlers? Each handler is added with a priority, for example: (smaller value is higher) - UAD_PRIORITY = 0, - MENU_HANDLER_PRIORITY =1, - ... That way it's clear which one should come first, and the queue guarantees the order.
,
Sep 22 2016
#14: I like that suggestion, assuming that there aren't any objections to hardcoding knowledge about different "layers" of handlers in aura.
,
Sep 22 2016
,
Sep 22 2016
I think user-activity should be an explicit thing, so aura/ash should explicitly notify the activity-monitor when some user activity happens, instead of implicitly notifying it through an EventHandler. That's how we are doing it in the UI server (mus) (code: https://cs.chromium.org/chromium/src/services/ui/ws/display.cc?sq=package:chromium&dr=CSs&rcl=1474529952&l=296)
,
Sep 23 2016
#17: I don't have any objections to doing that, but I also don't have much familiarity with Chrome's event-routing code. :-) Is there a natural place where ash (or even aura) can hand ui::Events to UserActivityDetector after they've been rewritten but before the pre-target handlers run? I was hopeful that AshWindowTreeHostPlatform::DispatchEvent() would be the spot for this, but it doesn't look like keyboard events go through there. (I've also verified that switching UserActivityDetector back to being a pre-target handler fixes the brightness-key issue described here but reintroduces the issue where it doesn't see key events consumed by menus.)
,
Feb 17 2017
,
Feb 20 2017
,
Mar 16 2017
Just as a note for myself: this is going to be broken in mash as well, but it may also be harder to fix there since the UserActivityMonitor service doesn't pass the underlying event to UserActivityObserver.
,
Mar 19 2018
Katie, I noticed that your change at https://crrev.com/c/967358 makes some improvements in how we order EventHandlers. Any chance that that will help with this as well?
,
Mar 30 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51 commit b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51 Author: Katie D <katie@chromium.org> Date: Fri Mar 30 18:47:36 2018 Adds priorities to EventHandlers, instead of using Prepend/Add. This will enable us to add a super-high priority, the accessibility priority, which can go first among the EventHandlers. It may also help with other bugs related to EventHandler ordering. Bug: 819860 ,599883 Change-Id: Ie35cc01f8b8d5a15388814e79c7b189d30e7c271 Reviewed-on: https://chromium-review.googlesource.com/967358 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Katie Dektar <katie@chromium.org> Cr-Commit-Position: refs/heads/master@{#547223} [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/ash/display/touch_calibrator_controller_unittest.cc [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/ash/drag_drop/drag_drop_controller.cc [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/ash/magnifier/docked_magnifier_controller.cc [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/ash/magnifier/magnification_controller_unittest.cc [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/ash/shell.cc [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/ash/shell_unittest.cc [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/ash/system/power/power_button_display_controller.cc [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/ash/system/power/power_button_screenshot_controller.cc [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/ash/utility/screenshot_controller.cc [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/ash/wm/system_gesture_event_filter_unittest.cc [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/ash/wm/window_cycle_event_filter_classic.cc [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/chrome/browser/chromeos/login/ui/input_events_blocker.cc [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/chrome/browser/ui/views/javascript_app_modal_event_blocker_x11.cc [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/components/exo/wm_helper.cc [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/components/ui_devtools/views/overlay_agent.cc [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/ui/events/BUILD.gn [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/ui/events/event_target.cc [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/ui/events/event_target.h [add] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/ui/events/event_target_unittest.cc [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/ui/events/test/events_test_utils.h [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/ui/views/bubble/tray_bubble_view.cc [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/ui/views/controls/button/menu_button_unittest.cc [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/ui/views/controls/menu/menu_pre_target_handler.cc [modify] https://crrev.com/b68a552d1f6cb7cccf72f3f9ff8f6c1310713d51/ui/wm/core/focus_controller_unittest.cc
,
Sep 17
Hi, all Wonder if there is any recent update for this issue because we still saw this issue on GLK projects. Thanks.
,
Sep 17
No, please star the issue to receive updates.
,
Jan 12
This is still happening. I'm not sure of the current state of event-rewriting, but adding some people who may have thoughts or may know if this is easier (or harder) to address with recent mustash changes.
,
Jan 12
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/1b09b7b7fd8206e6c470fc745a606fedd2ed861b commit 1b09b7b7fd8206e6c470fc745a606fedd2ed861b Author: Justin TerAvest <teravest@chromium.org> Date: Sat Jan 12 07:54:31 2019 power: Fix brightness-down description This commit fixes documentation in screen_brightness.md by removing a section that claims that the brightness-down key will not cause the brightness to increase. crbug.com/599883 tracks reports where users are seeing the brightness increase anyway. Since there have been reports for well over a year, the documentation should be consistent with the overall behavior. BUG=b:121156976,chromium:599883 TEST=None Change-Id: I8cebee2d38e32b4d5fa9d369ac5f8f30e26d494b Reviewed-on: https://chromium-review.googlesource.com/1407813 Commit-Ready: Justin TerAvest <teravest@chromium.org> Tested-by: Justin TerAvest <teravest@chromium.org> Reviewed-by: Dan Erat <derat@chromium.org> [modify] https://crrev.com/1b09b7b7fd8206e6c470fc745a606fedd2ed861b/power_manager/docs/screen_brightness.md |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by derat@chromium.org
, May 13 2016