New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 884275 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 25
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug
Team-Accessibility



Sign in to add a comment

ash::Shell::OnFirstSessionStarted() doesn't fire during login

Project Member Reported by jamescook@chromium.org, Sep 14

Issue description

linux-chromeos ToT 47421c6017f558f53945d65ffc509305932a2345

While working on another issue I noticed OnFirstSessionStarted() doesn't fire, either while going through OOBE or during login.

I suspect this breaks some accessibility features in kiosk mode (magnifier) and for hotrod (long-press for spoken feedback):

https://cs.chromium.org/chromium/src/ash/shell.cc?q=shell::onfirstsession&sq=package:chromium&l=1438

I suspect it regressed in May:

"ChromeShellDelegate: Cleanup: move/remove some methods"
https://chromium-review.googlesource.com/c/chromium/src/+/1070771

I have a fix. I just wonder if we should backport it.

 
Cc: tbarzic@chromium.org michae...@chromium.org
Yikes, oops, apologies.

+michaelpg@, +tbarzic@ for input on backporting.

Project Member

Comment 2 by bugdroid1@chromium.org, Sep 14

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

commit 9a19742ed78ab493fb308903b84bf41aab2cdacc
Author: James Cook <jamescook@chromium.org>
Date: Fri Sep 14 20:18:27 2018

chromeos: Fix SessionObserver::OnFirstSessionStarted() not firing

During OOBE and normal user login, when
SessionController::SetUserSessionOrder() is called there is an
existing user session, so last_active_account_id.is_valid() returns
true and the observer never fires.

I suspect this breaks some accessibility features in kiosk mode
(magnifier keyboard control) and for hotrod (long-press for
spoken feedback).

It's probably been broken since the observer was added in May:
https://chromium-review.googlesource.com/c/chromium/src/+/1070771

Bug:  884275 
Test: added to ash_unittests
Change-Id: Idbe4f20b7426742703995a05c7ebb1c5c8f1a7c8
Reviewed-on: https://chromium-review.googlesource.com/1226876
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591450}
[modify] https://crrev.com/9a19742ed78ab493fb308903b84bf41aab2cdacc/ash/session/session_controller.cc
[modify] https://crrev.com/9a19742ed78ab493fb308903b84bf41aab2cdacc/ash/session/session_controller_unittest.cc

Given we haven't heard anything from enterprise or hotrod teams after going an entire stable cycle, I wouldn't consider backporting to be a high priority -- maybe to M70 as the fix looks fairly innocuous.
Labels: Merge-Request-70 M-70
M70 seems reasonable to me for backport. The fix is pretty simple.

Project Member

Comment 5 by sheriffbot@chromium.org, Sep 17

Labels: -Merge-Request-70 Merge-Review-70 Hotlist-Merge-Review
This bug requires manual review: M70 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: geohsu@chromium.org
geohsu, can I backport this?


Owner: geohsu@chromium.org
Status: Assigned (was: Started)
geohsu, can I backport this? Assign back to me if so.
Labels: -Merge-Review-70 Merge-Approved-70
Owner: jamescook@chromium.org
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 24

Labels: -merge-approved-70 merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/65feb8b62e4afd4f1dfbc732a43859731b20061d

commit 65feb8b62e4afd4f1dfbc732a43859731b20061d
Author: James Cook <jamescook@chromium.org>
Date: Mon Sep 24 21:14:18 2018

M70: chromeos: Fix SessionObserver::OnFirstSessionStarted() not firing

During OOBE and normal user login, when
SessionController::SetUserSessionOrder() is called there is an
existing user session, so last_active_account_id.is_valid() returns
true and the observer never fires.

I suspect this breaks some accessibility features in kiosk mode
(magnifier keyboard control) and for hotrod (long-press for
spoken feedback).

It's probably been broken since the observer was added in May:
https://chromium-review.googlesource.com/c/chromium/src/+/1070771

Bug:  884275 
Test: added to ash_unittests
Change-Id: Idbe4f20b7426742703995a05c7ebb1c5c8f1a7c8
Reviewed-on: https://chromium-review.googlesource.com/1226876
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#591450}(cherry picked from commit 9a19742ed78ab493fb308903b84bf41aab2cdacc)
Reviewed-on: https://chromium-review.googlesource.com/1241276
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#600}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/65feb8b62e4afd4f1dfbc732a43859731b20061d/ash/session/session_controller.cc
[modify] https://crrev.com/65feb8b62e4afd4f1dfbc732a43859731b20061d/ash/session/session_controller_unittest.cc

Labels: Merge-Merged-70-3538
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/65feb8b62e4afd4f1dfbc732a43859731b20061d

Commit: 65feb8b62e4afd4f1dfbc732a43859731b20061d
Author: jamescook@chromium.org
Commiter: jamescook@chromium.org
Date: 2018-09-24 21:14:18 +0000 UTC

M70: chromeos: Fix SessionObserver::OnFirstSessionStarted() not firing

During OOBE and normal user login, when
SessionController::SetUserSessionOrder() is called there is an
existing user session, so last_active_account_id.is_valid() returns
true and the observer never fires.

I suspect this breaks some accessibility features in kiosk mode
(magnifier keyboard control) and for hotrod (long-press for
spoken feedback).

It's probably been broken since the observer was added in May:
https://chromium-review.googlesource.com/c/chromium/src/+/1070771

Bug:  884275 
Test: added to ash_unittests
Change-Id: Idbe4f20b7426742703995a05c7ebb1c5c8f1a7c8
Reviewed-on: https://chromium-review.googlesource.com/1226876
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#591450}(cherry picked from commit 9a19742ed78ab493fb308903b84bf41aab2cdacc)
Reviewed-on: https://chromium-review.googlesource.com/1241276
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#600}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
Status: Fixed (was: Assigned)

Sign in to add a comment