Closed Bug 1372276 Opened 7 years ago Closed 2 years ago

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

Categories

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

task

Tracking

()

RESOLVED FIXED
103 Branch
Tracking Status
firefox103 --- fixed

People

(Reporter: d, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(3 files, 9 obsolete files)

4.17 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
48 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-github-pull-request
Details | Review
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
Anne, do you see other reasons that we'd consider to remain this feature in our codebase?
Flags: needinfo?(annevk)
Only if we use it ourselves somewhere, but I kinda doubt that.
Flags: needinfo?(annevk)
Andrew, this is probably a good starter project for someone.
Mentor: bzbarsky
Flags: needinfo?(overholt)
Summary: Remove HTML context menu support → Remove HTML context menu (<menu> and <menuitem> tag) support
Flags: needinfo?(overholt)
Priority: -- → P2
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?
Flags: needinfo?(felipc)
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!
Flags: needinfo?(felipc)
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.
FWIW: hsivonen or wchen should be able to help with regenerating the Java parser files when you get to that point.
Oh, does anyone know if PageMenu.jsm is related to this feature?

And what's this Java parser files that was mentioned?
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.)
Assignee: nobody → gphemsley
(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).
(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
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?
(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?
(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
(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...
> 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.
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.
Flags: needinfo?(wchen)
Flags: needinfo?(hsivonen)
Status: NEW → ASSIGNED
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.
Flags: needinfo?(wchen)
Flags: needinfo?(hsivonen)
Attachment #8891791 - Attachment is obsolete: true
Attachment #8891791 - Flags: review?(bzbarsky)
Attachment #8891792 - Attachment is obsolete: true
Attachment #8891792 - Flags: review?(william)
Attachment #8891793 - Attachment is obsolete: true
Attachment #8891793 - Flags: review?(william)
Attachment #8891794 - Attachment is obsolete: true
Attachment #8891794 - Flags: review?(bzbarsky)
Attachment #8891794 - Flags: review?(bugs)
Attachment #8891795 - Attachment is obsolete: true
Attachment #8891795 - Flags: review?(bzbarsky)
Attachment #8891797 - Attachment is obsolete: true
Attachment #8891796 - Attachment is obsolete: true
Attachment #8891797 - Flags: review?(bzbarsky)
Attachment #8891796 - Flags: review?(bzbarsky)
Attachment #8891798 - Attachment is obsolete: true
Attachment #8891798 - Flags: review?(snorp)
Attachment #8891798 - Flags: review?(bzbarsky)
Attachment #8891798 - Flags: review?(bugs)
Attachment #8891799 - Attachment is obsolete: true
Attachment #8891799 - Flags: review?(william)
Attachment #8891799 - Flags: review?(nfroyd)
Attachment #8891799 - Flags: review?(masayuki)
Attachment #8891799 - Flags: review?(dietrich)
Attachment #8891799 - Flags: review?(bzbarsky)
Attachment #8891791 - Attachment is obsolete: false
Attachment #8891791 - Flags: review?(bzbarsky)
Attachment #8891792 - Attachment is obsolete: false
Attachment #8891792 - Flags: review?(william)
Attachment #8891793 - Attachment is obsolete: false
Attachment #8891793 - Flags: review?(william)
Attachment #8891794 - Attachment is obsolete: false
Attachment #8891794 - Flags: review?(bzbarsky)
Attachment #8891794 - Flags: review?(bugs)
Attachment #8891795 - Attachment is obsolete: false
Attachment #8891795 - Flags: review?(bzbarsky)
Attachment #8891796 - Attachment is obsolete: false
Attachment #8891796 - Flags: review?(bzbarsky)
Attachment #8891797 - Attachment is obsolete: false
Attachment #8891797 - Flags: review?(bzbarsky)
Attachment #8891798 - Attachment is obsolete: false
Attachment #8891798 - Flags: review?(snorp)
Attachment #8891798 - Flags: review?(bzbarsky)
Attachment #8891798 - Flags: review?(bugs)
Attachment #8891799 - Attachment is obsolete: false
Attachment #8891799 - Flags: review?(william)
Attachment #8891799 - Flags: review?(nfroyd)
Attachment #8891799 - Flags: review?(masayuki)
Attachment #8891799 - Flags: review?(dietrich)
Attachment #8891799 - Flags: review?(bzbarsky)
(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!
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
(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?
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.
(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 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.
Attachment #8891799 - Flags: review?(masayuki) → review+
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 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.
Attachment #8891799 - Flags: review?(nfroyd) → review+
Comment on attachment 8891798 [details]
Bug 1372276 - Part 5: Remove HTML MenuBuilder;

https://reviewboard.mozilla.org/r/162830/#review168324
Attachment #8891798 - Flags: review?(snorp) → review+
(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. :)
(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
Attachment #8891792 - Flags: review?(william) → review?(hsivonen)
Attachment #8891793 - Flags: review?(william) → review?(hsivonen)
Attachment #8891799 - Flags: review?(william) → review?(hsivonen)
Comment on attachment 8891794 [details]
Bug 1372276 - Part 1: Remove contextMenu from HTMLElement;

https://reviewboard.mozilla.org/r/162822/#review168510
Attachment #8891794 - Flags: review?(bugs) → review+
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.
Attachment #8891798 - Flags: review?(bugs) → review+
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!)
(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.)
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...
(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. :)
(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 on attachment 8891791 [details]
Bug 1372276 - Part 0a: Add missing inclusions;

https://reviewboard.mozilla.org/r/162816/#review169910
Attachment #8891791 - Flags: review?(bzbarsky) → review+
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".
Attachment #8891794 - Flags: review?(bzbarsky) → review+
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.
Attachment #8891795 - Flags: review?(bzbarsky) → review+
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".
Attachment #8891796 - Flags: review?(bzbarsky) → review+
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
Flags: needinfo?(gphemsley)
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.
Attachment #8891797 - Flags: review?(bzbarsky) → review+
Comment on attachment 8891798 [details]
Bug 1372276 - Part 5: Remove HTML MenuBuilder;

https://reviewboard.mozilla.org/r/162830/#review169948
Attachment #8891798 - Flags: review?(bzbarsky) → review+
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".
Attachment #8891799 - Flags: review?(bzbarsky) → review+
(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
Flags: needinfo?(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?
Flags: needinfo?(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....).
Flags: needinfo?(bzbarsky)
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
Attachment #8891792 - Flags: review?(hsivonen) → review+
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.
Attachment #8891799 - Flags: review?(hsivonen) → review+
(Still expecting a mach clang-format update to part 0c.)
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.
Attachment #8891799 - Flags: review?(dietrich)
(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.
(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.
Flags: needinfo?(bzbarsky)
Well the obvious options are whoever wrote or reviewed this code, whoever that is.
Flags: needinfo?(bzbarsky)
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.
Attachment #8891793 - Flags: review?(hsivonen) → review-
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.
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.
See Also: → 1430235
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
Depends on: 1431512
Depends on: 1430235
Blocks bug 897102.
Flags: needinfo?(gphemsley)
Blocks: 897102
Flags: needinfo?(gphemsley)
Andrew, this could be a decent bug for someone new, maybe.
Flags: needinfo?(overholt)
Flags: needinfo?(overholt)
Flags: needinfo?(echen)
Comment 73 still applies.
See also this mailing list post from :bgrins

https://groups.google.com/d/msg/firefox-dev/mUlCUvgL6Do/jSZAlNYiCAAJ
(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.
Gordon is not actively working on this.
Assignee: gphemsley → nobody
Status: ASSIGNED → NEW
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.
Flags: needinfo?(bzbarsky)
Ruksar, this is probably not a great first project.  Also, have someone working on it as of a few days ago...
Assignee: nobody → agakhokidze
Flags: needinfo?(bzbarsky)
Okay, Thanks..
Flags: needinfo?(echen)
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.
What about bug 746087? <menu type="toolbar"> is still in the spec.
See Also: → 746087
(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
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.

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

Flags: needinfo?(bzbarsky)

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

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?

Flags: needinfo?(agakhokidze)

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....

Flags: needinfo?(bzbarsky)

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

Flags: needinfo?(agakhokidze)
Assignee: agakhokidze → nobody
Type: enhancement → task
Mentor: bzbarsky

Can I be assigned this bug?

Flags: needinfo?(htsai)

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

Flags: needinfo?(htsai)

(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!

Flags: needinfo?(sarentz)
Flags: needinfo?(bugs)

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

Flags: needinfo?(bugs)

Where can I clone the repo for this?

Flags: needinfo?(htsai)

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

Flags: needinfo?(htsai)

(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.

Flags: needinfo?(sarentz)
See Also: → 1679155

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.

(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.

Removing good-first-bug per comment #101.

Keywords: good-first-bug
Assignee: nobody → emilio
Attached patch Parser changes.Splinter Review

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.

Attachment #8891791 - Attachment is obsolete: true
Attachment #8891792 - Attachment is obsolete: true
Attachment #8891793 - Attachment is obsolete: true
Attachment #8891794 - Attachment is obsolete: true
Attachment #8891795 - Attachment is obsolete: true
Attachment #8891796 - Attachment is obsolete: true
Attachment #8891797 - Attachment is obsolete: true
Attachment #8891798 - Attachment is obsolete: true
Attachment #8891799 - Attachment is obsolete: true
Attachment #9282348 - Flags: review?(hsivonen)

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 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?
Attachment #9282348 - Flags: review?(hsivonen) → review+

Sure thing :)

Blocks: 1775477
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/065b130fb59f
Remove HTML menuitem. r=smaug,mconley,agi
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch

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!

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: