Closed Bug 1465219 Opened 6 years ago Closed 6 years ago

Replace MenuBoxObject with XULElement subclass

Categories

(Core :: XUL, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: enndeakin, Assigned: enndeakin, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

      No description provided.
Mentor: enndeakin
Priority: -- → P5
Assignee: nobody → emalysz
Attached patch 8/3 - still some failures on try (obsolete) — Splinter Review
Attachment #8997605 - Attachment is obsolete: true
Assignee: emalysz → nobody
Flags: needinfo?(mconley)
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
Flags: needinfo?(mconley)
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.
Assignee: nobody → enndeakin
Attachment #9005152 - Attachment is obsolete: true
Attachment #9006263 - Attachment description: xelem-menu → Use XULMenuElement, a subclass of nsXULElement instead of MenuBoxObject for menu and menulist elements
Attachment #8999251 - Attachment is obsolete: true
Attachment #9006263 - Flags: review?(paolo.mozmail)
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.
Attachment #9006263 - Flags: review?(bzbarsky)
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
Attachment #9006263 - Flags: review?(paolo.mozmail) → review+
Attached patch Address commentsSplinter Review
Comment on attachment 9009165 [details] [diff] [review]
Address comments

Webidl and test_interfaces.js changes
Attachment #9009165 - Flags: review?(peterv)
>+      } 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.
Attachment #9006263 - Flags: review?(bzbarsky)
Attachment #9009165 - Flags: review?(peterv) → review?(bzbarsky)
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.
Attachment #9009165 - Flags: review?(bzbarsky) → review+
(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.
Blocks: 1492324
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
https://hg.mozilla.org/mozilla-central/rev/6dd5d0b6ba70
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
> 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.
Flags: needinfo?(enndeakin)
Blocks: 1494000
Flags: needinfo?(enndeakin)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: