Closed Bug 1439778 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit :: XUL Widgets, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

Details

Attachments

(1 file)

+++ 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"]
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...
No longer blocks: 1437873
No longer blocks: 1439773
No longer blocks: 1439761
Attachment #8952576 - Flags: review?(paolo.mozmail)
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?
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?
Flags: needinfo?(yzenevich)
Attachment #8952576 - Flags: review?(enndeakin)
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.
Attachment #8952576 - Flags: review?(enndeakin) → review?(surkov.alexander)
Attachment #8952576 - Flags: review?(paolo.mozmail)
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
Attachment #8952576 - Flags: review?(surkov.alexander) → review+
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 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?
Attachment #8952576 - Flags: review+ → review?(surkov.alexander)
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?
Attachment #8952576 - Flags: review?(surkov.alexander) → review+
Pushed by timdream@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/724ea239987f
Move XBL accessibility role="xul:panel" into XULMap.h r=surkov
https://hg.mozilla.org/mozilla-central/rev/724ea239987f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Flags: needinfo?(yzenevich)
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.