Closed Bug 1216478 Opened 9 years ago Closed 8 years ago

Items with tooltips inside items with a label should use their own tooltip as an accessible name, not the ancestor's label

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox44 --- affected
firefox47 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(1 file)

MozReview Request: Bug 1216478 - prefer tooltiptext on a XUL element over title attribute on a containing toolbaritem when determining accessible name, r?surkov
58 bytes, text/x-review-board-request
surkov
: review+
Details 
This affected (pre bug 1123760) items in the location bar, but probably happens in other places. It seems to me like items with tooltiptexts shouldn't use the label of the parent/ancestor, but their own tooltip. If I have, say, an input box with buttons inside them, labeling the buttons the same way as their parent inputbox doesn't really make sense.
No longer blocks: 1123760
Blocks: 1123760
can you give an example of markup so I can have better understanding of the problem?
The location bar markup looked like this:

<toolbaritem title="Foo">
  <!-- some levels of nesting -->
    <toolbarbutton tooltiptext="Bar"/>
</toolbaritem>


and somehow the buttons got called "foo" instead of "bar".

You can still see this on release or aurora if you object-navigate around the location bar - the go, reload, stop buttons as well as the dropdown arrow and all the notification icons at the start of the bar suffer from this issue.
Flags: needinfo?(surkov.alexander)
tooltip text is a last resort for accessible name, thus if toolbarbutton picks up its name from its parent then tooltip is rather ignored. What does toolbaritem stand for?
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov from comment #3)
> tooltip text is a last resort for accessible name, thus if toolbarbutton
> picks up its name from its parent then tooltip is rather ignored.

I understand that's how it works right now. I don't understand why, though. It doesn't make sense. Why is the parent's label more important than the tooltip of the element itself, especially on an interactive element like a button?

> What does
> toolbaritem stand for?

It's a XUL <toolbaritem>. It's essentially a container of things that are all kept together. Does that help?
Flags: needinfo?(surkov.alexander)
I don't know much about toolbaritem and toolbarbutton's semantics, thus I cannot say what makes sense in this case. If you think that toolbarbutton shouldn't pick up its name from containing toolbaritem then it's fine with me as you presumably have better expertise in XUL UI area.
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov from comment #5)
> I don't know much about toolbaritem and toolbarbutton's semantics, thus I
> cannot say what makes sense in this case. If you think that toolbarbutton
> shouldn't pick up its name from containing toolbaritem then it's fine with
> me as you presumably have better expertise in XUL UI area.

Hmm. Actually, thinking about add-ons, Dão, does that still sound right? Because I'm not so sure anymore. For an unlabeled toolbarbutton, I don't know that the tooltiptext is *always* more important. I think the problem in this case is that there are multiple buttons that are part of a toolbaritem, and so it's clear that the tooltiptext should take precedence. In the individual case:

<toolbaritem title="Yahoo fizzwidget">
  <toolbarbutton tooltiptext="35 celebetrity 'news' articles you have been ignoring"/>
</toolbaritem>

I guess the current behaviour makes more sense ?
Flags: needinfo?(dao)
(In reply to :Gijs Kruitbosch from comment #6)
> Because I'm not so sure anymore. For an unlabeled toolbarbutton, I don't
> know that the tooltiptext is *always* more important. I think the problem in
> this case is that there are multiple buttons that are part of a toolbaritem,
> and so it's clear that the tooltiptext should take precedence. In the
> individual case:
> 
> <toolbaritem title="Yahoo fizzwidget">
>   <toolbarbutton tooltiptext="35 celebetrity 'news' articles you have been
> ignoring"/>
> </toolbaritem>
> 
> I guess the current behaviour makes more sense ?

Yes, but why would you wrap a single toolbarbutton into a toolbaritem, if the latter is meant to be a container?
(In reply to Marco Zehe (:MarcoZ) from comment #7)
> (In reply to :Gijs Kruitbosch from comment #6)
> > Because I'm not so sure anymore. For an unlabeled toolbarbutton, I don't
> > know that the tooltiptext is *always* more important. I think the problem in
> > this case is that there are multiple buttons that are part of a toolbaritem,
> > and so it's clear that the tooltiptext should take precedence. In the
> > individual case:
> > 
> > <toolbaritem title="Yahoo fizzwidget">
> >   <toolbarbutton tooltiptext="35 celebetrity 'news' articles you have been
> > ignoring"/>
> > </toolbaritem>
> > 
> > I guess the current behaviour makes more sense ?
> 
> Yes, but why would you wrap a single toolbarbutton into a toolbaritem, if
> the latter is meant to be a container?

No idea, but it happens more often than you'd think:

https://mxr.mozilla.org/addons/source/62/chrome/content/wmlbrowser/navigatorOverlay.xul#11

https://mxr.mozilla.org/addons/source/246/chrome/content/foobar.xul#50

https://mxr.mozilla.org/addons/source/337/chrome/content/overlay.xul#23

https://mxr.mozilla.org/addons/source/356/switch.xul#21

https://mxr.mozilla.org/addons/source/492/chrome/content/googlebarlite.xul#82

(I haven't looked at how these particular ones are labeled, fwiw, but just the concept of having a single <toolbaritem> wrapping a <toolbarbutton> is not unheard of. I vaguely recall running into some when we did Australis' styling as well.)
(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to alexander :surkov from comment #5)
> > I don't know much about toolbaritem and toolbarbutton's semantics, thus I
> > cannot say what makes sense in this case. If you think that toolbarbutton
> > shouldn't pick up its name from containing toolbaritem then it's fine with
> > me as you presumably have better expertise in XUL UI area.
> 
> Hmm. Actually, thinking about add-ons, Dão, does that still sound right?
> Because I'm not so sure anymore. For an unlabeled toolbarbutton, I don't
> know that the tooltiptext is *always* more important. I think the problem in
> this case is that there are multiple buttons that are part of a toolbaritem,
> and so it's clear that the tooltiptext should take precedence. In the
> individual case:
> 
> <toolbaritem title="Yahoo fizzwidget">
>   <toolbarbutton tooltiptext="35 celebetrity 'news' articles you have been
> ignoring"/>
> </toolbaritem>
> 
> I guess the current behaviour makes more sense ?

Except when "35 celebetrity 'news' articles you have been ignoring" is actually the informative part you're interested in, which doesn't seem far-fetched. So I still think we should do this even considering this constructed example.
Flags: needinfo?(dao)
Chiming in from the side: I think we'll want a mochitest for this, too.
Comment on attachment 8709954 [details]
MozReview Request: Bug 1216478 - prefer tooltiptext on a XUL element over title attribute on a containing toolbaritem when determining accessible name, r?surkov

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31613/diff/1-2/
Gah, forgot an extra return.
Comment on attachment 8709954 [details]
MozReview Request: Bug 1216478 - prefer tooltiptext on a XUL element over title attribute on a containing toolbaritem when determining accessible name, r?surkov

https://reviewboard.mozilla.org/r/31613/#review28663

::: accessible/generic/Accessible.cpp:830
(Diff revision 2)
>          parent->GetAttr(kNameSpaceID_None, nsGkAtoms::title, aName)) {

I would rather grab toooltiptext here pretending it's a real name similar to https://mxr.mozilla.org/mozilla-central/source/accessible/generic/HyperTextAccessible.cpp#1875

::: accessible/html/HTMLFormControlAccessible.cpp:330
(Diff revision 2)
> -    XULElmName(mDoc, widgetElm, aName);
> +    ENameValueFlag xulNameFlag = XULElmName(mDoc, widgetElm, aName);

you can reuse nameFlag
Attachment #8709954 - Flags: review?(surkov.alexander)
https://reviewboard.mozilla.org/r/31613/#review28663

> I would rather grab toooltiptext here pretending it's a real name similar to https://mxr.mozilla.org/mozilla-central/source/accessible/generic/HyperTextAccessible.cpp#1875

I don't understand what you're suggesting. The tooltiptext is on the element itself, not on the parent. What would be the benefit of fetching it inside the loop (and is that what you mean)?

I'm also not following the analogy with HyperTextAccessible, because that only uses the title attribute on the abbr element if `aName` is still empty. If we overrode NativeName() in XULToolbarButtonAccessible, and called  AccessibleWrap::NativeName() first, it wouldn't be empty - it would return the text provided by the toolbaritem's title attribute (which is what it does now).

Can you clarify what you want me to do?
(In reply to :Gijs Kruitbosch from comment #15)
> https://reviewboard.mozilla.org/r/31613/#review28663
> 
> > I would rather grab toooltiptext here pretending it's a real name similar to https://mxr.mozilla.org/mozilla-central/source/accessible/generic/HyperTextAccessible.cpp#1875
> 
> I don't understand what you're suggesting. The tooltiptext is on the element
> itself, not on the parent. What would be the benefit of fetching it inside
> the loop (and is that what you mean)?

> I'm also not following the analogy with HyperTextAccessible, because that
> only uses the title attribute on the abbr element if `aName` is still empty.
> If we overrode NativeName() in XULToolbarButtonAccessible, and called 
> AccessibleWrap::NativeName() first, it wouldn't be empty - it would return
> the text provided by the toolbaritem's title attribute (which is what it
> does now).
> 
> Can you clarify what you want me to do?

tooltip is applied as a last resort in current algorithm, so if you do that early in Accessible::NativeName then some XUL accessibles won't pick up name from tooltip as they override NativeName(). I'm not sure if this is critical or not but that's a change, and I would avoid it.

In this case tooltip feels as a real name (like in the abbreviation case) because you prefer it over something else. There's a difference between real name and recovered name (from tooltip), which is that a recovered name isn't used in recursive name computation. Recursive name computation is probably not a case for XUL toolbaritems, so no real bugs, but it would keep code more consistent.

if that cycle computes name for XULToolbarButtonAccessible then can we move it there and skip it if tooltip is presented?
(In reply to alexander :surkov from comment #16)
> if that cycle computes name for XULToolbarButtonAccessible then can we move
> it there and skip it if tooltip is presented?

That would mean no longer using the title attribute for anything else inside a <toolbaritem>, right? Or having to copy it everywhere. That doesn't sound good, either...
Flags: needinfo?(surkov.alexander)
what else can be contained in toolbaritem that needs a name from it?
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov from comment #18)
> what else can be contained in toolbaritem that needs a name from it?

In principle, anything.
(In reply to :Gijs Kruitbosch from comment #19)
> (In reply to alexander :surkov from comment #18)
> > what else can be contained in toolbaritem that needs a name from it?
> 
> In principle, anything.

ok, so if you just put the tooltip check right into that loop then would it fix the bug (being not super elegant solution though)?
(In reply to alexander :surkov from comment #20)
> (In reply to :Gijs Kruitbosch from comment #19)
> > (In reply to alexander :surkov from comment #18)
> > > what else can be contained in toolbaritem that needs a name from it?
> > 
> > In principle, anything.
> 
> ok, so if you just put the tooltip check right into that loop then would it
> fix the bug (being not super elegant solution though)?

I mean, having the tooltip check before the loop as I did in the patch would work, too, right? That would avoid using the tooltiptext of parent elements, which I'm not sure would be a good idea.

It seems to me you're objecting to removing the tooltiptext from the general Accessible::Name code. That's fine, we can keep that, and the patch will still work if we look it up in two places, it just seemed neater to get rid of the second place...
Flags: needinfo?(surkov.alexander)
(In reply to :Gijs Kruitbosch from comment #21)
> (In reply to alexander :surkov from comment #20)
> > (In reply to :Gijs Kruitbosch from comment #19)
> > > (In reply to alexander :surkov from comment #18)
> > > > what else can be contained in toolbaritem that needs a name from it?
> > > 
> > > In principle, anything.
> > 
> > ok, so if you just put the tooltip check right into that loop then would it
> > fix the bug (being not super elegant solution though)?
> 
> I mean, having the tooltip check before the loop as I did in the patch would
> work, too, right? That would avoid using the tooltiptext of parent elements,
> which I'm not sure would be a good idea.
> 
> It seems to me you're objecting to removing the tooltiptext from the general
> Accessible::Name code. That's fine, we can keep that, and the patch will
> still work if we look it up in two places, it just seemed neater to get rid
> of the second place...

so you suggest to add a tooltip check into XULName, and make it to return eNameFromTooltip, correct? That's probably fine. However if that check was inside the loop then would it look worse? That'd be a smaller patch.
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to alexander :surkov from comment #22)
> (In reply to :Gijs Kruitbosch from comment #21)
> > (In reply to alexander :surkov from comment #20)
> > > (In reply to :Gijs Kruitbosch from comment #19)
> > > > (In reply to alexander :surkov from comment #18)
> > > > > what else can be contained in toolbaritem that needs a name from it?
> > > > 
> > > > In principle, anything.
> > > 
> > > ok, so if you just put the tooltip check right into that loop then would it
> > > fix the bug (being not super elegant solution though)?
> > 
> > I mean, having the tooltip check before the loop as I did in the patch would
> > work, too, right? That would avoid using the tooltiptext of parent elements,
> > which I'm not sure would be a good idea.
> > 
> > It seems to me you're objecting to removing the tooltiptext from the general
> > Accessible::Name code. That's fine, we can keep that, and the patch will
> > still work if we look it up in two places, it just seemed neater to get rid
> > of the second place...
> 
> so you suggest to add a tooltip check into XULName, and make it to return
> eNameFromTooltip, correct? That's probably fine. However if that check was
> inside the loop then would it look worse? That'd be a smaller patch.

I don't really understand this request. Inside which loop? The only place that whose tooltiptext we should consider is the element itself, not its parents. So it doesn't seem like there's a point to putting it in the loop, I'd just need to do extra checks that we're not on a parent of the element. Can you explain what you mean?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(surkov.alexander)
I meant something like that, which might be not optimal but going with name calculation algorithm.

Accessible::XULElmName

// Toolbaritem provides a name to its children iff the child doesn't have own name (including the one provided by @title).
while (parent) {
  if (parent->IsXULElement(nsGkAtoms::toolbaritem) &&
      parent->HasAttr(kNameSpaceID_None, nsGkAtoms::title, aName)) {
    // Before grabbing a name from parent's toolbartitem, check if the element has own @title. If so then
    // prefer it over parent's name.
      
    return;
  }
  parent = parent->GetParent();
}
Flags: needinfo?(surkov.alexander)
This keeps dropping off my radar, assigning so that stops happening.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment on attachment 8709954 [details]
MozReview Request: Bug 1216478 - prefer tooltiptext on a XUL element over title attribute on a containing toolbaritem when determining accessible name, r?surkov

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31613/diff/2-3/
Attachment #8709954 - Flags: review?(surkov.alexander)
Comment on attachment 8709954 [details]
MozReview Request: Bug 1216478 - prefer tooltiptext on a XUL element over title attribute on a containing toolbaritem when determining accessible name, r?surkov

https://reviewboard.mozilla.org/r/31613/#review31377

r=me
Attachment #8709954 - Flags: review?(surkov.alexander) → review+
https://hg.mozilla.org/mozilla-central/rev/90736e1afae9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Depends on: 1259024
No longer depends on: 1259024
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: