Remaining failures can be found at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3c2259f55acec35669120d64f743294b49b2c2b

Hey Neil, mcote et al are still working on the redirect service for these old MozReview bugs to send you to the repository endpoint. In the meantime, he did the look-up manually - here's the latest revision for this bug: https://hg.mozilla.org/mozreview/gecko/rev/684b5e916803c760afee537740e1847ee168e436

This patch fixes up the tests. Changes made: 1. The 'open' property had been removed and this patch restores it. 2. Flush needed before opening popup as some tests need this 3. UITour test needed changes to not use the box object. 4. Change accessibility tests to not use getClassName 5. Some places were just comparing if an element was a XULElement but should have been checking if it was a menu specifically. Added an isMenu check for this.

Comment on attachment 9006263 [details] [diff] [review] Use XULMenuElement, a subclass of nsXULElement instead of MenuBoxObject for menu and menulist elements Do we need the new entries in the XULElement interface? Maybe we can use something like "instanceof XULMenuElement" instead of the isMenu() helper, or check if one of the menu-specific functions exist on the element? I'd also be interested in what Boris thinks, and we need a DOM peer review for the interfaces anyways.

The check needs to handle menu, menulist and menu-type buttons (which do not implement XULMenuElement). I think it is better to have a single function that checks the right thing than have callers potentially make different or incorrect checks.

Ah yes, that looks like the simplest thing that would support buttons of the menu type as well.

Comment on attachment 9006263 [details] [diff] [review] Use XULMenuElement, a subclass of nsXULElement instead of MenuBoxObject for menu and menulist elements Review of attachment 9006263 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the JavaScript portion, with a comment on the interface. ::: accessible/tests/mochitest/events.js @@ +1105,5 @@ > var x = 1, y = 1; > if (aArgs && ("where" in aArgs) && aArgs.where == "right") { > if (isHTMLElement(targetNode)) > x = targetNode.offsetWidth - 1; > + else if (isXULElement(targetNode)) Might be worth fixing the indentation while here - and add braces as well. ::: dom/webidl/XULElement.webidl @@ +87,5 @@ > readonly attribute CSSStyleDeclaration style; > + > + // Returns true if this is a menu-type element > + // (Has a menu frame associated with it) > + boolean isMenu(); The name isMenu looks like a class instance check helper, but it's not, since it may be true even if this is not a XULMenuElement. Maybe one of canOpenMenu, hasMenuFrame, hasMenu? ::: dom/xul/XULMenuElement.h @@ +24,2 @@ > > + // void OpenMenu(bool aOpenFlag); nit: just remove the line ::: toolkit/content/tests/widgets/popup_shared.js @@ +252,5 @@ > function openMenu(menu) { > if ("open" in menu) { > menu.open = true; > + } else if (menu.isMenu()) { > + menu.openMenu(true); nit: indentation @@ +262,5 @@ > function closeMenu(menu, popup) { > if ("open" in menu) { > menu.open = false; > + } else if (menu.isMenu()) { > + menu.openMenu(false); nit: indentation ::: toolkit/content/widgets/button.xml @@ +24,5 @@ > onset="this.setAttribute('group', val); return val;"/> > > <property name="open" onget="return this.hasAttribute('open');"> > <setter><![CDATA[ > + if (this.isMenu()) { nit: indentation

Comment on attachment 9009165 [details] [diff] [review] Address comments Webidl and test_interfaces.js changes

>+ } else if (isXULElement(targetNode)) Missing opening curly at the end of this line?

Yes, I will fix that.

Comment on attachment 9006263 [details] [diff] [review] Use XULMenuElement, a subclass of nsXULElement instead of MenuBoxObject for menu and menulist elements Looking at the other patch now.

Comment on attachment 9009165 [details] [diff] [review] Address comments >Bug 1465219, use XULMenuElement, a subclass of nsXULElement instead of MenuBoxObject for menu and menulist elements. Please add a comma after "nsXULElement". >+++ b/accessible/tests/mochitest/events.js >+function isXULElement(aNode) { Hmm. These checks were broken ever since XULFrameElement and XULScrollElement got added, eh? >@@ -1087,27 +1092,28 @@ function synthClick(aNodeOrID, aCheckerO >+ } else if (isXULElement(targetNode)) Needs '{' at end. >+++ b/dom/bindings/BindingUtils.cpp >+ } else if (definition->mLocalName == nsGkAtoms::menu || >+ definition->mLocalName == nsGkAtoms::menulist) { >+ cb = XULMenuElement_Binding::GetConstructorObject; We didn't use to use a menuboxobject for <menulist>, right? Why are we adding it here? >+++ b/dom/xul/XULMenuElement.cpp >+void XULMenuElement::SetActiveChild(Element* arg) Return type on separate line. Same for the other methods here. >+ nsMenuFrame* menu = do_QueryFrame(GetPrimaryFrame()); We used to flush frames but now won't. Why the change? >+bool XULMenuElement::HandleKeyPress(KeyboardEvent& keyEvent) >+ nsMenuFrame* menu = do_QueryFrame(GetPrimaryFrame()); Again, we used to flush frames. >+bool XULMenuElement::OpenedWithKey() >+ nsMenuFrame* menuframe = do_QueryFrame(GetPrimaryFrame()); And here. Please document in the commit message why these changes are OK, assuming they're OK. >+++ b/dom/xul/XULMenuElement.h >+ explicit XULMenuElement(already_AddRefed<mozilla::dom::NodeInfo>& aNodeInfo): nsXULElement(aNodeInfo) Line break before ": nsXULElement(aNodeInfo)", please. >+++ b/dom/xul/nsXULElement.cpp >+ nsMenuFrame* menu = do_QueryFrame(GetPrimaryFrame()); Should this flush frames? >+nsXULElement::OpenMenu(bool aOpenFlag) >+ nsCOMPtr<nsIContent> kungFuDeathGrip = this; // keep a reference Bindings should guarantee this if you are called only via those... Maybe annotate this function MOZ_CAN_RUN_SCRIPT and then static analysis will enforce that callers are holding strong refs to you? Might be a good idea to add that to anything that flushes frames if we add more flushes per above comments. r=me with the above addressed.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #17) > Hmm. These checks were broken ever since XULFrameElement and > XULScrollElement got added, eh? Yes, although the check is never done on an element where the difference matters, so it shouldn't currently fail. > We didn't use to use a menuboxobject for <menulist>, right? Why are we > adding it here? menulist is a menu via display="xul:menu" > >+ nsMenuFrame* menu = do_QueryFrame(GetPrimaryFrame()); > > We used to flush frames but now won't. Why the change? I added back the flushes to avoid chnaging this, although I don't think they are needed -- these methods are mostly called only during event handlers where the frames are already up to date. Only 'OpenedWithKey' would be called from outside the menu/menulist implementation itself.

Pushed by neil@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6dd5d0b6ba70 use XULMenuElement, a subclass of nsXULElement, instead of MenuBoxObject for menu and menulist elements, r=paolo,bz

> I added back the flushes to avoid chnaging this Why not use Element::GetPrimaryFrame instead of reimplementing it? At the very least that needs documentation. If it's because of the kungFuDeathGrip, can we replace that with MOZ_CAN_RUN_SCRIPT annotations instead? Followup please.