Chromium should include MenuListPopups' and MenuListOptions' within the ax tree.
Created attachment 140085 [details] Patch
Comment on attachment 140085 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140085&action=review > Source/WebCore/accessibility/AccessibilityMockObject.h:42 > + virtual bool accessibilityIsIgnored() const { return accessibilityPlatformIncludesObject() == DefaultBehavior || accessibilityPlatformIncludesObject() == IgnoreObject; } i don't think mock objects should be ignored if the platform says default behavior > Source/WebCore/accessibility/chromium/AccessibilityObjectChromium.cpp:40 > + return IncludeObject; you can probably just remove this block all together. my guess is the default behavior will include these
(In reply to comment #2) > (From update of attachment 140085 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140085&action=review > > > Source/WebCore/accessibility/AccessibilityMockObject.h:42 > > + virtual bool accessibilityIsIgnored() const { return accessibilityPlatformIncludesObject() == DefaultBehavior || accessibilityPlatformIncludesObject() == IgnoreObject; } > > i don't think mock objects should be ignored if the platform says default behavior AccessibilityObject currently returns true for accessibilityIsIgnored. As the immediate subclass, AccessibilityMockObject defaults currently to this behavior. Would you prefer != IncludeObject or == IncludeObject? > > > Source/WebCore/accessibility/chromium/AccessibilityObjectChromium.cpp:40 > > + return IncludeObject; > > you can probably just remove this block all together. my guess is the default behavior will include these See above; currently, AccessibilityMockObject doesn't ask the platform for its preference on ignoring an object and defaults to ignore.
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 140085 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=140085&action=review > > > > > Source/WebCore/accessibility/AccessibilityMockObject.h:42 > > > + virtual bool accessibilityIsIgnored() const { return accessibilityPlatformIncludesObject() == DefaultBehavior || accessibilityPlatformIncludesObject() == IgnoreObject; } > > > > i don't think mock objects should be ignored if the platform says default behavior > > AccessibilityObject currently returns true for accessibilityIsIgnored. As the immediate subclass, AccessibilityMockObject defaults currently to this behavior. > Hmm. Yes, mock object is really meant to be subclassed. I don't think there should be any instances of just mock object. Same goes for AccessibilityObject as well. it seem like this logic could probably move into AccessibilityObject itself (i assume something similar is already in AXRenderObject... which could be removed). we could probably write this as accessibilityPlatformIncludes() != IncludeObject > Would you prefer != IncludeObject or == IncludeObject? > > > > > > Source/WebCore/accessibility/chromium/AccessibilityObjectChromium.cpp:40 > > > + return IncludeObject; > > > > you can probably just remove this block all together. my guess is the default behavior will include these > > See above; currently, AccessibilityMockObject doesn't ask the platform for its preference on ignoring an object and defaults to ignore. Are menu list elements MockObjects? if so, aren't they subclassed in some way?
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > (From update of attachment 140085 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=140085&action=review > > > > > > > Source/WebCore/accessibility/AccessibilityMockObject.h:42 > > > > + virtual bool accessibilityIsIgnored() const { return accessibilityPlatformIncludesObject() == DefaultBehavior || accessibilityPlatformIncludesObject() == IgnoreObject; } > > > > > > i don't think mock objects should be ignored if the platform says default behavior > > > > AccessibilityObject currently returns true for accessibilityIsIgnored. As the immediate subclass, AccessibilityMockObject defaults currently to this behavior. > > > > Hmm. Yes, mock object is really meant to be subclassed. I don't think there should be any instances of just mock object. Same goes for AccessibilityObject as well. > > it seem like this logic could probably move into AccessibilityObject itself (i assume something similar is already in AXRenderObject... which could be removed). > > we could probably write this as > > accessibilityPlatformIncludes() != IncludeObject > > > Would you prefer != IncludeObject or == IncludeObject? > > There's a bit more going on in AccessibilityRenderObject which would make bubbling up to AccessibilityObject difficult. > > > > > > > Source/WebCore/accessibility/chromium/AccessibilityObjectChromium.cpp:40 > > > > + return IncludeObject; > > > > > > you can probably just remove this block all together. my guess is the default behavior will include these > > > > See above; currently, AccessibilityMockObject doesn't ask the platform for its preference on ignoring an object and defaults to ignore. > > Are menu list elements MockObjects? if so, aren't they subclassed in some way? Yes, there are AccessibilityMenuPopupList's and AccessibilityMenuListOption's both of which are subclassing AccessibilityMockObject. Neither, however, overrides accessibilityIsIgnored so both default to ignored as well.
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (In reply to comment #2) > > > > (From update of attachment 140085 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=140085&action=review > > > > > > > > > Source/WebCore/accessibility/AccessibilityMockObject.h:42 > > > > > + virtual bool accessibilityIsIgnored() const { return accessibilityPlatformIncludesObject() == DefaultBehavior || accessibilityPlatformIncludesObject() == IgnoreObject; } > > > > > > > > i don't think mock objects should be ignored if the platform says default behavior > > > > > > AccessibilityObject currently returns true for accessibilityIsIgnored. As the immediate subclass, AccessibilityMockObject defaults currently to this behavior. > > > > > > > Hmm. Yes, mock object is really meant to be subclassed. I don't think there should be any instances of just mock object. Same goes for AccessibilityObject as well. > > > > it seem like this logic could probably move into AccessibilityObject itself (i assume something similar is already in AXRenderObject... which could be removed). > > > > we could probably write this as > > > > accessibilityPlatformIncludes() != IncludeObject > > > > > Would you prefer != IncludeObject or == IncludeObject? > > > > > There's a bit more going on in AccessibilityRenderObject which would make bubbling up to AccessibilityObject difficult. > > > > > > > > > > Source/WebCore/accessibility/chromium/AccessibilityObjectChromium.cpp:40 > > > > > + return IncludeObject; > > > > > > > > you can probably just remove this block all together. my guess is the default behavior will include these > > > > > > See above; currently, AccessibilityMockObject doesn't ask the platform for its preference on ignoring an object and defaults to ignore. > > > > Are menu list elements MockObjects? if so, aren't they subclassed in some way? > > Yes, there are AccessibilityMenuPopupList's and AccessibilityMenuListOption's both of which are subclassing AccessibilityMockObject. Neither, however, overrides accessibilityIsIgnored so both default to ignored as well. Maybe just those mock object subclasses should be modified in this case.
Created attachment 140243 [details] Patch
Attachment 140243 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 140243 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140243&action=review > Source/WebCore/accessibility/AccessibilityMenuListPopup.h:44 > + virtual bool accessibilityIsIgnored() const { return accessibilityPlatformIncludesObject() == DefaultBehavior || accessibilityPlatformIncludesObject() == IgnoreObject; } Since there's a bit of logic here i would move the function in the .cpp file instead of the header Also, I think accessibilityPlatformIncludesObject() != IncludeBehavior is a better way of writing since a) it only calls accessibilityPlatformIncludesObject() one time b) it's a little more clear than reference DefaultBehavior
Created attachment 140253 [details] Patch
Attachment 140253 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/accessibility/AccessibilityMenuListPopup.cpp:65: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/accessibility/AccessibilityMenuListOption.cpp:99: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 3 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 140254 [details] Patch
Attachment 140254 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 140254 [details] Patch looks good, just fix the style issues
Created attachment 140258 [details] Patch
Comment on attachment 140258 [details] Patch Rejecting attachment 140258 [details] from review queue. dtseng@google.com does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.
Comment on attachment 140258 [details] Patch Rejecting attachment 140258 [details] from commit-queue. dtseng@google.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
Comment on attachment 140258 [details] Patch you can't review your own patches yet. i assume you wanted CQ+ so i'm doing that for you
Comment on attachment 140258 [details] Patch Clearing flags on attachment: 140258 Committed r116125: <http://trac.webkit.org/changeset/116125>
All reviewed patches have been landed. Closing bug.