Replace MenuBoxObject with XULElement subclass

RESOLVED FIXED in Firefox 64

Status

()

P5
normal
RESOLVED FIXED
10 months ago
6 months ago

People

(Reporter: enndeakin, Assigned: enndeakin, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

10 months ago
Mentor: enndeakin
(Assignee)

Updated

10 months ago
Priority: -- → P5

Updated

9 months ago
Assignee: nobody → emalysz

Comment 1

8 months ago
Comment hidden (mozreview-request)

Updated

8 months ago
Attachment #8997605 - Attachment is obsolete: true

Updated

8 months ago
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)
(Assignee)

Comment 5

7 months ago
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)

Comment 6

7 months ago
Assignee: nobody → enndeakin
Attachment #9005152 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Attachment #9006263 - Attachment description: xelem-menu → Use XULMenuElement, a subclass of nsXULElement instead of MenuBoxObject for menu and menulist elements
(Assignee)

Updated

7 months ago
Attachment #8999251 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #9006263 - Flags: review?(paolo.mozmail)

Comment 8

6 months ago
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)
(Assignee)

Comment 9

6 months ago
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.

Comment 10

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

Comment 11

6 months ago
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+
(Assignee)

Comment 13

6 months ago
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?
(Assignee)

Comment 15

6 months ago
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+
(Assignee)

Comment 18

6 months ago
(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.
(Assignee)

Updated

6 months ago
Blocks: 1492324

Comment 19

6 months ago
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

Comment 20

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6dd5d0b6ba70
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox64: --- → fixed
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)
(Assignee)

Updated

6 months ago
Blocks: 1494000
(Assignee)

Updated

6 months ago
Flags: needinfo?(enndeakin)
You need to log in before you can comment on or make changes to this bug.