Closed Bug 1571461 Opened 1 year ago Closed 4 months ago

Text search relies on an inline event listener for accessibility

Categories

(Toolkit :: XUL Widgets, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla76
Tracking Status
firefox76 --- fixed

People

(Reporter: jkt, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

The following is used in XULMap.h:

if (aElement->HasAttr(kNameSpaceID_None, nsGkAtoms::onclick)) {

This means that anything with onclick="true" is shown in the a11y tree however instead we should be using click handlers as used elsewhere to be consistent.

The code that needs to hide a search icon from the accessibility tree should be using aria-hidden instead of showing it with onclick=true

Summary: Text search relies on an unline event handler for accessibility → Text search relies on an inline event listener for accessibility
Assignee: nobody → jkt

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a9f46b944150f56c1235a3f02e5d42ade0a51a3

Bug 1497189 will be exempting this code that is unsafe inline, with this patch we can remove that exemption.

Depends on: 1497189

It seems like we should be adding the correct role of graphic as well. Otherwise I am getting a failure here too: accessible/tests/mochitest/tree/test_image.xul

The priority flag is not set for this bug.
:aswan, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(andrew.swan)
Flags: needinfo?(andrew.swan) → needinfo?(surkov.alexander)

it'd be nice to have it fixed, it is a good improvement, but not that urgent as far as I understand, setting p3.

Jonathan, the patch seems looking good, is it ready for review?

Flags: needinfo?(surkov.alexander) → needinfo?(jkt)

Yeah flagged you, hopefully it hasn't bit rotted since.

Flags: needinfo?(jkt)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:jkt, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(jkt)
Priority: -- → P3

Christoph, this patch removes @onclick attribute on searchButtonIcon. So how extension.xul should be updated? https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.xul#24. Should be sha512 fragment associated with searchButtonIcon changed or removed?

Flags: needinfo?(ckerschb)

Sorry it took me forever to answer on this one. I am clearing out old ni?s at the moment. It seems extensions.xul is gone by now. I am rebasing the patch from :jkt and flagging you for review again. In other words, I'll drive the patch the :jkt wrote over the finishing line.

Assignee: jonathan → ckerschb
Status: NEW → ASSIGNED
Flags: needinfo?(jonathan)
Flags: needinfo?(ckerschb)
Attachment #9083027 - Attachment is obsolete: true
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/155281fd18fd
Change accessibility of search widget to look for all event handlers and remove hack to hide.r=surkov,MarcoZ

(In reply to Cristian Brindusan [:cbrindusan] from comment #12)

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=294552583&repo=autoland&lineNumber=3173

role/test_general.xhtml fails: apparently XUL image element [1] has a registered click listener as part of its implementation, i.e. any XUL image is now exposed as a toolbarbutton. I suppose the search image could have role='button', because I doubt that XULToolbarbutton has anything special that role='button' doesn't in this particular case, and then image toolbarmapping can be safely removed [2] (I suppose the search is unique consumer of this behaviour).

[1] https://searchfox.org/mozilla-central/source/accessible/tests/mochitest/role/test_general.xhtml#53
[2] https://searchfox.org/mozilla-central/source/accessible/base/XULMap.h#48-50

Attachment #9083027 - Attachment is obsolete: false

Alexander, thanks for your help, though I am not sure I did what you suggested (it's a little outside my comfort zone), can you take a look at:
https://phabricator.services.mozilla.com/D68023

FWIW, when running role/test_general.xhtml with the patch applied I get the following errors:

  • FAIL image without tooltiptext shouldn't be accessible.
  • FAIL Can't get accessible for 'image-onclick'
  • FAIL Wrong role for 'image-onclick' ! - got -1, expected 43
Flags: needinfo?(ckerschb) → needinfo?(surkov.alexander)

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14)

Alexander, thanks for your help, though I am not sure I did what you suggested (it's a little outside my comfort zone), can you take a look at:
https://phabricator.services.mozilla.com/D68023

I suggested to put role='button' on searchbutton in the search widget, in other words replace setAttribute("aria-hidden", "true") on setting role="none" and removeAttribute("aria-hidden") on role="button", then it's worth to check to the solution with MarcoZ (cc'ed).

FWIW, when running role/test_general.xhtml with the patch applied I get the following errors:

  • FAIL image without tooltiptext shouldn't be accessible.

do not set role="button" :) setting role attribute makes element accessible (except role='none') what makes the test fail, I suppose we can skip testing role='button' case, because it has to be already covered by the test suite.

  • FAIL Can't get accessible for 'image-onclick'
  • FAIL Wrong role for 'image-onclick' ! - got -1, expected 43

if the solution above works, then I think you can remove "image-onclick" testing at all

Flags: needinfo?(surkov.alexander)
Attachment #9083027 - Attachment is obsolete: true
Attachment #9135386 - Attachment description: Bug 1571461 - Change accessibility of search widget to look for all event handlers and remove hack to hide.r=surkov → Bug 1571461 - Change accessibility of search widget to look for all event handlers and remove hack to hide.r=surkov,marcoz

(In reply to alexander :surkov (:asurkov) from comment #15)

if the solution above works, then I think you can remove "image-onclick" testing at all

Yes, that seems to work. I removed the "image-onclick" testing and the test succeeds again - thank you!

(In reply to Tim Nguyen :ntim from comment #17)

FWIW, I think https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/aboutaddons.html#12 needs to be updated in this patch as well.

Thanks! I just updated that:
https://phabricator.services.mozilla.com/D68023

Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf8910303fdf
Change accessibility of search widget to look for all event handlers and remove hack to hide.r=surkov,MarcoZ
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla76
You need to log in before you can comment on or make changes to this bug.