Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2737663005:
Add custom accessibility actions to ReadingList (Closed)
|
Created:
3 years, 9 months ago by gambard Modified:
3 years, 9 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, aboxhall+watch_chromium.org, pkl (ping after 24h if needed), nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, noyau+watch_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, marq+watch_chromium.org, stkhapugin, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd custom accessibility actions to ReadingList
This CL adds 6 custom accessibility actions to Reading List:
- Delete entry
- Mark Read/Unread
- Open in New Tab
- Open in New Incognito Tab
- Open Offline in New Tab
- Copy Link
BUG=676290
TEST=Enable custom action with VoiceOver.
Test that the Reading List entries have the custom actions described in the CL
description.
Test that the current edit behavior (using the toolbar) is undisturbed.
Review-Url: https://codereview.chromium.org/2737663005
Cr-Commit-Position: refs/heads/master@{#456055}
Committed: https://chromium.googlesource.com/chromium/src/+/515d5521f09b617a56e33e43ef5fc5a6d34133ea
Patch Set 1 #Patch Set 2 : Add mark Read/Unread #
Total comments: 30
Patch Set 3 : Address comments after split #
Total comments: 6
Patch Set 4 : Fix #Patch Set 5 : Address comments #Messages
Total messages: 17 (7 generated)
Description was changed from ========== Add custom accessibility actions to ReadingList This CL adds 5 custom accessibility actions to Reading List: - Delete entry - Open in New Tab - Open in New Incognito Tab - Open Offline in New Tab - Copy Link BUG=676290 TEST=Enable custom action with VoiceOver. Test that the Reading List entries have the custom actions described in the CL description. ========== to ========== Add custom accessibility actions to ReadingList This CL adds 5 custom accessibility actions to Reading List: - Delete entry - Mark Read/Unread - Open in New Tab - Open in New Incognito Tab - Open Offline in New Tab - Copy Link BUG=676290 TEST=Enable custom action with VoiceOver. Test that the Reading List entries have the custom actions described in the CL description. Test that the current edit behavior (using the toolbar) is undisturbed. ==========
gambard@chromium.org changed reviewers: + lpromero@chromium.org, olivierrobin@chromium.org
PTAL. https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm (right): https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:1041: [self reconfigureCellsAtIndexPaths:sortedIndexPaths]; As the custom actions are set when the cell is configured, this is the only way I found to change the "Mark Read" to "Mark Unread" when the entry is moved from one section to the other.
Update the description as there are 6 (technically 7 ;) actions. There are a lot of things going on in this CL which makes it hard to review. You might have first refactored the code (moved methods to public, changed methods that took no argument to now take arguments, etc.) https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.h (right): https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.h:59: - (void)readingListCollectionViewController: How come this was not needed before accessibility? https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm (left): https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:824: NSArray* indexPaths = [self.collectionView.indexPathsForSelectedItems copy]; Don't you need to sort as in the two other methods? https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm (right): https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:140: // Deletes all the items at |indexPath|. indexPaths https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:401: const ReadingListEntry* readingListentry = readingList_E_ntry, here and in the other methods too. https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:404: if (!readingListentry) { Can you add a comment on why this is needed? https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:421: if (readingListentry->IsRead()) { Can't you just call [self isEntryRead:entry] instead of repeating the impl? You'll need to modify the flow a bit here. https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:432: [self deleteItemsAtIndexPaths:indexPaths]; Nit: [self deleteItemsAtIndexPaths:@[ [self.collectionViewModel indexPathForItem:entry inSectionWithIdentifier:sectionIdentifier] ]]; https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:722: .indexPathsForSelectedItems copy]]; Why the copy here and not above? https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:826: ReadingListCollectionViewController* strongSelf = weakSelf; The goal of strongSelf is to return early if weakSelf is nil. Otherwise, keep using weakSelf. https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:1041: [self reconfigureCellsAtIndexPaths:sortedIndexPaths]; On 2017/03/08 16:48:52, gambard wrote: > As the custom actions are set when the cell is configured, this is the only way > I found to change the "Mark Read" to "Mark Unread" when the entry is moved from > one section to the other. Ack. Makes sense. https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h (right): https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h:8: #import "components/reading_list/ios/reading_list_entry.h" This is an issue. The UI layer shouldn't know about this model class. Ideally (New Architecture), the view controller should declare a similar enum and never know about this one. To mitigate, until Reading List has a coordinator and mediator, can you make the item agnostic of this? https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h:30: @property(nonatomic, weak) Add a comment. https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm (right): https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:224: openInNewIncognitoTabAction, copyURLAction I don't like the repetition with above. Create a mutable array and add items one by one, conditionally. https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:230: return YES; I have seen that it needs to return a bool (https://developer.apple.com/reference/uikit/uiaccessibilitycustomaction/16204...), but not what the returned value does. Any idea? https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_coordinator.mm (right): https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_coordinator.mm:255: #pragma mark - Private These are not private anymore.
Description was changed from ========== Add custom accessibility actions to ReadingList This CL adds 5 custom accessibility actions to Reading List: - Delete entry - Mark Read/Unread - Open in New Tab - Open in New Incognito Tab - Open Offline in New Tab - Copy Link BUG=676290 TEST=Enable custom action with VoiceOver. Test that the Reading List entries have the custom actions described in the CL description. Test that the current edit behavior (using the toolbar) is undisturbed. ========== to ========== Add custom accessibility actions to ReadingList This CL adds 6 custom accessibility actions to Reading List: - Delete entry - Mark Read/Unread - Open in New Tab - Open in New Incognito Tab - Open Offline in New Tab - Copy Link BUG=676290 TEST=Enable custom action with VoiceOver. Test that the Reading List entries have the custom actions described in the CL description. Test that the current edit behavior (using the toolbar) is undisturbed. ==========
Splitting up the CL. Those comments are addressed in https://codereview.chromium.org/2745563002/ https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.h (right): https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.h:59: - (void)readingListCollectionViewController: On 2017/03/09 12:57:55, lpromero wrote: > How come this was not needed before accessibility? It was private. It is only used in long press context menu which is handled by the coordinator. https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_coordinator.mm (right): https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_coordinator.mm:255: #pragma mark - Private On 2017/03/09 12:57:56, lpromero wrote: > These are not private anymore. Done.
Splitting the addressed comments to: https://codereview.chromium.org/2740833003/ https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm (left): https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:824: NSArray* indexPaths = [self.collectionView.indexPathsForSelectedItems copy]; On 2017/03/09 12:57:55, lpromero wrote: > Don't you need to sort as in the two other methods? No because you delete all the element at the same time, whereas in the other functions of you move them one by one, changing the index path at each time. https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm (right): https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:140: // Deletes all the items at |indexPath|. On 2017/03/09 12:57:55, lpromero wrote: > indexPaths Done. https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:722: .indexPathsForSelectedItems copy]]; On 2017/03/09 12:57:55, lpromero wrote: > Why the copy here and not above? I don't know. Probably legacy code. I have removed it. https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:826: ReadingListCollectionViewController* strongSelf = weakSelf; On 2017/03/09 12:57:55, lpromero wrote: > The goal of strongSelf is to return early if weakSelf is nil. Otherwise, keep > using weakSelf. Done.
Splitting addressed comments to https://codereview.chromium.org/2743713002/ https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h (right): https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h:8: #import "components/reading_list/ios/reading_list_entry.h" On 2017/03/09 12:57:55, lpromero wrote: > This is an issue. The UI layer shouldn't know about this model class. Ideally > (New Architecture), the view controller should declare a similar enum and never > know about this one. To mitigate, until Reading List has a coordinator and > mediator, can you make the item agnostic of this? It has a coordinator but no mediator. I will do it in a followup CL. https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h:30: @property(nonatomic, weak) On 2017/03/09 12:57:55, lpromero wrote: > Add a comment. Done. https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm (right): https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:224: openInNewIncognitoTabAction, copyURLAction On 2017/03/09 12:57:55, lpromero wrote: > I don't like the repetition with above. Create a mutable array and add items one > by one, conditionally. Done. https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.mm:230: return YES; On 2017/03/09 12:57:55, lpromero wrote: > I have seen that it needs to return a bool > (https://developer.apple.com/reference/uikit/uiaccessibilitycustomaction/16204...), > but not what the returned value does. Any idea? No idea... My guess is that it returns whether the action was a success or not (there is an audio hint when executing the action).
Thanks! I have split the CL and addressed the last comments. PTAL. https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm (right): https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:401: const ReadingListEntry* readingListentry = On 2017/03/09 12:57:55, lpromero wrote: > readingList_E_ntry, here and in the other methods too. Done. https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:404: if (!readingListentry) { On 2017/03/09 12:57:55, lpromero wrote: > Can you add a comment on why this is needed? Done. https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:421: if (readingListentry->IsRead()) { On 2017/03/09 12:57:55, lpromero wrote: > Can't you just call [self isEntryRead:entry] instead of repeating the impl? > You'll need to modify the flow a bit here. It would mean isEntryRead: is return 3 states: yes, no, and "no entry". I have added a new method to do it. https://codereview.chromium.org/2737663005/diff/20001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:432: [self deleteItemsAtIndexPaths:indexPaths]; On 2017/03/09 12:57:55, lpromero wrote: > Nit: > > [self deleteItemsAtIndexPaths:@[ > [self.collectionViewModel indexPathForItem:entry > inSectionWithIdentifier:sectionIdentifier] > ]]; Done.
lgtm Thanks for splitting the CLs! https://codereview.chromium.org/2737663005/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm (right): https://codereview.chromium.org/2737663005/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:123: // an entry, return nullptr and reload the UI. returns and reloads https://codereview.chromium.org/2737663005/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:408: if (!readingListentry) { s/e/E here and below (idem for the other methods). https://codereview.chromium.org/2737663005/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:701: [self reloadData]; For the future: will it be possible to subscribe to model updates and update in place instead of a master reloadData?
Thanks! https://codereview.chromium.org/2737663005/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm (right): https://codereview.chromium.org/2737663005/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:123: // an entry, return nullptr and reload the UI. On 2017/03/09 14:49:56, lpromero wrote: > returns and reloads Done. https://codereview.chromium.org/2737663005/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:408: if (!readingListentry) { On 2017/03/09 14:49:56, lpromero wrote: > s/e/E here and below (idem for the other methods). Done. https://codereview.chromium.org/2737663005/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller.mm:701: [self reloadData]; On 2017/03/09 14:49:56, lpromero wrote: > For the future: will it be possible to subscribe to model updates and update in > place instead of a master reloadData? The class is observing the model but it is observing all changes. It would be possible to add an intermediary model and apply the sync changes in place, but it would add a lot of complexity and it is not expected that the sync happens often.
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2737663005/#ps80001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489154754892310,
"parent_rev": "d1b61106131df8a2eca7d0a71ea026d6e3707680", "commit_rev":
"515d5521f09b617a56e33e43ef5fc5a6d34133ea"}
Message was sent while issue was closed.
Description was changed from ========== Add custom accessibility actions to ReadingList This CL adds 6 custom accessibility actions to Reading List: - Delete entry - Mark Read/Unread - Open in New Tab - Open in New Incognito Tab - Open Offline in New Tab - Copy Link BUG=676290 TEST=Enable custom action with VoiceOver. Test that the Reading List entries have the custom actions described in the CL description. Test that the current edit behavior (using the toolbar) is undisturbed. ========== to ========== Add custom accessibility actions to ReadingList This CL adds 6 custom accessibility actions to Reading List: - Delete entry - Mark Read/Unread - Open in New Tab - Open in New Incognito Tab - Open Offline in New Tab - Copy Link BUG=676290 TEST=Enable custom action with VoiceOver. Test that the Reading List entries have the custom actions described in the CL description. Test that the current edit behavior (using the toolbar) is undisturbed. Review-Url: https://codereview.chromium.org/2737663005 Cr-Commit-Position: refs/heads/master@{#456055} Committed: https://chromium.googlesource.com/chromium/src/+/515d5521f09b617a56e33e43ef5f... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/515d5521f09b617a56e33e43ef5f... |
