WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
117834
[ATK] Adds support for aria-haspopup property.
https://bugs.webkit.org/show_bug.cgi?id=117834
Summary
[ATK] Adds support for aria-haspopup property.
Krzysztof Czech
Reported
2013-06-20 06:55:01 PDT
aria-haspopup indicates that the element has a popup context menu or sub-level menu.
Attachments
Patch
(9.17 KB, patch)
2013-07-02 04:05 PDT
,
Krzysztof Czech
no flags
Details
Formatted Diff
Diff
Patch
(19.64 KB, patch)
2013-07-03 01:11 PDT
,
Krzysztof Czech
no flags
Details
Formatted Diff
Diff
Patch
(15.53 KB, patch)
2013-07-03 01:23 PDT
,
Krzysztof Czech
no flags
Details
Formatted Diff
Diff
Patch
(15.42 KB, patch)
2013-07-03 02:09 PDT
,
Krzysztof Czech
no flags
Details
Formatted Diff
Diff
Patch
(15.50 KB, patch)
2013-07-09 16:18 PDT
,
Krzysztof Czech
no flags
Details
Formatted Diff
Diff
Patch
(15.60 KB, patch)
2013-07-12 02:34 PDT
,
Krzysztof Czech
buildbot
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
(558.64 KB, application/zip)
2013-07-12 04:42 PDT
,
Build Bot
no flags
Details
Patch
(15.60 KB, patch)
2013-07-12 04:44 PDT
,
Krzysztof Czech
no flags
Details
Formatted Diff
Diff
Patch
(15.60 KB, patch)
2013-07-16 01:54 PDT
,
Krzysztof Czech
no flags
Details
Formatted Diff
Diff
Patch
(15.60 KB, patch)
2013-07-16 02:03 PDT
,
Krzysztof Czech
no flags
Details
Formatted Diff
Diff
Patch
(15.60 KB, patch)
2013-07-16 02:13 PDT
,
Krzysztof Czech
no flags
Details
Formatted Diff
Diff
Show Obsolete
(9)
View All
Add attachment
proposed patch, testcase, etc.
Krzysztof Czech
Comment 1
2013-07-02 04:05:09 PDT
Created
attachment 205887
[details]
Patch
Mario Sanchez Prada
Comment 2
2013-07-02 05:37:32 PDT
Comment on
attachment 205887
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=205887&action=review
> Tools/ChangeLog:13 > + * WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp: > + (WTR::attributeSetValueById): > + (WTR::AccessibilityUIElement::stringAttributeValue): > + (WTR::AccessibilityUIElement::hasPopup):
I wonder if it would be possible to implement the same functionality in DRT's AccessibilityUIElementAtk.cpp, so we can pass the test as well for WebKit1
> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:1098 > + if (attributeValue.isNull()) > + return false; > + > + return attributeValue == "true" ? true : false;
I would probably write this way: if (attributeValue.isNull() || attributeValue == "false") return false; return true; ... but I'm fine with how it is now anyway. Your call.
Krzysztof Czech
Comment 3
2013-07-02 05:45:30 PDT
(In reply to
comment #2
)
> (From update of
attachment 205887
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=205887&action=review
> > > Tools/ChangeLog:13 > > + * WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp: > > + (WTR::attributeSetValueById): > > + (WTR::AccessibilityUIElement::stringAttributeValue): > > + (WTR::AccessibilityUIElement::hasPopup): > > I wonder if it would be possible to implement the same functionality in DRT's AccessibilityUIElementAtk.cpp, so we can pass the test as well for WebKit1
Ohh I forgot, you are right, I'll do it.
> > > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:1098 > > + if (attributeValue.isNull()) > > + return false; > > + > > + return attributeValue == "true" ? true : false; > > I would probably write this way: > > if (attributeValue.isNull() || attributeValue == "false") > return false; > > return true; > > ... but I'm fine with how it is now anyway. Your call.
Nice hint, sounds good to me as well, I'll change it.
Krzysztof Czech
Comment 4
2013-07-03 01:11:54 PDT
Created
attachment 205976
[details]
Patch
WebKit Commit Bot
Comment 5
2013-07-03 01:13:54 PDT
Attachment 205976
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
Krzysztof Czech
Comment 6
2013-07-03 01:23:19 PDT
Created
attachment 205978
[details]
Patch
Krzysztof Czech
Comment 7
2013-07-03 01:27:26 PDT
> I wonder if it would be possible to implement the same functionality in DRT's AccessibilityUIElementAtk.cpp, so we can pass the test as well for WebKit1
WK1 support, done.
>
> I would probably write this way: > > if (attributeValue.isNull() || attributeValue == "false") > return false; > > return true; > > ... but I'm fine with how it is now anyway. Your call.
How about just return return attributeValue == "true" ? true : false; I guess operator overloading for String will take care in case of nulls
Mario Sanchez Prada
Comment 8
2013-07-03 01:49:01 PDT
(In reply to
comment #7
)
> > I wonder if it would be possible to implement the same functionality in DRT's AccessibilityUIElementAtk.cpp, so we can pass the test as well for WebKit1 > > WK1 support, done.
Cool, thanks
> > I would probably write this way: > > > > if (attributeValue.isNull() || attributeValue == "false") > > return false; > > > > return true; > > > > ... but I'm fine with how it is now anyway. Your call. > How about just return return attributeValue == "true" ? true : false; > I guess operator overloading for String will take care in case of nulls
As I said, I'm fine with that too. Just sharing how I would probably do it, not necessarily meaning it's the best way to do it :). In any case, if you're not checking nulls explicitly anyway then you could reduce it even more to a single statement: return attributeSetValueById(atk_object_get_attributes(ATK_OBJECT(m_element.get())), "aria-haspopup") == "true";
Krzysztof Czech
Comment 9
2013-07-03 02:09:30 PDT
Created
attachment 205982
[details]
Patch
Krzysztof Czech
Comment 10
2013-07-03 02:11:26 PDT
> In any case, if you're not checking nulls explicitly anyway then you could reduce it even more to a single statement: > > return attributeSetValueById(atk_object_get_attributes(ATK_OBJECT(m_element.get())), "aria-haspopup") == "true";
This one look very nice, thanks.
Krzysztof Czech
Comment 11
2013-07-05 01:11:40 PDT
Hello Chris, I already have an informal review of this patch. Could look at this, thanks.
chris fleizach
Comment 12
2013-07-09 08:53:22 PDT
Comment on
attachment 205982
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=205982&action=review
> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:41 > +static void attributesClear(AtkAttributeSet* attributesSet)
this method should probably be named "clearAttributes()"
> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:319 > +static String attributeSetValueById(AtkAttributeSet* attributeSet, const String& id)
this method seems like it should be named attributeValueForId after reading through what it does
> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:326 > + attributeValue.set(g_strdup(attribute->value));
Do you need to copy this to a GOwnPtr, or can you just return directly from here with return String(g_strdup(attribute->value))
> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:1038 > + return attributeSetValueById(atk_object_get_attributes(ATK_OBJECT(m_element)), "aria-haspopup") == "true";
you probably want to do an equalIgnoringCase in case someone does aria-haspop=True
> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:83 > +static String attributeSetValueById(AtkAttributeSet* attributeSet, const String& id)
ditto
> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:1093 > + return attributeSetValueById(atk_object_get_attributes(ATK_OBJECT(m_element.get())), "aria-haspopup") == "true";
ditto
Krzysztof Czech
Comment 13
2013-07-09 16:18:12 PDT
Created
attachment 206358
[details]
Patch
Krzysztof Czech
Comment 14
2013-07-09 16:18:46 PDT
Rebased after
r152397
Krzysztof Czech
Comment 15
2013-07-09 16:23:34 PDT
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:319 > > +static String attributeSetValueById(AtkAttributeSet* attributeSet, const String& id) > > this method seems like it should be named attributeValueForId after reading through what it does
Yes, you are right, that sounds more accurate.
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:326 > > + attributeValue.set(g_strdup(attribute->value)); > > Do you need to copy this to a GOwnPtr, or can you just return directly from here > > with return String(g_strdup(attribute->value))
I cannot directly return, I must clear AtkAttributeSet first.
> you probably want to do an equalIgnoringCase in case someone does aria-haspop=True
Yes, you are right Thanks, applied all suggestions.
Chris Dumez
Comment 16
2013-07-11 08:10:32 PDT
Comment on
attachment 206358
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=206358&action=review
> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:59 > + for (AtkAttributeSet* attributes = attributeSet; attributes; attributes = attributes->next) {
attributes -> attribute ?
> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:62 > + attributeValue.set(g_strdup(atkAttribute->value));
The strdup would not be needed if you freed the attributeSet list *after* constructing the return String. attributeValue would then be a const char*.
> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:68 > + return String(attributeValue.get());
String::fromUTF8(attributeValue)? iirc the string string is UTF-8 encoded, right?
> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:64 > + attributeValue.set(g_strdup(atkAttribute->value));
Same comments as above.
Krzysztof Czech
Comment 17
2013-07-12 02:34:03 PDT
Created
attachment 206517
[details]
Patch
Krzysztof Czech
Comment 18
2013-07-12 02:36:23 PDT
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:59 > > + for (AtkAttributeSet* attributes = attributeSet; attributes; attributes = attributes->next) { > > attributes -> attribute ?
I named it attributes because, it describes range of ATK attributes.
> > > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:62 > > + attributeValue.set(g_strdup(atkAttribute->value)); > > The strdup would not be needed if you freed the attributeSet list *after* constructing the return String. attributeValue would then be a const char*. >
I agree, done.
> > Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:68 > > + return String(attributeValue.get()); > > String::fromUTF8(attributeValue)? iirc the string string is UTF-8 encoded, right? >
I guess you are right, corrected
> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:64 > > + attributeValue.set(g_strdup(atkAttribute->value)); > > Same comments as above.
Krzysztof Czech
Comment 19
2013-07-12 02:37:53 PDT
Thanks for review.
Build Bot
Comment 20
2013-07-12 04:42:35 PDT
Comment on
attachment 206517
[details]
Patch
Attachment 206517
[details]
did not pass mac-ews (mac): Output:
http://webkit-queues.appspot.com/results/1063080
New failing tests: fullscreen/full-screen-iframe-with-max-width-height.html
Build Bot
Comment 21
2013-07-12 04:42:37 PDT
Created
attachment 206525
[details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.3
Krzysztof Czech
Comment 22
2013-07-12 04:44:27 PDT
Created
attachment 206526
[details]
Patch
Chris Dumez
Comment 23
2013-07-15 04:52:58 PDT
Comment on
attachment 206526
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=206526&action=review
> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:54 > +static String atkAttributeSetValueForId(AtkObject* accessible, const String& id)
This could take a const char* in argument and return a CString. It does not look like we need a String at all (inside the function or the callers). Also, I find the name a bit confusing because it sounds like it 'sets' something. How about getAttributeSetValueForId() ?
> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:56 > +static String atkAttributeSetValueForId(AtkObject* accessible, const String& id)
Same comments as for DRT.
Krzysztof Czech
Comment 24
2013-07-16 01:54:50 PDT
Created
attachment 206741
[details]
Patch
Krzysztof Czech
Comment 25
2013-07-16 02:03:10 PDT
Created
attachment 206742
[details]
Patch
WebKit Commit Bot
Comment 26
2013-07-16 02:04:38 PDT
Attachment 206742
[details]
did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/accessibility/element-haspopup-expected.txt', u'LayoutTests/accessibility/element-haspopup.html', u'LayoutTests/accessibility/popup-button-title-expected.txt', u'LayoutTests/accessibility/popup-button-title.html', u'LayoutTests/platform/mac/accessibility/element-haspopup-expected.txt', u'LayoutTests/platform/mac/accessibility/element-haspopup.html', u'LayoutTests/platform/mac/accessibility/popup-button-title-expected.txt', u'LayoutTests/platform/mac/accessibility/popup-button-title.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp', u'Tools/ChangeLog', u'Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp', u'Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp']" exit_code: 1 Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:496: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Krzysztof Czech
Comment 27
2013-07-16 02:13:21 PDT
Created
attachment 206744
[details]
Patch
Krzysztof Czech
Comment 28
2013-07-16 02:16:58 PDT
> > This could take a const char* in argument and return a CString. It does not look like we need a String at all (inside the function or the callers).
There's one place when I need return String. It's an equalIgnoringCase function. It does not take two raw chars. I will have to convert one of those params to String.
> > Also, I find the name a bit confusing because it sounds like it 'sets' something. How about getAttributeSetValueForId() ?
Sounds good to me.
> > > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:56 > > +static String atkAttributeSetValueForId(AtkObject* accessible, const String& id) >
Done
Chris Dumez
Comment 29
2013-07-16 02:52:52 PDT
Comment on
attachment 206744
[details]
Patch Looks fine. r=me.
WebKit Commit Bot
Comment 30
2013-07-16 05:54:56 PDT
Comment on
attachment 206744
[details]
Patch Clearing flags on attachment: 206744 Committed
r152716
: <
http://trac.webkit.org/changeset/152716
>
WebKit Commit Bot
Comment 31
2013-07-16 05:55:02 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug