1439778 - Move XBL accessibility role="xul:panel" into XULMap.h

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

+++ This bug was initially created as a clone of Bug #1430777 +++

I am not sure if by checking type=autocomplete{-richlistbox} we can properly separate the former and the later. We'll see.

toolkit/content/widgets/autocomplete.xml#autocomplete-base-popup role="none"
  toolkit/content/widgets/autocomplete.xml#autocomplete-result-popup
    panel[type="autocomplete"]
    browser/components/search/content/search.xml#browser-search-autocomplete-result-popup
      #PopupSearchAutoComplete
  toolkit/content/widgets/autocomplete.xml#autocomplete-rich-result-popup
    panel[type="autocomplete-richlistbox"]
    browser/base/content/urlbarBindings.xml#urlbar-rich-result-popup
      #PopupAutoCompleteRichResult

toolkit/content/widgets/popup.xml#panel role="xul:panel"
  panel
  toolkit/content/widgets/popup.xml#arrowpanel
    panel[type="arrow"]
    toolkit/content/widgets/datetimepopup.xml#datetime-popup
      #DateTimePickerPanel[active="true"]

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I don't think `panel[type="autocomplete"]` matches anywhere in m-c?

https://dxr.mozilla.org/mozilla-central/search?q=panel+type%3D%22autocomplete&redirect=false

There is one in comm-central though

https://dxr.mozilla.org/comm-central/search?q=%3Cpanel+type%3D%22autocomplete&redirect=false

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

OK. They were removed by bug 1427363. Are we sure we did not break anything, specifically here?

https://hg.mozilla.org/mozilla-central/rev/86fa284eb0e0#l1.12

I will still try to redo that check in CPP though...

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1439778 - Move XBL accessibility role="xul:panel" into XULMap.h

Review commit: https://reviewboard.mozilla.org/r/221832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221832/

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34c60503ee31cab2a0460fd9eeef9e7f20ce01f1

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8952576 [details]
Bug 1439778 - Move XBL accessibility role="xul:panel" into XULMap.h

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221832/diff/1-2/

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8952576 [details]
Bug 1439778 - Move XBL accessibility role="xul:panel" into XULMap.h

https://reviewboard.mozilla.org/r/221830/#review228048

::: accessible/base/XULMap.h:81
(Diff revision 2)
>  XULMAP(
> +  panel,
> +  [](nsIContent* aContent, Accessible* aContext) -> Accessible* {
> +    if (aContent->IsElement() &&
> +          (aContent->AsElement()->AttrValueIs(kNameSpaceID_None, nsGkAtoms::type,
> +                                              NS_LITERAL_STRING("autocomplete-richlistbox"),

I am still not sure about comment 2. I've tried the accessibility inspector (bug 1151468) but I don't think I can see autocomplete list with or without my patch.

Is this expected? If so, do we really need this check here?

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=708b059ce4c6e11a3b3690270cee5a6275137a72

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Hi Yura, could you answer comment 6? Is the accessibility inspector supposed to see the autocomplete list? Do we have a test in place to confirm that? If so, do we expose that differently so the popup panel must be hidden from the accessibility tree? How do confirm the panel is indeed hidden?

Comment 9

[Neil Deakin]

Comment on attachment 8952576 [details]
Bug 1439778 - Move XBL accessibility role="xul:panel" into XULMap.h

I think an accessibility person should review the xulmap.h change.

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 8952576 [details]
Bug 1439778 - Move XBL accessibility role="xul:panel" into XULMap.h

https://reviewboard.mozilla.org/r/221832/#review230036

r=me with comments addressed

::: accessible/base/XULMap.h:85
(Diff revision 2)
> +          (aContent->AsElement()->AttrValueIs(kNameSpaceID_None, nsGkAtoms::type,
> +                                              NS_LITERAL_STRING("autocomplete-richlistbox"),
> +                                              eIgnoreCase) ||
> +           aContent->AsElement()->AttrValueIs(kNameSpaceID_None, nsGkAtoms::type,
> +                                              NS_LITERAL_STRING("autocomplete"),
> +                                              eIgnoreCase))) {

it'd be nicer to replace these on

static Element::AttrValuesArray sIgnoreTypeVals[] =
      { &nsGkAtoms::autocomplete_richlistbox, &nsGkAtoms::autocomplete, nullptr };

nsStaticAtom* const* const mValues;
if (!aContent->IsElement() || aContent->AsElement()->FindAttrValueIn(kNameSpaceID_None, nsGKAtoms:type, sIgnoreTypeVals, eIgnoreCase)) {
  return null_ptr;
}

::: accessible/xul/XULColorPickerAccessible.cpp:139
(Diff revision 2)
>  XULColorPickerAccessible::IsAcceptableChild(nsIContent* aEl) const
>  {
> -  nsAutoString role;
> +  return aEl->IsXULElement(nsGkAtoms::panel) &&
> -  nsCoreUtils::XBLBindingRole(aEl, role);
> -  return role.EqualsLiteral("xul:panel") &&
>      aEl->IsElement() &&

aEl->IsElement() is not needed, since it has to be true because of the condition above

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8952576 [details]
Bug 1439778 - Move XBL accessibility role="xul:panel" into XULMap.h

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221832/diff/2-3/

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8952576 [details]
Bug 1439778 - Move XBL accessibility role="xul:panel" into XULMap.h

https://reviewboard.mozilla.org/r/221830/#review230412

::: accessible/base/nsAccessibilityService.cpp:326
(Diff revision 3)
>  
>  #undef MARKUPMAP
>  
>  #ifdef MOZ_XUL
> -#define XULMAP(atom, new_func) \
> -  { &nsGkAtoms::atom, new_func },
> +#define XULMAP(atom, ...) \
> +  { &nsGkAtoms::atom, __VA_ARGS__ },

With a initializer list I am getting `too many arguments provided to function-like macro invocation` error, hence the change.

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8952576 [details]
Bug 1439778 - Move XBL accessibility role="xul:panel" into XULMap.h

https://reviewboard.mozilla.org/r/221832/#review230036

> it'd be nicer to replace these on
> 
> static Element::AttrValuesArray sIgnoreTypeVals[] =
>       { &nsGkAtoms::autocomplete_richlistbox, &nsGkAtoms::autocomplete, nullptr };
> 
> nsStaticAtom* const* const mValues;
> if (!aContent->IsElement() || aContent->AsElement()->FindAttrValueIn(kNameSpaceID_None, nsGKAtoms:type, sIgnoreTypeVals, eIgnoreCase)) {
>   return null_ptr;
> }

What you have given didn't equal to the check I would need to do here (see comment 2). I did change the code to use Element::FindAttrValueIn(). Would you mind take a look at it again?

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 8952576 [details]
Bug 1439778 - Move XBL accessibility role="xul:panel" into XULMap.h

https://reviewboard.mozilla.org/r/221832/#review230690

::: accessible/base/XULMap.h:98
(Diff revision 3)
> +      { &nsGkAtoms::autocomplete_richlistbox, &nsGkAtoms::autocomplete, nullptr };
> +
> +    if (aContent->IsElement() &&
> +        aContent->AsElement()->FindAttrValueIn(kNameSpaceID_None, nsGkAtoms::type,
> +                                               sIgnoreTypeVals, eIgnoreCase) >= 0) {
> +      return nullptr;

I prefer to have
!aContent->IsElement() ||
aContent->AsElement()->FindAttValueIs()) {
  return null;
}

this will allow you to get rid of IsElement() check from the second 'if' statement

::: accessible/base/nsAccessibilityService.cpp:326
(Diff revision 3)
>  
>  #undef MARKUPMAP
>  
>  #ifdef MOZ_XUL
> -#define XULMAP(atom, new_func) \
> -  { &nsGkAtoms::atom, new_func },
> +#define XULMAP(atom, ...) \
> +  { &nsGkAtoms::atom, __VA_ARGS__ },

seems unrelated change?

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8952576 [details]
Bug 1439778 - Move XBL accessibility role="xul:panel" into XULMap.h

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221832/diff/3-4/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/724ea239987f
Move XBL accessibility role="xul:panel" into XULMap.h r=surkov

Comment 17

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/724ea239987f