Closed Bug 1422465 Opened 2 years ago Closed 2 years ago

Remove nsIDOMXULPopupElement interface and [implements] attribute on the "popup-base" and "panel" bindings

Categories

(Core :: DOM: Core & HTML, task)

task
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(2 files)

AFAICT this interface is used only in one place (to change an accessibilty role for a richlistbox with a panel parent). I haven't been able to confirm that code ever runs, but if so this case should be able to be changed to check for the tag name or some other information about the parent node.

If the interface is gone then we can remove the [implements] attribute on the bindings that use it, which should simplify replacing the XBL version of those bindings with Custom Elements.

- https://searchfox.org/mozilla-central/search?q=XULPopupElement&redirect=false
- https://searchfox.org/mozilla-central/rev/0b613c3887789f7786cd3131dfe9648398f4a6ac/accessible/xul/XULListboxAccessible.cpp#158-165
Alexander, can you help me figure out if the change to XULListboxAccessible is correct? Unfortunately this condition doesn't seem to be covered by automated tests, as the accessibility tests pass even with it commented out. And I'm not sure how to manually test this (or write a test to make sure it is working).  The comment indicates that this is for the URL bar but I'm not seeing the code being called when opening the URL bar - do I need to somehow enable accessibility features?
Flags: needinfo?(surkov.alexander)
XUL combobox tests are located here [1]. I'd be nice to add a new one there, if your case is not covered. However if we have test coverage for your case, then the reason, why the changes don't have any effect, could be [2].

[1] https://dxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/tree/test_combobox.xul?q=test_combobox.xul&redirect_type=direct
[2] https://dxr.mozilla.org/mozilla-central/source/accessible/generic/Accessible.cpp?q=ARIATransformRole&redirect_type=direct#1463
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov from comment #4)
> XUL combobox tests are located here [1]. I'd be nice to add a new one there,
> if your case is not covered. However if we have test coverage for your case,
> then the reason, why the changes don't have any effect, could be [2].

Huh, so does (2) indicate that this condition in XULListboxAccessible::NativeRole is dead code? I don't want to add a test for this unless if we can confirm this is actually used in the browser. If I add printf inside of that function it never gets run when I `./mach run` and open the URL bar - do I need to do something else to enable accessibility features?
Flags: needinfo?(surkov.alexander)
(In reply to Brian Grinstead [:bgrins] from comment #5)
> (In reply to alexander :surkov from comment #4)
> > XUL combobox tests are located here [1]. I'd be nice to add a new one there,
> > if your case is not covered. However if we have test coverage for your case,
> > then the reason, why the changes don't have any effect, could be [2].
> 
> Huh, so does (2) indicate that this condition in
> XULListboxAccessible::NativeRole is dead code? 

probably not, there's a mutlti column list case there, see roles::TREE in XULListboxAccessible::NativeRole() 

> I don't want to add a test
> for this unless if we can confirm this is actually used in the browser. If I
> add printf inside of that function it never gets run when I `./mach run` and
> open the URL bar - do I need to do something else to enable accessibility
> features?

yes, you could start a screen reader, for example, VoiceOver on mac (CMD+f5), or invoke it via the browser console, like:
Cc["@mozilla.org/accessibilityService;1"].getService(Ci.nsIAccessibilityService)
Flags: needinfo?(surkov.alexander)
(In reply to alexander :surkov from comment #6)
> > for this unless if we can confirm this is actually used in the browser. If I
> > add printf inside of that function it never gets run when I `./mach run` and
> > open the URL bar - do I need to do something else to enable accessibility
> > features?
> 
> yes, you could start a screen reader, for example, VoiceOver on mac
> (CMD+f5), or invoke it via the browser console, like:
> Cc["@mozilla.org/accessibilityService;1"].getService(Ci.
> nsIAccessibilityService)

OK, perfect - I can see the condition is getting run both with and without the patch once the service is started. I'll look into adding a test case in the combobox test.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Comment on attachment 8933865 [details]
Bug 1422465 - Remove the nsIDOMXULPopupElement interface and [implements] attribute on the "popup-base" and "panel" bindings

https://reviewboard.mozilla.org/r/204792/#review211278

This is going to break the code at https://searchfox.org/comm-central/source/suite/common/utilityOverlay.js#1354 because that does `instanceof Components.interfaces.nsIDOMXULPopupElement`.  Please file a comm-central bug so they'll know to update to some other method of detecting popups (e.g. copy what browser/base/content/utilityOverlay.js does with explicit namespace/tagname tests).

Nice to see the classinfo bits go away!
Attachment #8933865 - Flags: review?(bzbarsky) → review+
Comment on attachment 8934712 [details]
Bug 1422465 - Add regression test to ensure that the awesomebar richlistbox gets the COMBOBOX_LIST role;

https://reviewboard.mozilla.org/r/205608/#review211480

::: accessible/tests/browser/general/browser.ini:8
(Diff revision 1)
>    !/accessible/tests/browser/shared-head.js
>    head.js
>    !/accessible/tests/mochitest/*.js
>  
>  [browser_test_doc_creation.js]
> +[browser_test_popup_autocomplete.js]

browser_test_urlbar.js would be a better match to better reflect we test browser UI there, otherwise it makes sense to replicate urlbar widget in the test
Attachment #8934712 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8933865 [details]
Bug 1422465 - Remove the nsIDOMXULPopupElement interface and [implements] attribute on the "popup-base" and "panel" bindings

https://reviewboard.mozilla.org/r/204792/#review211482
Attachment #8933865 - Flags: review?(surkov.alexander) → review+
Depends on: 1423723
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1355b86006a4
Add regression test to ensure that the awesomebar richlistbox gets the COMBOBOX_LIST role;r=surkov
https://hg.mozilla.org/integration/autoland/rev/319fbe5fff76
Remove the nsIDOMXULPopupElement interface and [implements] attribute on the "popup-base" and "panel" bindings;r=bz,surkov
https://hg.mozilla.org/mozilla-central/rev/1355b86006a4
https://hg.mozilla.org/mozilla-central/rev/319fbe5fff76
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1426062
Blocks: 1461793
How would I fix the following code
/*
 * @param {nsIDOMXULPopupElement} target Current menupopup node
 */
function QpFolderMenu(target) {

	if (!(target instanceof CI.nsIDOMXULPopupElement)) {
		throw new Error('QpFolderMenu: Illegal argument "target" ' + target);
	}


this throws 
Error: TypeError: invalid 'instanceof' operand CI.nsIDOMXULPopupElement

is there an alternative interface?
There is not.  What semantics do you actually want?  That is, why do you care whether you have a nsIDOMXULPopupElement?
Component: DOM → DOM: Core & HTML
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.