[iOS] Reading List: iPad EG tests failing without flag enabled |
|||||||||
Issue description
,
Jul 20
,
Jul 20
I was debugging this today and discovered that this is happening on iPad only. EG is able to find and tap a button, but for some reason the tap fails unbeknownst to EG. I've set breakpoints in the button's target selector, and they are never reached. The toolbar buttons are functional when using the app; this failure is specific to EG failing to tap the button.
,
Jul 23
,
Jul 23
,
Jul 23
This bug was filed 1 day after M69 branch which doesn't give much time to resolve before the first beta on Wednesday. This is also already being tracked per M-69 and Proj-UIRefresh labels. Removing RBB.
,
Jul 23
This bug was introduced by crrev.com/c/1139901, and is fixed by crrev.com/c/1147310. The fix is in the CQ now, so will likely be mergeable later this afternoon. I don't know how we want to prioritize merging this since it's a fix for the pre-UI Refresh codebase, but since M69 is likely the last branch with this flag, we should probably merge it for that branch. Regardless of RBB status, I'll request merge once the fix is landed, and we can decide whether it's something we'd like to do for M69.
,
Jul 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f4815dd13856719927d601287d825a6265c4729c commit f4815dd13856719927d601287d825a6265c4729c Author: Kurt Horimoto <kkhorimoto@chromium.org> Date: Tue Jul 24 02:20:09 2018 [iOS] Set a11y identifiers directly to UIButtons for legacy toolbar. LegacyReadingListToolbarButton is actually a plain UIView superview of the UIButton handling touch events, so setting the a11y identifier on the superview was not passing touches to the underlying button in EG tests. This CL updates LegacyReadingListToolbarButton to take an a11y identifier that's applied to the underlying UIButton, allowing touches to be handled properly. Bug: 866012 Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I1e98ae482bd21e1e913d500d53f43526fadb62af Reviewed-on: https://chromium-review.googlesource.com/1147310 Commit-Queue: Justin Cohen <justincohen@chromium.org> Reviewed-by: Justin Cohen <justincohen@chromium.org> Reviewed-by: Peter Lee <pkl@chromium.org> Cr-Commit-Position: refs/heads/master@{#577407} [modify] https://crrev.com/f4815dd13856719927d601287d825a6265c4729c/ios/chrome/browser/ui/reading_list/legacy_reading_list_toolbar.mm [modify] https://crrev.com/f4815dd13856719927d601287d825a6265c4729c/ios/chrome/browser/ui/reading_list/legacy_reading_list_toolbar_button.h [modify] https://crrev.com/f4815dd13856719927d601287d825a6265c4729c/ios/chrome/browser/ui/reading_list/legacy_reading_list_toolbar_button.mm
,
Jul 24
Fix has landed and cycled through the trybots a few times: https://uberchromegw.corp.google.com/i/internal.bling.main/builders/bijou-simulator
,
Jul 24
Requesting M69 merge despite removal of RBB label. This fixes tests for non-UI Refresh reading list, so merging this to branch would give us some extra confidence in that implementation in case we need to turn off the flag for some reason.
,
Jul 24
Ok thanks for fixing this quickly. Let's get canary verification before merge please.
,
Jul 24
The fix doesn't actually change any functionality, it just updates underlying view identifier information that's used in EG tests. Not sure if my CL landed before today's canary, so I can verify tomorrow that it still works. The only expected change from my CL is that the EG tests should pass without the UI refresh flag enabled, which has been occurring on the bijou-simulator bot linked in Comment #9.
,
Jul 24
Ok thanks for clarifying. You can verify yourself and then update this bug with your findings, so I can address the merge request.
,
Jul 25
Your change meets the bar and is auto-approved for M69. Please go ahead and merge the CL to branch 3497 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 25
Checked it out on canary this morning; looks good! Kariah, should I land this, or wait for a non-automated approval?
,
Jul 25
In general, please wait for my manual approval just for M69 since we have so many merges. This is approved to merge!
,
Jul 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a8489fa0a100f9a7813303026f0b226d6a1fba97 commit a8489fa0a100f9a7813303026f0b226d6a1fba97 Author: Kurt Horimoto <kkhorimoto@chromium.org> Date: Wed Jul 25 20:49:31 2018 [iOS] Set a11y identifiers directly to UIButtons for legacy toolbar. LegacyReadingListToolbarButton is actually a plain UIView superview of the UIButton handling touch events, so setting the a11y identifier on the superview was not passing touches to the underlying button in EG tests. This CL updates LegacyReadingListToolbarButton to take an a11y identifier that's applied to the underlying UIButton, allowing touches to be handled properly. Bug: 866012 Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I1e98ae482bd21e1e913d500d53f43526fadb62af Reviewed-on: https://chromium-review.googlesource.com/1147310 Commit-Queue: Justin Cohen <justincohen@chromium.org> Reviewed-by: Justin Cohen <justincohen@chromium.org> Reviewed-by: Peter Lee <pkl@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#577407}(cherry picked from commit f4815dd13856719927d601287d825a6265c4729c) Reviewed-on: https://chromium-review.googlesource.com/1150584 Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#87} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/a8489fa0a100f9a7813303026f0b226d6a1fba97/ios/chrome/browser/ui/reading_list/legacy_reading_list_toolbar.mm [modify] https://crrev.com/a8489fa0a100f9a7813303026f0b226d6a1fba97/ios/chrome/browser/ui/reading_list/legacy_reading_list_toolbar_button.h [modify] https://crrev.com/a8489fa0a100f9a7813303026f0b226d6a1fba97/ios/chrome/browser/ui/reading_list/legacy_reading_list_toolbar_button.mm |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by kkhorimoto@chromium.org
, Jul 20