New issue
Advanced search Search tips

Issue 866012 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 24
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug
Q2



Sign in to add a comment

[iOS] Reading List: iPad EG tests failing without flag enabled

Project Member Reported by kkhorimoto@chromium.org, Jul 20

Issue description

Labels: Q2
Cc: linds...@chromium.org
Summary: [iOS] Reading List: iPad EG tests failing without flag enabled (was: [iOS] Reading List: EG tests failing without flag enabled)
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.
Cc: justincohen@chromium.org
Labels: -ReleaseBlock-Beta
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.
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.
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Cc: kariahda@chromium.org
Status: Fixed (was: Started)
Fix has landed and cycled through the trybots a few times: https://uberchromegw.corp.google.com/i/internal.bling.main/builders/bijou-simulator
Labels: Merge-Request-69
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.
Ok thanks for fixing this quickly. Let's get canary verification before merge please.
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.
Ok thanks for clarifying. You can verify yourself and then update this bug with your findings, so I can address the merge request.
Project Member

Comment 14 by sheriffbot@chromium.org, Jul 25

Labels: -Merge-Request-69 Hotlist-Merge-Approved Merge-Approved-69
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
Checked it out on canary this morning; looks good!  Kariah, should I land this, or wait for a non-automated approval?
In general, please wait for my manual approval just for M69 since we have so many merges.

This is approved to merge!
Project Member

Comment 17 by bugdroid1@chromium.org, Jul 25

Labels: -merge-approved-69 merge-merged-3497
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