Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2207573002:
Implement 3 Mac accessibility parameterized attributes (Closed)
|
Created:
4 years, 4 months ago by dmazzoni Modified:
4 years, 4 months ago Reviewers:
nektarios, nektar CC:
aboxhall+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, jam, je_julie, nektar+watch_chromium.org, yuzo+watch_chromium.org, Peter Beverloo Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement 3 Mac accessibility parameterized attributes
This change implements support for AXBoundsForTextMarkerRange,
AXTextMarkerRangeForUnorderedTextMarkers, and AXIndexForChildUIElement.
The noticeable benefit of this change is that VoiceOver now draws its
bounding box around the cursor or selection when you're typing or
selecting in a text field.
BUG=633598
Committed: https://crrev.com/958086533d06dfbd0f172405036e78be79ba283f
Cr-Commit-Position: refs/heads/master@{#409712}
Patch Set 1 #Patch Set 2 : Address feedback #Patch Set 3 : Fix NSNumber return and indentation #Patch Set 4 : Switched to enum #
Messages
Total messages: 37 (20 generated)
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
dmazzoni@chromium.org changed reviewers: + nektar@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
+ if ([attribute isEqualToString:
+ NSAccessibilityBoundsForTextMarkerRangeParameterizedAttribute]) {
Is the indentation wrong here?
+ BrowserAccessibility* start_object;
+ BrowserAccessibility* end_object;
+ int start_offset, end_offset;
I know that the whole method is like this, but isn't it true that if the method
is an Objective C method, it should follow Objective C style? So variables
should not be using underscores.
+ if ([attribute isEqualToString:
+
NSAccessibilityTextMarkerRangeForUnorderedTextMarkersParameterizedAttribute]) {
Perhaps the indentation is wrong here too?
+ if (![parameter isKindOfClass:[NSArray self]])
+ return nil;
Shouldn't it say "isKindOfClass:[NSArray class]?
BrowserAccessibilityManager::IsBeforeInTreeOrder
Should we return an int here that will signify the tree order. -1 before, 0
equal and 1 after.
+ if (object1.IsDescendantOf(&object2)) {
+ *is_in_order = false;
+ return true;
+ }
+ return false;
Should we NOTREACHED() before returning false?
+ if (!in_order) {
+ first = &end_object;
+ last = &start_object;
+ }
I think it's clearer to say std::swap here too.
+ const BrowserAccessibility* current = first;
+ while (current) {
+ if (current->IsTextOnlyObject()) {
Better use a "do...while" loop here.
+ if (in_order)
+ start_char_index = start_offset;
+ else
+ end_char_index = start_offset;
I think I read in the style guide that if there is an else clause there should
always be braces?
+ // Sets |out_is_before| to true if |object1| comes before |object2|
+ // in tree order, and false if the objects are the same or not in the
+ // same tree.
Should we say which tree order are we using? Pre-order or post-order.
In the test:
+ std::vector<int32_t> character_offsets;
+ character_offsets.push_back(10); // 0
+ character_offsets.push_back(21); // 1
+ character_offsets.push_back(33); // 2
I'd prefer to use an initializer list, character_offsets{10, 21, 33}.
+ EXPECT_EQ(gfx::Rect(10, 20, 23, 29).ToString(),
+ manager->GetLocalBoundsForRange(
+ *static_text_accessible, 2,
+ *static_text_accessible2, 1).ToString());
Did you try reversing the object arguments to see if we get the same rect?
+ if ([attribute isEqualToString:
+
NSAccessibilityTextMarkerRangeForUnorderedTextMarkersParameterizedAttribute]) {
+ if (![parameter isKindOfClass:[NSArray self]])
+ return nil;
+
+ NSArray* array = parameter;
+ id first = [array objectAtIndex:0];
+ id second = [array objectAtIndex:1];
Please verify that the array has indeed two elements.
Re: AXIndexForChildUIElement I can't find it in the WebKit source code. git grep .*IndexForChildUIElement.*" I remember seeing it before but are you sure it's still necessary?
On 2016/08/03 at 15:28:14, nektar wrote:
> + if ([attribute isEqualToString:
> + NSAccessibilityBoundsForTextMarkerRangeParameterizedAttribute]) {
> Is the indentation wrong here?
>
> + BrowserAccessibility* start_object;
> + BrowserAccessibility* end_object;
> + int start_offset, end_offset;
> I know that the whole method is like this, but isn't it true that if the
method is an Objective C method, it should follow Objective C style? So
variables should not be using underscores.
You're right. Fixed throughout this change.
> + if ([attribute isEqualToString:
> +
NSAccessibilityTextMarkerRangeForUnorderedTextMarkersParameterizedAttribute]) {
> Perhaps the indentation is wrong here too?
Fixed
> + if (![parameter isKindOfClass:[NSArray self]])
> + return nil;
> Shouldn't it say "isKindOfClass:[NSArray class]?
You're right. Oddly it still worked.
> BrowserAccessibilityManager::IsBeforeInTreeOrder
> Should we return an int here that will signify the tree order. -1 before, 0
equal and 1 after.
There are also other error cases, though - like if one is not in the same
tree at all, or something like that - so we'd need at least four possible
return values. Do you think that'd be better, or is this more clear?
> + if (object1.IsDescendantOf(&object2)) {
> + *is_in_order = false;
> + return true;
> + }
> + return false;
> Should we NOTREACHED() before returning false?
No, I think it's possible we could reach this because VoiceOver could
call this with nodes that are detached from the tree or something weird
like that. We should be prepared for unusual corner cases.
> + if (!in_order) {
> + first = &end_object;
> + last = &start_object;
> + }
> I think it's clearer to say std::swap here too.
Done
> + const BrowserAccessibility* current = first;
> + while (current) {
> + if (current->IsTextOnlyObject()) {
> Better use a "do...while" loop here.
Done
> + if (in_order)
> + start_char_index = start_offset;
> + else
> + end_char_index = start_offset;
> I think I read in the style guide that if there is an else clause there should
always be braces?
Done
> + // Sets |out_is_before| to true if |object1| comes before |object2|
> + // in tree order, and false if the objects are the same or not in the
> + // same tree.
> Should we say which tree order are we using? Pre-order or post-order.
Done
> In the test:
> + std::vector<int32_t> character_offsets;
> + character_offsets.push_back(10); // 0
> + character_offsets.push_back(21); // 1
> + character_offsets.push_back(33); // 2
> I'd prefer to use an initializer list, character_offsets{10, 21, 33}.
Done
> + EXPECT_EQ(gfx::Rect(10, 20, 23, 29).ToString(),
> + manager->GetLocalBoundsForRange(
> + *static_text_accessible, 2,
> + *static_text_accessible2, 1).ToString());
> Did you try reversing the object arguments to see if we get the same rect?
Good idea, done. This caught a bug.
On 2016/08/03 at 15:32:19, nektar wrote:
> + if ([attribute isEqualToString:
> +
NSAccessibilityTextMarkerRangeForUnorderedTextMarkersParameterizedAttribute]) {
> + if (![parameter isKindOfClass:[NSArray self]])
> + return nil;
> +
> + NSArray* array = parameter;
> + id first = [array objectAtIndex:0];
> + id second = [array objectAtIndex:1];
>
> Please verify that the array has indeed two elements.
Done.
On 2016/08/03 at 15:42:03, nektar wrote:
> Re: AXIndexForChildUIElement
> I can't find it in the WebKit source code.
> git grep .*IndexForChildUIElement.*"
> I remember seeing it before but are you sure it's still necessary?
Yes, I logged all of the calls we're getting from VoiceOver that
we're not handling, and this was one of them.
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
+ BrowserAccessibilityCocoa* childCocoaObj = + (BrowserAccessibilityCocoa*)parameter; + BrowserAccessibility* child = [childCocoaObj browserAccessibility]; + if (!child) + return nil; + + if (child->GetParent() != browserAccessibility_) + return nil; + + return [NSNumber numberWithInt:child->GetIndexInParent()]; I think you can do return @(child->GetIndexInParent());
On 8/3/2016 2:28 PM, dmazzoni@chromium.org wrote: > On 2016/08/03 at 15:28:14, nektar wrote: > > + if ([attribute isEqualToString: > > + NSAccessibilityBoundsForTextMarkerRangeParameterizedAttribute]) { > > Is the indentation wrong here? Did you check this? I don't know but in all the three "if" statements you added for each new attribute, the indentation on the second line appears wrong in my diff. Please check all three. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> > BrowserAccessibilityManager::IsBeforeInTreeOrder
> > Should we return an int here that will signify the tree order. -1
> before, 0
> equal and 1 after.
>
> There are also other error cases, though - like if one is not in the same
> tree at all, or something like that - so we'd need at least four possible
> return values. Do you think that'd be better, or is this more clear?
Better create an enum.
enum class TreeOrderRelation {
Equal, Forward, Reverse, Descendant, Ancestor, NotInTree
};
--
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dmazzoni@chromium.org changed reviewers: + nektar@chromium.orgt - nektar@chromium.org
Take a look, I fixed the indentation and switched to an enum
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
+
+ if ([attribute isEqualToString:
+
NSAccessibilityTextMarkerRangeForUnorderedTextMarkersParameterizedAttribute]) {
+ if (![parameter isKindOfClass:[NSArray class]])
+ return nil;
+
+ NSArray* array = parameter;
I still don't see the check for [array count] == 2 here.
nektar@chromium.org changed reviewers: + nektar@chromium.org
lgtm
Latest version has this line immediately after the one you commented on: if (2 != [parameter count]) On Wed, Aug 3, 2016 at 3:26 PM <nektar@chromium.org> wrote: > lgtm > > > > https://codereview.chromium.org/2207573002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Implement 3 Mac accessibility parameterized attributes This change implements support for AXBoundsForTextMarkerRange, AXTextMarkerRangeForUnorderedTextMarkers, and AXIndexForChildUIElement. The noticeable benefit of this change is that VoiceOver now draws its bounding box around the cursor or selection when you're typing or selecting in a text field. BUG=633598 ========== to ========== Implement 3 Mac accessibility parameterized attributes This change implements support for AXBoundsForTextMarkerRange, AXTextMarkerRangeForUnorderedTextMarkers, and AXIndexForChildUIElement. The noticeable benefit of this change is that VoiceOver now draws its bounding box around the cursor or selection when you're typing or selecting in a text field. BUG=633598 Committed: https://crrev.com/958086533d06dfbd0f172405036e78be79ba283f Cr-Commit-Position: refs/heads/master@{#409712} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/958086533d06dfbd0f172405036e78be79ba283f Cr-Commit-Position: refs/heads/master@{#409712}
Message was sent while issue was closed.
I'm seeing the following non-fatal error in my local build after this CL: [528/29014] ACTION //ui/accessibility:ax_gen_schema_generator(//build/toolchain/linux:clang_x64) ../../ui/accessibility/ax_enums.idl(522) : Error : Unexpected comment after symbol after. I can also see this in the log files of the build bots, i.e. https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... So presumable everyone will get it. Would you mind moving the comment or fixing the IDL parser to mute the build output? (It's the only line when building "all" for me.)
Message was sent while issue was closed.
Description was changed from ========== Implement 3 Mac accessibility parameterized attributes This change implements support for AXBoundsForTextMarkerRange, AXTextMarkerRangeForUnorderedTextMarkers, and AXIndexForChildUIElement. The noticeable benefit of this change is that VoiceOver now draws its bounding box around the cursor or selection when you're typing or selecting in a text field. BUG=633598 Committed: https://crrev.com/958086533d06dfbd0f172405036e78be79ba283f Cr-Commit-Position: refs/heads/master@{#409712} ========== to ========== Implement 3 Mac accessibility parameterized attributes This change implements support for AXBoundsForTextMarkerRange, AXTextMarkerRangeForUnorderedTextMarkers, and AXIndexForChildUIElement. The noticeable benefit of this change is that VoiceOver now draws its bounding box around the cursor or selection when you're typing or selecting in a text field. BUG=633598 Committed: https://crrev.com/958086533d06dfbd0f172405036e78be79ba283f Cr-Commit-Position: refs/heads/master@{#409712} ==========
Message was sent while issue was closed.
Trying this: https://codereview.chromium.org/2207643005 On Thu, Aug 4, 2016 at 4:01 AM <peter@chromium.org> wrote: > I'm seeing the following non-fatal error in my local build after this CL: > > [528/29014] ACTION > > //ui/accessibility:ax_gen_schema_generator(//build/toolchain/linux:clang_x64) > ../../ui/accessibility/ax_enums.idl(522) : Error : Unexpected comment after > symbol after. > > I can also see this in the log files of the build bots, i.e. > > > https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... > > So presumable everyone will get it. Would you mind moving the comment or > fixing > the IDL parser to mute the build output? (It's the only line when building > "all" > for me.) > > https://codereview.chromium.org/2207573002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
