Bug 85541 – Chromium should include MenuListPopups' and MenuListOptions' within the ax tree.

• Component: New Bugs
• Status: RESOLVED FIXED

[David Tseng]

Chromium should include MenuListPopups' and MenuListOptions' within the ax tree.

[David Tseng]

Created attachment 140085 [details]
Patch

[chris fleizach]

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

[David Tseng]

(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.

[chris fleizach]

(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?

[David Tseng]

(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.

[chris fleizach]

(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.

[David Tseng]

Created attachment 140243 [details]
Patch

[WebKit Review Bot]

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.

[chris fleizach]

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

[David Tseng]

Created attachment 140253 [details]
Patch

[WebKit Review Bot]

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.

[David Tseng]

Created attachment 140254 [details]
Patch

[WebKit Review Bot]

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.

[chris fleizach]

Comment on attachment 140254 [details]
Patch

looks good, just fix the style issues

[David Tseng]

Created attachment 140258 [details]
Patch

[WebKit Review Bot]

Comment on attachment 140258 [details]
Patch

Rejecting attachment 140258 [details] from review queue.

[e-mail] 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.

[WebKit Review Bot]

Comment on attachment 140258 [details]
Patch

Rejecting attachment 140258 [details] from commit-queue.

[e-mail] 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.

[chris fleizach]

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

[WebKit Review Bot]

Comment on attachment 140258 [details]
Patch

Clearing flags on attachment: 140258

Committed r116125: <http://trac.webkit.org/changeset/116125>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.