1372276 - Remove HTML context menu (<menu> and <menuitem> tag) support

Status: RESOLVED FIXED

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

Description

[Domenic Denicola]

In https://github.com/whatwg/html/pull/2742 we removed the context menu feature from HTML due to lack of implementer interest. Unfortunately Firefox was the one implementer who put in the work to implement this neat feature. But it looks like it does not have a path forward as being part of the interoperable web, and so it is now non-standard.

Tests are available at http://w3c-test.org/html/semantics/interactive-elements/contextmenu-historical.html

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Anne, do you see other reasons that we'd consider to remain this feature in our codebase?

Comment 2

[Anne (:annevk)]

Only if we use it ourselves somewhere, but I kinda doubt that.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Andrew, this is probably a good starter project for someone.

Comment 5

[Kohei Yoshino [:kohei]]

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/html-context-menu-support-will-be-removed/

Comment 6

[Mike Conley (:mconley) (:⚙️)]

I know that felipe's intern Perry is already hanging around in nsContextMenu.js for bug 1360406... perhaps we want to point him at this too while he's at it?

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Ah we're focusing on Quantum Flow or Photon Perf bugs on his internship, so I'll leave this one for some other contributor who would like to step in! But thanks for the pointer!

Comment 8

[Thomas Wisniewski]

Just FYI, I don't mind pitching in to write a patch for this, if we don't have other candidates in mind. I'd probably need some help figuring out how to regenerate the java parser files, though.

Comment 9

[Anne (:annevk)]

FWIW: hsivonen or wchen should be able to help with regenerating the Java parser files when you get to that point.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Oh, does anyone know if PageMenu.jsm is related to this feature?

And what's this Java parser files that was mentioned?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

http://searchfox.org/mozilla-central/source/parser/html/java
http://searchfox.org/mozilla-central/source/parser/html/javasrc

Comment 12

[Gordon P. Hemsley [:GPHemsley]]

Just for fun, I thought I'd tackle this.

I've got a set of patches that passes 10 of the 11 tests from comment 0, but I'm stuck on a build error about mozilla::dom::CustomElementRegistry in dom/html/nsHTMLContentSink.cpp when I try to remove the actual <menuitem> element.

Are there any known gotchas in that area? (I assume there's something about C++ that I'm missing.)

Comment 13

[Thomas Wisniewski]

(In reply to Gordon P. Hemsley [:GPHemsley] from comment #12)
> I'm stuck on a build error about mozilla::dom::CustomElementRegistry in
> dom/html/nsHTMLContentSink.cpp when I try to remove the actual <menuitem>
> element.

Depending on the error, it might be as simple as just #include'ing a file that is no longer being included (because the menu-related files that included it before are gone).

Comment 14

[Gordon P. Hemsley [:GPHemsley]]

(In reply to Thomas Wisniewski from comment #13)
> (In reply to Gordon P. Hemsley [:GPHemsley] from comment #12)
> > I'm stuck on a build error about mozilla::dom::CustomElementRegistry in
> > dom/html/nsHTMLContentSink.cpp when I try to remove the actual <menuitem>
> > element.
> 
> Depending on the error, it might be as simple as just #include'ing a file
> that is no longer being included (because the menu-related files that
> included it before are gone).

That would make sense, but I can't find anything that was included by MenuItem that isn't also included by Menu.

The errors I'm getting are here:
https://pastebin.mozilla.org/9027850

Comment 15

[Thomas Wisniewski]

Yeah, it looks like a file is missing an: #include "CustomElementRegistry.h"

I'm not sure if it would be best to include it in dom/html/nsGenericHTMLFrameElement.cpp, though. Maybe dom/html/nsHTMLContentSink.cpp?

Comment 16

[Gordon P. Hemsley [:GPHemsley]]

(In reply to Thomas Wisniewski from comment #15)
> Yeah, it looks like a file is missing an: #include "CustomElementRegistry.h"
> 
> I'm not sure if it would be best to include it in
> dom/html/nsGenericHTMLFrameElement.cpp, though. Maybe
> dom/html/nsHTMLContentSink.cpp?

That's odd. Why would it suddenly be cropping up now that I've removed files?

I don't see anywhere that it sticks out as being related, except maybe the bindings?

http://searchfox.org/mozilla-central/search?q=CustomElementRegistry&case=false&regexp=false&path=

Adding #include "CustomElementRegistry.h" to dom/html/nsHTMLContentSink.cpp fixed the one error, but the other error about nsAttrValueOrString is apparently unrelated. What else is missing? Some version of Element.h?

Comment 17

[Thomas Wisniewski]

(In reply to Gordon P. Hemsley [:GPHemsley] from comment #16)
> That's odd. Why would it suddenly be cropping up now that I've removed files?

Presumably it's just because those files were including it (or including something else which included it). Now they're gone, and something else has to do so for nsGenericHTMLFrameElement to compile.


>What else is missing? Some version of Element.h?

I'm guessing it just needs a similar: #include "nsAttrValueOrString.h" in nsHMTLContentSink.cpp

But since I'm not so familiar with this code myself, there might be more to it than my suggestions. It may be that it would be best to just include them in nsHTMLContentSink

Comment 18

[Gordon P. Hemsley [:GPHemsley]]

(In reply to Thomas Wisniewski from comment #17)
> (In reply to Gordon P. Hemsley [:GPHemsley] from comment #16)
> >What else is missing? Some version of Element.h?
> 
> I'm guessing it just needs a similar: #include "nsAttrValueOrString.h" in
> nsHMTLContentSink.cpp
> 
> But since I'm not so familiar with this code myself, there might be more to
> it than my suggestions. It may be that it would be best to just include them
> in nsHTMLContentSink

Adding #include "nsAttrValueOrString.h" to dom/html/nsGenericHTMLFrameElement.cpp did the trick, though I still have no idea why either of these were working before.

And with that, I have a build that passes all 11 tests from comment 0.

Now to figure out what the story is with the built-in tests...

Comment 19

[Boris Zbarsky [:bzbarsky]]

> That's odd. Why would it suddenly be cropping up now that I've removed files?

Because unified build boundaries changed as a result, most likely.

Comment 20

[Gordon P. Hemsley [:GPHemsley]]

Something appears to be wrong with the HTML parser wrt named character entities. I ran the build instructions in place per the readme from comment 11 to prep for my changes, and it looks like it did a bunch of stylistic things, but now a bunch of character entities are not being defined:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a608725e0c372677f845cb70eb1143a0f505096
https://hg.mozilla.org/try/rev/8151de38263de0bd36f9708b00cdf7be5317bdc0

I ran `make named_characters` again to be sure, but it doesn't appear to be generating any changes.

Comment 21

[Gordon P. Hemsley [:GPHemsley]]

I figured it out. The named-character-references.html file in m-c has not been kept up to date with spec changes, so I switched the Makefile to use the copy in the htmlparser repo instead.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb0a932a7bc2c86b5b32326d635194f452022b88
https://hg.mozilla.org/try/rev/0728e2b554ea12130568b7a7e6e2e6455f491bd3

So hopefully now I'm unblocked in evaluating my test results.

Comment 22

[Gordon P. Hemsley [:GPHemsley]]

Attachment: Bug 1372276 - Part 0a: Add missing inclusions;

Review commit: https://reviewboard.mozilla.org/r/162816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162816/

Comment 23

[Gordon P. Hemsley [:GPHemsley]]

Attachment: Bug 1372276 - Part 0b: Use named character references list built in to htmlparser;

Review commit: https://reviewboard.mozilla.org/r/162818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162818/

Comment 24

[Gordon P. Hemsley [:GPHemsley]]

Attachment: Bug 1372276 - Part 0c: Update HTML5 parser;

Review commit: https://reviewboard.mozilla.org/r/162820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162820/

Comment 25

[Gordon P. Hemsley [:GPHemsley]]

Attachment: Bug 1372276 - Part 1: Remove contextMenu from HTMLElement;

Review commit: https://reviewboard.mozilla.org/r/162822/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162822/

Comment 26

[Gordon P. Hemsley [:GPHemsley]]

Attachment: Bug 1372276 - Part 2: Remove onshow HTML Event;

Review commit: https://reviewboard.mozilla.org/r/162824/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162824/

Comment 27

[Gordon P. Hemsley [:GPHemsley]]

Attachment: Bug 1372276 - Part 3: Remove special styling for menu[type=context];

Review commit: https://reviewboard.mozilla.org/r/162826/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162826/

Comment 28

[Gordon P. Hemsley [:GPHemsley]]

Attachment: Bug 1372276 - Part 4: Remove type and label attributes from menu HTML element;

Review commit: https://reviewboard.mozilla.org/r/162828/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162828/

Comment 29

[Gordon P. Hemsley [:GPHemsley]]

Attachment: Bug 1372276 - Part 5: Remove HTML MenuBuilder;

Review commit: https://reviewboard.mozilla.org/r/162830/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162830/

Comment 30

[Gordon P. Hemsley [:GPHemsley]]

Attachment: Bug 1372276 - Part 6: Remove <menuitem> HTML element support;

Review commit: https://reviewboard.mozilla.org/r/162832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162832/

Comment 31

[Gordon P. Hemsley [:GPHemsley]]

Comment on attachment 8891797 [details]
Bug 1372276 - Part 4: Remove type and label attributes from menu HTML element;

Review commit: https://reviewboard.mozilla.org/r/162828/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162828/

Comment 32

[Gordon P. Hemsley [:GPHemsley]]

Comment on attachment 8891798 [details]
Bug 1372276 - Part 5: Remove HTML MenuBuilder;

Review commit: https://reviewboard.mozilla.org/r/162830/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162830/

Comment 33

[Gordon P. Hemsley [:GPHemsley]]

Comment on attachment 8891799 [details]
Bug 1372276 - Part 6: Remove <menuitem> HTML element support;

Review commit: https://reviewboard.mozilla.org/r/162832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162832/

Comment 34

[Gordon P. Hemsley [:GPHemsley]]

(Sorry for the bugspam. MozReview had some connection issues, and so barfed all over the bug. All review requests are active.)

This change touches a lot of places, so some parts have many reviewers.

I'm fully expecting to have changes to make, especially nits, so have at it!

Comment 35

[:Felipe Gomes (needinfo for replies!)]

Note: there's also front-end code to be removed: PageMenu.jsm, and some small snippets here and there, in browser.xul and nsContextMenu.js, and some others, according to the original landing: https://hg.mozilla.org/mozilla-central/rev/561821863607

Comment 36

[Gordon P. Hemsley [:GPHemsley]]

(In reply to :Felipe Gomes (needinfo me!) from comment #35)
> Note: there's also front-end code to be removed: PageMenu.jsm, and some
> small snippets here and there, in browser.xul and nsContextMenu.js, and some
> others, according to the original landing:
> https://hg.mozilla.org/mozilla-central/rev/561821863607

I deliberately did not touch the XUL version of <menu> and <menuitem>, as those are not (to my knowledge) governed by the HTML spec. Am I wrong?

Comment 37

[:Felipe Gomes (needinfo for replies!)]

No, what you're saying is not wrong. But this code pointed out is not related to the XUL menu implementation, it's the front-end code that supports the HTML menu and menuitem. Basically it's what adds the custom entries provided by the webpage to our own (XUL) menus.

Comment 38

[Gordon P. Hemsley [:GPHemsley]]

(In reply to :Felipe Gomes (needinfo me!) from comment #37)
> No, what you're saying is not wrong. But this code pointed out is not
> related to the XUL menu implementation, it's the front-end code that
> supports the HTML menu and menuitem. Basically it's what adds the custom
> entries provided by the webpage to our own (XUL) menus.

Bah, OK. Anything else I'm missing?

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8891799 [details]
Bug 1372276 - Part 6: Remove <menuitem> HTML element support;

https://reviewboard.mozilla.org/r/162832/#review168184

r=masayuki for the editor part.

Comment 40

[Henri Sivonen (:hsivonen)]

Comment on attachment 8891793 [details]
Bug 1372276 - Part 0c: Update HTML5 parser;

https://reviewboard.mozilla.org/r/162820/#review168274

::: parser/html/nsHtml5AttributeName.h:76
(Diff revision 1)
>      static nsIAtom** SVG_DIFFERENT(nsIAtom* name, nsIAtom* camel);
>      static nsIAtom** MATH_DIFFERENT(nsIAtom* name, nsIAtom* camel);
>      static nsIAtom** COLONIFIED_LOCAL(nsIAtom* name, nsIAtom* suffix);
>    public:
>      static nsIAtom** SAME_LOCAL(nsIAtom* name);
> -    inline static int32_t levelOrderBinarySearch(jArray<int32_t, int32_t> data,
> +    inline static int32_t levelOrderBinarySearch(jArray<int32_t,int32_t> data, int32_t key)

Please run `./mach clang-format` after running the HTML parser translator.

Comment 41

[Nathan Froyd [:froydnj]]

Comment on attachment 8891799 [details]
Bug 1372276 - Part 6: Remove <menuitem> HTML element support;

https://reviewboard.mozilla.org/r/162832/#review168302

r=me on the `xpcom/` bits.

Comment 42

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8891798 [details]
Bug 1372276 - Part 5: Remove HTML MenuBuilder;

https://reviewboard.mozilla.org/r/162830/#review168324

Comment 43

[Gordon P. Hemsley [:GPHemsley]]

(In reply to Henri Sivonen (:hsivonen) from comment #40)
> Comment on attachment 8891793 [details]
> Bug 1372276 - Part 0c: Update HTML5 parser;
> 
> https://reviewboard.mozilla.org/r/162820/#review168274
> 
> ::: parser/html/nsHtml5AttributeName.h:76
> (Diff revision 1)
> >      static nsIAtom** SVG_DIFFERENT(nsIAtom* name, nsIAtom* camel);
> >      static nsIAtom** MATH_DIFFERENT(nsIAtom* name, nsIAtom* camel);
> >      static nsIAtom** COLONIFIED_LOCAL(nsIAtom* name, nsIAtom* suffix);
> >    public:
> >      static nsIAtom** SAME_LOCAL(nsIAtom* name);
> > -    inline static int32_t levelOrderBinarySearch(jArray<int32_t, int32_t> data,
> > +    inline static int32_t levelOrderBinarySearch(jArray<int32_t,int32_t> data, int32_t key)
> 
> Please run `./mach clang-format` after running the HTML parser translator.

Ooh, that does make things a lot prettier! Lots fewer changes now, too. I'll push the new commit for review once some of the rest settles down.

Welcome back! I'll need your guidance with the htmlparser side of things, which is conspicuously missing from this patchset because I couldn't figure out how to get it run successfully. :)

Comment 44

[:Felipe Gomes (needinfo for replies!)]

(In reply to Gordon P. Hemsley [:GPHemsley] from comment #38)
> (In reply to :Felipe Gomes (needinfo me!) from comment #37)
> > No, what you're saying is not wrong. But this code pointed out is not
> > related to the XUL menu implementation, it's the front-end code that
> > supports the HTML menu and menuitem. Basically it's what adds the custom
> > entries provided by the webpage to our own (XUL) menus.
> 
> Bah, OK. Anything else I'm missing?

Not that I know of.. I think that's all for the front-end

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8891794 [details]
Bug 1372276 - Part 1: Remove contextMenu from HTMLElement;

https://reviewboard.mozilla.org/r/162822/#review168510

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8891798 [details]
Bug 1372276 - Part 5: Remove HTML MenuBuilder;

https://reviewboard.mozilla.org/r/162830/#review168512

::: dom/webidl/HTMLMenuElement.webidl:26
(Diff revision 1)
>  partial interface HTMLMenuElement {
>             [CEReactions, SetterThrows]
>             attribute boolean compact;
>  };
>  
>  // Mozilla specific stuff

Remove this partial, now empty interface.

Comment 47

[Dietrich Ayala (:dietrich)]

Hey Gordon! I'm probably not the right reviewer. Which bits did you want me to review? I can point to the right person.

(I would ni? you, but I got a msg from Bugzilla saying you're not currently accepting them!)

Comment 48

[Gordon P. Hemsley [:GPHemsley]]

(In reply to Dietrich Ayala (:dietrich) from comment #47)
> Hey Gordon! I'm probably not the right reviewer. Which bits did you want me
> to review? I can point to the right person.

I think I included you for the addon-sdk change, since MozReview was giving me a hard time with some others.

> (I would ni? you, but I got a msg from Bugzilla saying you're not currently
> accepting them!)

Oh, thanks for letting me know. I've turned that off now. (But anyway, I'm watching this bug.)

Comment 49

[Andrew Swan [:aswan]]

Assuming this isn't going to land before 57 (ie, it doesn't land in the next few hours and it doesn't get uplifted), there's no point getting the addon-sdk bits reviewed, that code will be removed from m-c soon.  Apologies for whatever energy you already had to put into getting the patch ready...

Comment 50

[Gordon P. Hemsley [:GPHemsley]]

(In reply to Andrew Swan [:aswan] from comment #49)
> Assuming this isn't going to land before 57 (ie, it doesn't land in the next
> few hours and it doesn't get uplifted), there's no point getting the
> addon-sdk bits reviewed, that code will be removed from m-c soon.  Apologies
> for whatever energy you already had to put into getting the patch ready...

It's just a configuration change, so don't sweat it. :)

Comment 51

[Gordon P. Hemsley [:GPHemsley]]

(In reply to Gordon P. Hemsley [:GPHemsley] from comment #50)
> (In reply to Andrew Swan [:aswan] from comment #49)
> > Assuming this isn't going to land before 57 (ie, it doesn't land in the next
> > few hours and it doesn't get uplifted), there's no point getting the
> > addon-sdk bits reviewed, that code will be removed from m-c soon.  Apologies
> > for whatever energy you already had to put into getting the patch ready...
> 
> It's just a configuration change, so don't sweat it. :)

Oh wait, that's not true. But nonetheless, it wasn't a big deal.

Comment 52

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8891791 [details]
Bug 1372276 - Part 0a: Add missing inclusions;

https://reviewboard.mozilla.org/r/162816/#review169910

Comment 53

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8891794 [details]
Bug 1372276 - Part 1: Remove contextMenu from HTMLElement;

https://reviewboard.mozilla.org/r/162822/#review169916

r+ modulo the questions below.

::: dom/browser-element/mochitest/browserElement_ContextmenuEvents.js
(Diff revision 1)
> -  });
> -}
> -
> -function checkInnerContextMenu() {
> -  sendContextMenuTo('#inner-link', function onContextMenu(detail) {
> -    is(detail.systemTargets.length, 1, 'Includes anchor data');

Is this test no longer relevant in the new world?  I'd think we would still test things to do with the normal browser context menu....

Someone familiar with this test should probably review these bits.

::: dom/browser-element/mochitest/browserElement_ContextmenuEvents.js
(Diff revision 1)
> -  }, /* ignorePreventDefault */ true);
> -}
> -
> -function checkImageContextMenu() {
> -  sendContextMenuTo('#menu3-trigger', function onContextMenu(detail) {
> -    var target = detail.systemTargets[0];

And this bit here; this is not checking the HTML contextmenu stuff, afaict...

::: testing/web-platform/meta/html/semantics/interactive-elements/contextmenu-historical.html.ini:9
(Diff revision 1)
>    [el.contextMenu must not be present]
> -    expected: FAIL
> +    expected: PASS

Just remove these two lines instead of adding "expected: PASS".

Comment 54

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8891795 [details]
Bug 1372276 - Part 2: Remove onshow HTML Event;

https://reviewboard.mozilla.org/r/162824/#review169920

::: testing/web-platform/meta/html/semantics/interactive-elements/contextmenu-historical.html.ini:6
(Diff revision 1)
>    [onshow must not be present on the GlobalEventHandlers locations]
> -    expected: FAIL
> +    expected: PASS

Again, no "expected: PASS".  Just remove the two lines.

Comment 55

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8891796 [details]
Bug 1372276 - Part 3: Remove special styling for menu[type=context];

https://reviewboard.mozilla.org/r/162826/#review169922

::: testing/web-platform/meta/html/semantics/interactive-elements/contextmenu-historical.html.ini:18
(Diff revision 1)
>    [The user-agent stylesheet must leave type="context" menus as block display like other menus]
> -    expected: FAIL
> +    expected: PASS

Again, no "expected: PASS".

Comment 56

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The view source page currently creates extra context menu items via <menu> and <menuitem> elements for things like "Go to line".  Does that stop working after this change?

STR:

1. Open view source (Tools -> Web Developer -> Page Source)
2. Right-click on the view source page
3. Look for items like "Go to line" in the context menu

http://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/toolkit/components/viewsource/content/viewSource-content.js#933-954

Comment 57

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8891797 [details]
Bug 1372276 - Part 4: Remove type and label attributes from menu HTML element;

https://reviewboard.mozilla.org/r/162828/#review169940

::: testing/web-platform/meta/html/semantics/interactive-elements/contextmenu-historical.html.ini:12
(Diff revision 1)
>    [menu.type must not exist or reflect the content attribute]
> -    expected: FAIL
> +    expected: PASS

You know the drill: no PASS.  Same for the next one.

Comment 58

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8891798 [details]
Bug 1372276 - Part 5: Remove HTML MenuBuilder;

https://reviewboard.mozilla.org/r/162830/#review169948

Comment 59

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8891799 [details]
Bug 1372276 - Part 6: Remove <menuitem> HTML element support;

https://reviewboard.mozilla.org/r/162832/#review169960

::: editor/libeditor/HTMLEditUtils.cpp:681
(Diff revision 1)
>    ELEM(listing, false, false, GROUP_NONE, GROUP_NONE),
>    ELEM(main, true, true, GROUP_BLOCK, GROUP_FLOW_ELEMENT),
>    ELEM(map, true, true, GROUP_SPECIAL, GROUP_BLOCK | GROUP_MAP_CONTENT),
>    ELEM(mark, true, true, GROUP_PHRASE, GROUP_INLINE_ELEMENT),
>    ELEM(marquee, false, false, GROUP_NONE, GROUP_NONE),
> -  ELEM(menu, true, true, GROUP_BLOCK, GROUP_LI | GROUP_FLOW_ELEMENT),
> +  ELEM(menu, true, true, GROUP_BLOCK, GROUP_LI),

Why the GROUP_FLOW_ELEMENT removal?  Something about this in the commit message would be a good idea.

::: testing/web-platform/meta/html/semantics/interactive-elements/contextmenu-historical.html.ini:3
(Diff revision 1)
>    [HTMLMenuItemElement must not be not present]
> -    expected: FAIL
> +    expected: PASS

No "expected: PASS".

Comment 60

[Gordon P. Hemsley [:GPHemsley]]

(In reply to Boris Zbarsky [:bz] from comment #53)
> Comment on attachment 8891794 [details]
> Bug 1372276 - Part 1: Remove contextMenu from HTMLElement;
> 
> https://reviewboard.mozilla.org/r/162822/#review169916
> 
> r+ modulo the questions below.
> 
> ::: dom/browser-element/mochitest/browserElement_ContextmenuEvents.js
> (Diff revision 1)
> > -  });
> > -}
> > -
> > -function checkInnerContextMenu() {
> > -  sendContextMenuTo('#inner-link', function onContextMenu(detail) {
> > -    is(detail.systemTargets.length, 1, 'Includes anchor data');
> 
> Is this test no longer relevant in the new world?  I'd think we would still
> test things to do with the normal browser context menu....
> 
> Someone familiar with this test should probably review these bits.
> 
> ::: dom/browser-element/mochitest/browserElement_ContextmenuEvents.js
> (Diff revision 1)
> > -  }, /* ignorePreventDefault */ true);
> > -}
> > -
> > -function checkImageContextMenu() {
> > -  sendContextMenuTo('#menu3-trigger', function onContextMenu(detail) {
> > -    var target = detail.systemTargets[0];
> 
> And this bit here; this is not checking the HTML contextmenu stuff, afaict...

detail.contextmenu is no longer available, but maybe I axed too much of the tests. I'll revisit. (Who would be better to review?)

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #56)
> The view source page currently creates extra context menu items via <menu>
> and <menuitem> elements for things like "Go to line".  Does that stop
> working after this change?
> 
> STR:
> 
> 1. Open view source (Tools -> Web Developer -> Page Source)
> 2. Right-click on the view source page
> 3. Look for items like "Go to line" in the context menu
> 
> http://searchfox.org/mozilla-central/rev/
> f0e4ae5f8c40ba742214e89aba3f554da0b89a33/toolkit/components/viewsource/
> content/viewSource-content.js#933-954

Hmm... It does indeed. I'll look into a way around that. It should probably be using XUL elements. (Maybe I can just change the namespace?) Pointers welcome.

(In reply to Boris Zbarsky [:bz] from comment #59)
> Comment on attachment 8891799 [details]
> Bug 1372276 - Part 6: Remove <menuitem> HTML element support;
> 
> https://reviewboard.mozilla.org/r/162832/#review169960
> 
> ::: editor/libeditor/HTMLEditUtils.cpp:681
> (Diff revision 1)
> >    ELEM(listing, false, false, GROUP_NONE, GROUP_NONE),
> >    ELEM(main, true, true, GROUP_BLOCK, GROUP_FLOW_ELEMENT),
> >    ELEM(map, true, true, GROUP_SPECIAL, GROUP_BLOCK | GROUP_MAP_CONTENT),
> >    ELEM(mark, true, true, GROUP_PHRASE, GROUP_INLINE_ELEMENT),
> >    ELEM(marquee, false, false, GROUP_NONE, GROUP_NONE),
> > -  ELEM(menu, true, true, GROUP_BLOCK, GROUP_LI | GROUP_FLOW_ELEMENT),
> > +  ELEM(menu, true, true, GROUP_BLOCK, GROUP_LI),
> 
> Why the GROUP_FLOW_ELEMENT removal?  Something about this in the commit
> message would be a good idea.

The removal of <menuitem> from the spec changed the content model of <menu>. It is now "Zero or more li and script-supporting elements." You can see that it was introduced with the original implementation: https://hg.mozilla.org/mozilla-central/rev/561821863607#l44.1

Comment 61

[Gordon P. Hemsley [:GPHemsley]]

(In reply to Boris Zbarsky [:bz] from comment #52)
> Comment on attachment 8891791 [details]
> Bug 1372276 - Part 0a: Add missing inclusions;
> 
> https://reviewboard.mozilla.org/r/162816/#review169910

No objection to placement?

Comment 62

[Boris Zbarsky [:bzbarsky]]

Now that you mention it, the CustomElementRegistry include should be "mozilla/dom/CustomElementRegistry.h" and placed with the other mozilla/dom bits (which should be together, alphabetically, and aren't....).

Comment 63

[Henri Sivonen (:hsivonen)]

Comment on attachment 8891792 [details]
Bug 1372276 - Part 0b: Use named character references list built in to htmlparser;

https://reviewboard.mozilla.org/r/162818/#review170170

Comment 64

[Henri Sivonen (:hsivonen)]

Comment on attachment 8891799 [details]
Bug 1372276 - Part 6: Remove <menuitem> HTML element support;

https://reviewboard.mozilla.org/r/162832/#review170174

r+ provided that the issues raised by bz are addressed.

Comment 65

[Henri Sivonen (:hsivonen)]

(Still expecting a mach clang-format update to part 0c.)

Comment 66

[Dietrich Ayala (:dietrich)]

Comment on attachment 8891799 [details]
Bug 1372276 - Part 6: Remove <menuitem> HTML element support;

Clearing review request. Per comment #49, the SDK code is going away.

Comment 67

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Gordon P. Hemsley [:GPHemsley] from comment #60)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #56)
> > The view source page currently creates extra context menu items via <menu>
> > and <menuitem> elements for things like "Go to line".  Does that stop
> > working after this change?
> > 
> > STR:
> > 
> > 1. Open view source (Tools -> Web Developer -> Page Source)
> > 2. Right-click on the view source page
> > 3. Look for items like "Go to line" in the context menu
> > 
> > http://searchfox.org/mozilla-central/rev/
> > f0e4ae5f8c40ba742214e89aba3f554da0b89a33/toolkit/components/viewsource/
> > content/viewSource-content.js#933-954
> 
> Hmm... It does indeed. I'll look into a way around that. It should probably
> be using XUL elements. (Maybe I can just change the namespace?) Pointers
> welcome.

Maybe we should recreate the items in nsContextMenu.js and detect when view source pages are open.  Normally I'd double check with :mconley, but he appears to be on PTO and not accepting needinfo right now.  Perhaps we can ask when he's back next week.

Comment 68

[Gordon P. Hemsley [:GPHemsley]]

(In reply to Boris Zbarsky [:bz] from comment #53)
> Comment on attachment 8891794 [details]
> Bug 1372276 - Part 1: Remove contextMenu from HTMLElement;
> 
> https://reviewboard.mozilla.org/r/162822/#review169916
> 
> r+ modulo the questions below.
> 
> ::: dom/browser-element/mochitest/browserElement_ContextmenuEvents.js
> (Diff revision 1)
> > -  });
> > -}
> > -
> > -function checkInnerContextMenu() {
> > -  sendContextMenuTo('#inner-link', function onContextMenu(detail) {
> > -    is(detail.systemTargets.length, 1, 'Includes anchor data');
> 
> Is this test no longer relevant in the new world?  I'd think we would still
> test things to do with the normal browser context menu....
> 
> Someone familiar with this test should probably review these bits.
> 
> ::: dom/browser-element/mochitest/browserElement_ContextmenuEvents.js
> (Diff revision 1)
> > -  }, /* ignorePreventDefault */ true);
> > -}
> > -
> > -function checkImageContextMenu() {
> > -  sendContextMenuTo('#menu3-trigger', function onContextMenu(detail) {
> > -    var target = detail.systemTargets[0];
> 
> And this bit here; this is not checking the HTML contextmenu stuff, afaict...

Digging into this a little deeper, removing the HTML contextmenu attribute as I do in part 1 doesn't seem to affect the results of this test, as it's actually operating on iframe.mozbrowsercontextmenu from the Browser API. (I expect removing <menuitem> in part 6 will have a different result, but I haven't yet checked.)

I was gonna wait until I had the changes sorted to ask again about who should review this, but I think I might need some insight before then.

Comment 69

[Boris Zbarsky [:bzbarsky]]

Well the obvious options are whoever wrote or reviewed this code, whoever that is.

Comment 70

[Henri Sivonen (:hsivonen)]

Comment on attachment 8891793 [details]
Bug 1372276 - Part 0c: Update HTML5 parser;

(In reply to Henri Sivonen (:hsivonen) from comment #65)
> (Still expecting a mach clang-format update to part 0c.)

Marking the patch r- to clarify that this bug is blocked on a clang-formatted update to the patch rather than review of the patch in its current form.

Comment 71

[Gordon P. Hemsley [:GPHemsley]]

I've made most of the changes requested. I just need to clean it all up and post to MozReview, which I'll get to eventually.

I haven't made any progress on restoring View Source functionality, though.

Comment 72

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Just a heads up, I'm working on removing XPCOM bindings for HTML elements, and finished the removal of this one in bug 1408169 before finding out about the code here. I'm gonna go ahead and land it, but the patches on this bug will now require a rebase. Since you're deleting most of the files I've changed, it shouldn't be too complicated.

Comment 73

[Jan Henning [:JanH]]

Note that <menu> is also used in a few places (about:downloads, about:addons, about:config) in Fennec [1], so that needs sorting out as well before this feature can be removed.


[1] https://dxr.mozilla.org/mozilla-central/search?q=path%3Amobile+%3Cmenu+-path%3Asrc%2Fmain%2Fres+-path%3A.java&redirect=false

Comment 74

[ExE Boss]

Blocks bug 897102.

Comment 75

[Boris Zbarsky [:bzbarsky]]

Andrew, this could be a decent bug for someone new, maybe.

Comment 77

[Jan Henning [:JanH]]

Comment 73 still applies.

Comment 78

[ExE Boss]

See also this mailing list post from :bgrins

https://groups.google.com/d/msg/firefox-dev/mUlCUvgL6Do/jSZAlNYiCAAJ

Comment 79

[Brian Grinstead [:bgrins]]

(In reply to ExE Boss from comment #78)
> See also this mailing list post from :bgrins
> 
> https://groups.google.com/d/msg/firefox-dev/mUlCUvgL6Do/jSZAlNYiCAAJ

For the chrome use-case we are adding support for <xul:menu> and associated elements to HTML documents (bug 1453783). So removing the HTML elements shouldn't affect this.

It's likely that for the Fennec use-cases in Comment 73 those instances could be ported from <menu> / <menuitem> to <xul:menu> / <xul:menuitem> once that lands.

Comment 80

[Boris Zbarsky [:bzbarsky]]

Gordon is not actively working on this.

Comment 81

[Ruksar]

Hi,

I would like to work on this project. I am new and this is would be my first project. Can you please clarify what exactly do I have to do? I would love to work on it.

Comment 82

[Boris Zbarsky [:bzbarsky]]

Ruksar, this is probably not a great first project.  Also, have someone working on it as of a few days ago...

Comment 83

[Ruksar]

Okay, Thanks..

Comment 84

[Boris Zbarsky [:bzbarsky]]

Anny, note that Gordon pushed his most-updated version of these patches (not merged to tip, so would need some merging) to try at https://hg.mozilla.org/try/pushloghtml?changeset=c09d14883219e2586ff33d5e0eb812233eb7489d

That push only has some of the patches; you want to drill back via the "parent" link from the first changeset in that push until you get to "Part 0a: Add missing inclusions", which has a big merge changeset as a parent.

Comment 85

[ExE Boss]

What about bug 746087? <menu type="toolbar"> is still in the spec.

Comment 86

[Gordon P. Hemsley [:GPHemsley]]

(In reply to ExE Boss from comment #85)
> What about bug 746087? <menu type="toolbar"> is still in the spec.

Neither the code I was working on nor the tests linked in comment 0 involve removing <menu> altogether.

There is no longer a type attribute on <menu>; toolbar is its only remaining meaning:

https://html.spec.whatwg.org/multipage/grouping-content.html#the-menu-element

Comment 87

[Stephan Sokolow]

Bear in mind that this feature is also used by the Greasemonkey 4 porting shim, gm4-polyfill.js, to implement GM_registerMenuCommand under Greasemonkey 4.

Comment 88

[Gordon P. Hemsley [:GPHemsley]]

I notice this hasn't gone anywhere since I pushed my WIP changes. Out of curiosity, what's the current status of this?

Comment 89

[Anny Gakhokidze [:annyG]]

I should probably remove myself from assigned. I was trying to rebase this and make it work with the latest mozilla-central at the time, but then I got the following problems, as you can see per IRC conversations here [1] and here [2] and I got stuck after that...

[1] https://mozilla.logbot.info/content/20180809#c15144848-c15146338
[2] https://mozilla.logbot.info/content/20180810#c15149827-c15149839

Comment 90

[Gordon P. Hemsley [:GPHemsley]]

So it looks like you're having trouble rebuilding the htmlparser?

Note that the pushlog mentioned in comment 84 builds on top of this pushlog:
https://hg.mozilla.org/try/pushloghtml?changeset=8a81ca583490

And that's the one that has the htmlparser changes. Does that help any?

Comment 91

[Boris Zbarsky [:bzbarsky]]

Anny, please let me know whether the above (and the fix for bug 1466449) helps at all and if not I can try to help out with the htmlparser bits if you want. If you're not planning to work on this, please let me know....

Comment 92

[Anny Gakhokidze [:annyG]]

I think for now I won't be able to work on this :( Sorry about that.

Comment 93

[Shailen Patel]

Can I be assigned this bug?

Comment 94

[Olli Pettay [:smaug][[e-mail]]]

Just upload patches and this should get assigned to you automatically.

Comment 95

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Olli Pettay [:smaug] from comment #94)

Just upload patches and this should get assigned to you automatically.

However, seeing comment 82, this wasn't suggested a great first project. Some underlying bugs were fixed after that, do you have a different thought than comment 82 and perhaps some tips for Shailen Patel to start, Olli?

According to the current plan for Fennec, can you confirm if this bug really depends on bug 1430235 and bug 1431512. Do we still need/want to do anything there, :st3fan?

Thank you!

Comment 96

[Olli Pettay [:smaug][[e-mail]]]

Looking at the existing patch might make this not very hard to to.
But yeah, some background in Gecko development would be good.

Comment 97

[Shailen Patel]

Where can I clone the repo for this?

Comment 98

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi, you can refer to the page for repo cloning instructions. Thank you. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code

Comment 99

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #95)

According to the current plan for Fennec, can you confirm if this bug really depends on bug 1430235 and bug 1431512. Do we still need/want to do anything there, :st3fan?

I would only invest in this enhancement if it makes sense for our mobile web platform in general, or for Fenix specifically. If this is a Fennec-only change then I would not bother - right now we only fix critical issues.

Comment 100

[Falguni Islam]

Hello, I am new to contributing to Bugzilla can you assigned me to this issue so that I can learn to fix these bugs and also suggest to me how can Is start working on this bug to fix it.

Comment 101

[Jonathan Kingston [:jkt] he/him]

(In reply to Falguni Islam from comment #100)

Hello, I am new to contributing to Bugzilla can you assigned me to this issue so that I can learn to fix these bugs and also suggest to me how can Is start working on this bug to fix it.

I recommend picking a smaller task from the https://bugzilla.mozilla.org/show_bug.cgi?id=1254929 ones. The attached patches will require a lot of changes I suspect and landing it requires a lot of dedication. https://bugzilla.mozilla.org/show_bug.cgi?id=1635018 or https://bugzilla.mozilla.org/show_bug.cgi?id=1336403 might be good examples before biting off something that touches the parser, dom, tests etc.

Comment 102

[Kagami Rosylight [:saschanaz] (they/them)]

Removing good-first-bug per comment #101.

Comment 103

[Emilio Cobos Álvarez (:emilio)]

Attachment: Parser changes.

These were only used for ENABLE_VOID_MENUITEM and having the HTMLMenuItemElement interface, both of which are going to be gone in the next patch.

Comment 104

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1372276 - Remove HTML menuitem. r=smaug,saschanaz

This removes HTMLMenuItemElement and all the code and tests preffed off
by dom.menuitem.enabled.

The HTML parser changes are the result of applying the previous patch.

Comment 105

[Henri Sivonen (:hsivonen)]

Comment on attachment 9282348 [details] [diff] [review]
Parser changes.

Review of attachment 9282348 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Could you take a moment to create a GitHub PR to https://github.com/validator/htmlparser/ with the commit message prefixed with "Mozilla " that is, "Mozilla bug 1372276" instead of "Bug 1372276", please?

Comment 106

[Emilio Cobos Álvarez (:emilio)]

Attachment: GitHub Pull Request for parser changes.

Sure thing :)

Comment 107

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/065b130fb59f
Remove HTML menuitem. r=smaug,mconley,agi

Comment 108

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/065b130fb59f

Comment 109

[Brian Thomas Smith [bsmth]]

I've added the dev-docs-complete keyword for this bug; the following GitHub issue is a parent task to track documentation changes related to this:

https://github.com/mdn/content/issues/19707

Thanks!