Figure out how to expose the main menubar (browser-menubar.inc) to an HTML document

RESOLVED FIXED

Status

()

enhancement
P3
normal
RESOLVED FIXED
10 months ago
8 months ago

People

(Reporter: bgrins, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 obsolete attachments)

(Reporter)

Description

10 months ago
We'd like to support top-level windows as HTML documents, so preprocessing in browser-menubar.inc isn't good for that.

There are a couple of general approaches we've thought of so far:
1) Parse out and append approximately the same markup, but do it from JS using a DOMParser (see MozXULElement.parseXULToFragment). This is an easier step, since we can even continue to use DTD files by including the entities in the string to be parsed).
2) Switch to a JS Menu API instead of using markup directly. Behind the scenes this would still generate XUL DOM (see for example https://searchfox.org/mozilla-central/rev/403038737ba75af3842ba6b43b6e2fb47eb06609/devtools/client/framework/menu.js#123). But it could also give a chance to refactor and improve how we do menus in the browser window (remove preprocessing, open up optimizations around not constructing the DOM or localizing until the menu items are going to be visible, etc).

Would like to do some more investigation to help figure out what the next steps are.

Updated

10 months ago
Priority: -- → P3
Comment hidden (mozreview-request)
(Reporter)

Comment 2

10 months ago
Pushed up a simpler alternative that would be less churn and would at least allow use from xhtml documents.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 9

9 months ago
The patches here actually mostly work, although various menu functionality will be broken (if the command relies on stuff that expects to be running in a XUL document such as various APIs on XULDocument, createElement assuming a XUL_NS, etc). The hidden window doesn't have very good test coverage to let us know what exactly is broken though, so I'm planning to prototype the main browser window as XHTML to figure out. This'll probably require a bunch of the work tracked in Bug 1453783.
Depends on: 1476333, 1476030
Summary: Figure out how to expose the main menubar (browser-menubar.inc) in a way that isn't XUL markup → Figure out how to expose the main menubar (browser-menubar.inc) to an HTML document
(Reporter)

Updated

9 months ago
Depends on: 1475342
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

9 months ago
Blocks: 1479125
(Reporter)

Updated

9 months ago
No longer blocks: 1479125
Depends on: 1479125
(Reporter)

Updated

9 months ago
Depends on: 1480465
(Reporter)

Updated

9 months ago
Depends on: 1481882

Comment 12

9 months ago
mozreview-review
Comment on attachment 8991142 [details]
Bug 1473165 - Migrate browser.xul to xhtml

https://reviewboard.mozilla.org/r/256110/#review269006

::: browser/base/content/browser.xhtml:34
(Diff revision 5)
>  # hiddenWindow.xul.
> -<!DOCTYPE window [
> +<!DOCTYPE html [
>  #include browser-doctype.inc
>  ]>
>  
> -<window id="main-window"
> +<html id="main-window"

Shouldn’t this be `html:html` as the default namespace is the XUL namespace, and last I checked, there’s no `html` element in XUL.

::: browser/base/content/browser.xhtml:75
(Diff revision 5)
>  # All JS files which are needed by browser.xul and other top level windows to
>  # support MacOS specific features *must* go into the global-scripts.inc file so
>  # that they can be shared with macWindow.inc.xul.
>  #include global-scripts.inc
>  
> -<script type="application/javascript">
> +<script type="text/javascript" xmlns="http://www.w3.org/1999/xhtml">

This should be `application/javascript`, as `text/javascript` is obsolete, according to [RFC 4329](https://tools.ietf.org/html/rfc4329#section-7).

::: browser/base/content/global-scripts.inc:10
(Diff revision 5)
>  
>  # If you update this list, you may need to add a mapping within the following
>  # file so that ESLint works correctly:
>  # tools/lint/eslint/eslint-plugin-mozilla/lib/environments/browser-window.js
>  
> -<script type="application/javascript">
> +<script type="text/javascript" xmlns="http://www.w3.org/1999/xhtml">

This should be `application/javascript`, as `text/javascript` is obsolete, according to [RFC 4329](https://tools.ietf.org/html/rfc4329#section-7).
(Reporter)

Comment 13

9 months ago
(In reply to ExE Boss from comment #12)
> Comment on attachment 8991142 [details]
> Bug 1473165 - Migrate browser.xul to xhtml
> 
> https://reviewboard.mozilla.org/r/256110/#review269006
> 
> ::: browser/base/content/browser.xhtml:34
> (Diff revision 5)
> >  # hiddenWindow.xul.
> > -<!DOCTYPE window [
> > +<!DOCTYPE html [
> >  #include browser-doctype.inc
> >  ]>
> >  
> > -<window id="main-window"
> > +<html id="main-window"
> 
> Shouldn’t this be `html:html` as the default namespace is the XUL namespace,
> and last I checked, there’s no `html` element in XUL.

I know it is odd, but by allowing the HTML element to be XUL namespaced, selectors like `:root` defined in XUL namespaced CSS files get applied to it. It may end up causing other issues later on, but it seems to work good enough at least for testing.
(In reply to Brian Grinstead [:bgrins] from comment #13)
> (In reply to ExE Boss from comment #12)
> > Comment on attachment 8991142 [details]
> > Bug 1473165 - Migrate browser.xul to xhtml
> > 
> > https://reviewboard.mozilla.org/r/256110/#review269006
> > 
> > ::: browser/base/content/browser.xhtml:34
> > (Diff revision 5)
> > >  # hiddenWindow.xul.
> > > -<!DOCTYPE window [
> > > +<!DOCTYPE html [
> > >  #include browser-doctype.inc
> > >  ]>
> > >  
> > > -<window id="main-window"
> > > +<html id="main-window"
> > 
> > Shouldn’t this be `html:html` as the default namespace is the XUL namespace,
> > and last I checked, there’s no `html` element in XUL.
> 
> I know it is odd, but by allowing the HTML element to be XUL namespaced,
> selectors like `:root` defined in XUL namespaced CSS files get applied to
> it. It may end up causing other issues later on, but it seems to work good
> enough at least for testing.

Sounds like this deserves a comment explaining this.
(Reporter)

Comment 15

9 months ago
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #14)
> > I know it is odd, but by allowing the HTML element to be XUL namespaced,
> > selectors like `:root` defined in XUL namespaced CSS files get applied to
> > it. It may end up causing other issues later on, but it seems to work good
> > enough at least for testing.
> 
> Sounds like this deserves a comment explaining this.

Yeah, for sure. FYI we're also looking into if it'll be possible to actually land browser.xhtml as more-or-less `#include browser.xul` with a define for the root tag name. That would make it easier for us to run tests against it and break down / fix  issues individually.
(Reporter)

Comment 16

8 months ago
(In reply to Brian Grinstead [:bgrins] from comment #15)
> FYI we're also looking into if it'll be possible to actually
> land browser.xhtml as more-or-less `#include browser.xul` with a define for
> the root tag name. That would make it easier for us to run tests against it
> and break down / fix  issues individually.

Alright, this can now be done with `mk_add_options 'export MOZ_BROWSER_XHTML=1'` in .mozconfig. I'm going to obsolete the patches here but keep this bug open as I know some of the menu functionality isn't working in that build yet.
Depends on: 1482256
(Reporter)

Updated

8 months ago
Attachment #8991142 - Attachment is obsolete: true
(Reporter)

Updated

8 months ago
Attachment #8991150 - Attachment is obsolete: true
(Reporter)

Comment 17

8 months ago
Going to close this since it's generally working, and handle any menubar breakage in individual bugs.
Status: NEW → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.