Closed Bug 1100442 Opened 5 years ago Closed 4 years ago

support "chrome" manifest property for desktop open web apps

Categories

(Firefox Graveyard :: Webapp Runtime, enhancement, P1)

30 Branch
enhancement

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: bwalker, Assigned: nick)

References

Details

(Whiteboard: DesktopWebRT2 [dependency: marketplace-partners])

Attachments

(7 files, 12 obsolete files)

948.03 KB, application/pdf
Details
157.82 KB, image/png
Details
173.51 KB, image/png
Details
93.36 KB, image/png
Details
119.47 KB, image/png
Details
v8
14.76 KB, patch
Details | Diff | Splinter Review
15.70 KB, image/png
Details
we should support "chrome" manifest property for desktop open web apps, our BD team suggests a number of important partner apps will need this.
Severity: normal → enhancement
Priority: -- → P1
Blocks: 1111077
Whiteboard: DesktopWebRT2
Attached image Current UI (obsolete) —
Attached patch v0 (obsolete) — Splinter Review
Got a working example.  This is just a bare bones version of the patch that needs some UI work.

Things to do:
* Hide the forward and back buttons when they don't apply.
* Use icons instead of strings that would have to be localized (unless we already have those strings localized somewhere).
* Remove the `|| true` that allows this to work for all apps, including ones that don't have this manifest property.
* Surely the XUL namespace string is somewhere I could include.
Attachment #8549876 - Flags: review?(myk)
Attachment #8549876 - Flags: review?(mar.castelluccio)
What you can do to test:
1. Apply the patch and ./mach build
2. Install an app from that build of FF.
3. goto http://www.mykzilla.org/app/ and and install
4. Launch mykzilla
5. click "other origin"
6. click back
7. click forward
8. click back
9. click "Get Media"
10. click refresh
Looks like I should be able to use gAppBrowser rather than another document.getElementById.  Will investigate the use of nsISHistoryListener to show or hide buttons when relevant.
Attached patch v1 (obsolete) — Splinter Review
For some reason, the code: 

  gAppBrowser.sessionHistory.addSHistoryListener({
    QueryInterface: XPCOMUtils.generateQI([
      Ci.nsISHistoryListener,
      Ci.nsISupportsWeakReference
    ]),
    OnHistoryGoBack: function () {
      console.log('back');
      return true;
    },
  });

Doesn't throw, but also doesn't seem to work.  What's strange is that if I hook up remote debugging, and run that snipped in a scratch pad, I now see logging.  This is the case in attachment v1.

Going off the blame for this code [0], maybe ttaubert or smacleod know more?

[0] http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/content/content-sessionStore.js#233
Flags: needinfo?(ttaubert)
Flags: needinfo?(smacleod)
To be more clear, that snippet is what's done in attachment v1, but does not work.  I wonder if a new nsIHistory is being created or something once we load the window?  After I've launched the app and hooked up remote debugging, I can see no logging when navigating back, but if I run that same snippet in a scratchpad, I now see logging when navigating back.  This is necessary to deduce when to show or hide the navigation buttons.
"mozbrowserlocationchange" events also don't seem to work. [0]  I noticed there where other parts of the code [1] assigning events that look like they're for iframes. [2]  Seeing that addEventListener in webapp.js, I assumed maybe the <browser> XUL tag implements the same interface as iframe or something, but if I add logging statements in the call back, I don't see anything.  Not seeing the existing "mozbrowseropenwindow" event handler get called along with my own "mozbrowserlocationchange" leads me to believe that <browser> does not implement iframe, and that the existing "mozbrowseropenwindow" code simply never gets executed and should be removed.

[0] https://developer.mozilla.org/en-US/docs/Web/Events/mozbrowserlocationchange
[1] https://dxr.mozilla.org/mozilla-central/source/webapprt/content/webapp.js#121
[2] https://developer.mozilla.org/en-US/docs/Web/API/Using_the_Browser_API#Events
Hi Tim, can you point us to the Metro back button design you mentioned to me?
Flags: needinfo?(tabraldes)
This is the CSS that controls the back button:
  http://hg.mozilla.org/projects/metro/file/e6eb970ca7a4/browser/metro/theme/browser.css#l359

And the actual images for the button are all the overlay-back* files in this dir - http://hg.mozilla.org/projects/metro/file/e6eb970ca7a4/browser/metro/theme/images/

I'm not sure where the JS that controlled the back button is, but I know that it could be moved up and down on the screen (always snapped to the left of the screen). I'll pull the repo and see if I can find that code.
(In reply to Nick Desaulniers [:\n] from comment #7)
> For some reason, the code: 
> 
>   gAppBrowser.sessionHistory.addSHistoryListener({
>     QueryInterface: XPCOMUtils.generateQI([
>       Ci.nsISHistoryListener,
>       Ci.nsISupportsWeakReference
>     ]),
>     OnHistoryGoBack: function () {
>       console.log('back');
>       return true;
>     },
>   });
> 
> Doesn't throw, but also doesn't seem to work.  What's strange is that if I
> hook up remote debugging, and run that snipped in a scratch pad, I now see
> logging.  This is the case in attachment v1.

My guess is that because it's a weak ref (nsISupportsWeakReference) it is CC'ed before the listeners are called. You probably have to root it somehow.
Flags: needinfo?(ttaubert)
Attachment #8549876 - Attachment is patch: true
Attachment #8549962 - Attachment is patch: true
Comment on attachment 8549962 [details] [diff] [review]
v1

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

(In reply to Nick Desaulniers [:\n] from comment #9)
> "mozbrowserlocationchange" events also don't seem to work. [0]  I noticed there where other parts of the code [1] assigning events that look like they're for iframes. [2]  Seeing that addEventListener in webapp.js, I assumed maybe the <browser> XUL tag implements the same interface as iframe or something, but if I add logging statements in the call back, I don't see anything.

XUL <browser> elements don't implement the same API as HTML <iframe mozbrowser> elements, so I wouldn't expect "mozbrowserlocationchange" events to be dispatched to them.

They do, however, implement the nsIWebProgress interface <https://dxr.mozilla.org/mozilla-central/source/uriloader/base/nsIWebProgress.idl>, so you can observe location changes by attaching a nsIWebProgressListener <https://dxr.mozilla.org/mozilla-central/source/uriloader/base/nsIWebProgressListener.idl>.

They also implement many other interfaces, and a variety of DOM content events do bubble up to them.  Check out desktop Firefox's XULBrowserWindow object <https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4010> and Android Firefox's Tab.prototype, especially this chunk in *create* <https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3412-3444>, for examples.


> Not seeing the existing "mozbrowseropenwindow" event handler get called along with my own "mozbrowserlocationchange" leads me to believe that <browser> does not implement iframe, and that the existing "mozbrowseropenwindow" code simply never gets executed and should be removed.

Ah, indeed, that code appears to be incorrect!

::: webapprt/content/webapp.js
@@ +151,5 @@
>    document.getElementById("menu_FileQuitItem").setAttribute("label", quitLabel);
>    document.getElementById("menu_mac_hide_app").setAttribute("label", hideLabel);
> +};
> +
> +let fetchManifest = Task.async(function*() {

Nit: this does more than just fetch the manifest, it also adds chrome and updates menu items.  So I'd give it a more generic name, like configureChrome.  Alternately, simply inline this code into onLoad, since this is all code that runs "on load", i.e.:

function onLoad() {
  window.removeEventListener("load", onLoad, false);

  gAppBrowser.addProgressListener(progressListener,
                                  Ci.nsIWebProgress.NOTIFY_LOCATION |
                                  Ci.nsIWebProgress.NOTIFY_STATE_DOCUMENT);

  Task.spawn(function*() {
    yield WebappRT.configPromise;

    let manifest = WebappRT.localeManifest;
    if ("chrome" in manifest && "navigation" in manfiest.chrome &&
        manifest.chrome.navigation || true) { // XXX: remove || true !!!
      addChrome();
    }
#ifdef XP_MACOSX
    // On Mac, we dynamically create the label for the Quit menuitem, using
    // a string property to inject the name of the webapp into it.
    updateMenuItems(manifest);
#endif
  });

  gAppBrowser.addEventListener("mozbrowseropenwindow", onOpenWindow);
}

@@ +155,5 @@
> +let fetchManifest = Task.async(function*() {
> +  yield WebappRT.configPromise;
> +
> +  let manifest = WebappRT.localeManifest;
> +  if ("chrome" in manifest && "navigation" in manfiest.chrome &&

manfiest -> manifest

@@ +161,5 @@
> +    addChrome();
> +  }
> +// On Mac, we dynamically create the label for the Quit menuitem, using
> +// a string property to inject the name of the webapp into it.
> +#ifdef XP_MACOSX

Nit: since this only calls updateMenuItems #ifdef XP_MACOSX, move the comment into the #ifdef block as well, so it only appears in the processed file on Mac.

@@ +278,5 @@
> +  let toolbox = document.createElementNS(NS, "toolbox");
> +  let toolbar = document.createElementNS(NS, "toolbar");
> +  let backButton = document.createElementNS(NS, "toolbarbutton");
> +  let forwardButton = document.createElementNS(NS, "toolbarbutton");
> +  let refreshButton = document.createElementNS(NS, "toolbarbutton");

Instead of dynamically creating these elements, I'd include them in webapp.xul but hide them unless the webapp has the appropriate manifest flag.
Comment on attachment 8549876 [details] [diff] [review]
v0

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

(Cancelling these reviews, as I've pre-reviewed the followup patch.)
Attachment #8549876 - Flags: review?(myk)
Attachment #8549876 - Flags: review?(mar.castelluccio)
The "mozbrowseropenwindow" should work, there are a few tests that depend on the event being dispatched (the webapprt/test/chrome/browser_window-* tests). The tests were passing the last time I've run them (a couple of weeks ago).
> Alternately, simply inline this code into onLoad

Strange, when I inline that code, I get an "illegal character" error which does not link to the correct line as the debugger seems to be unable to open webapp.js.
Attached patch v2 (obsolete) — Splinter Review
Attachment #8549876 - Attachment is obsolete: true
Attachment #8549962 - Attachment is obsolete: true
Flags: needinfo?(tabraldes)
Flags: needinfo?(smacleod)
Attachment #8552023 - Flags: review?(myk)
Attachment #8552023 - Flags: review?(mar.castelluccio)
(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #11)
> This is the CSS that controls the back button:
>  
> http://hg.mozilla.org/projects/metro/file/e6eb970ca7a4/browser/metro/theme/
> browser.css#l359
> 
> And the actual images for the button are all the overlay-back* files in this
> dir -
> http://hg.mozilla.org/projects/metro/file/e6eb970ca7a4/browser/metro/theme/
> images/
> 
> I'm not sure where the JS that controlled the back button is, but I know
> that it could be moved up and down on the screen (always snapped to the left
> of the screen). I'll pull the repo and see if I can find that code.

Found it - http://hg.mozilla.org/projects/metro/file/e6eb970ca7a4/browser/metro/base/content/NavButtonSlider.js
Comment on attachment 8552023 [details] [diff] [review]
v2

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

It works!  And the code looks reasonable, although I still want to understand why you weren't able to inline the configureChrome task, since it works for me.  Is it possible that you indented the preprocessor directives?  That would cause them to be copied to the output file instead of processed, which would cause the error you observed.

Other than that, this needs some styling, for which you'll need to add a stylesheet, since webapp.xul doesn't have one yet (besides the global one).  Take a look at webapprt/content/downloads/download.xul for an example of that, although download.xul is more complicated, since it actually has two extra stylesheets, whereas webapp.xul only needs one, and it can live alongside webapp.xul in the webapprt/content/ directory (as can any images for the buttons).

It also needs a unit test.  Take a look at the tests in webapprt/test/chrome/ for examples you can copy to create one.  (To run the chrome unit tests, do `./mach webapprt-test-chrome`.)

Nit: Git reports this whitespace error: /Users/myk/patch.txt:119: new blank line at EOF.

Nick, I'm setting feedback rather than review, since this isn't yet a complete patch.  It's looking good, though, just needs a few more bits!

::: webapprt/content/webapp.js
@@ +63,5 @@
> +
> +    document.getElementById("forwardButton").style.display =
> +      gAppBrowser.canGoForward ? "" : "none";
> +    document.getElementById("backButton").style.display =
> +      gAppBrowser.canGoBack ? "" : "none";

Worth checking what FxOS does, but I would disable/enable these buttons rather than showing/hiding them, so they don't jump around as the user navigates back/forward.  Setting the enabled/disabled state of a XUL button just requires twiddling its *disabled* property <https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/button#p-disabled>.

::: webapprt/content/webapp.xul
@@ +170,5 @@
> +      <toolbarbutton id="backButton" label="Back"></toolbarbutton>
> +      <toolbarbutton id="forwardButton" label="Forward"></toolbarbutton>
> +      <toolbarbutton id="refreshButton" label="Refresh"></toolbarbutton>
> +    </toolbar>
> +  </toolbox>

FxOS puts this toolbar at the bottom of the screen, but I wonder if it makes more sense to put it at the top on desktop, since that's where users are used to seeing such navigation toolbars.

It's also a shame that it takes up the width of the window when it only provides a few buttons. It'd be nice to find a way to minimize its space requirements.  That would probably require some creative design and engineering, though; certainly not necessary for this first iteration.
Attachment #8552023 - Flags: review?(myk) → feedback+
> Is it possible that you indented the preprocessor directives?
That was indeed the case!
Attached image UI with text at top (obsolete) —
How should we style these? (Replace with icons else localize.)
Attachment #8549874 - Attachment is obsolete: true
Attachment #8553884 - Flags: feedback?(myk)
Assignee: nobody → nick
Attached patch v3 (obsolete) — Splinter Review
Added possible unit tests, though I can't run them due to https://bugzilla.mozilla.org/show_bug.cgi?id=1125394
Attachment #8552023 - Attachment is obsolete: true
Attachment #8552023 - Flags: review?(mar.castelluccio)
Attachment #8553999 - Flags: feedback?(myk)
Attachment #8553884 - Attachment is patch: false
Attachment #8553884 - Attachment mime type: text/plain → image/png
Comment on attachment 8553884 [details]
UI with text at top

I would style these with icons that have localizable tooltips.
Attachment #8553884 - Flags: feedback?(myk) → feedback+
Comment on attachment 8553999 [details] [diff] [review]
v3

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

The test looks good to me overall, although I too am having trouble running it on Mac due to that bug and on Linux due to what appears to be other bugs.

::: webapprt/test/chrome/window-chrome.webapp
@@ +1,1 @@
> +Content-Type: application/x-web-app-manifest+json

This needs to be valid manifest JSON, something like:

{
  "name": "Window Chrome Test Webapp",
  "description": "A webapp for testing the window chrome.",
  "launch_path": "/webapprtChrome/webapprt/test/chrome/window-chrome.html",
  "chrome": { "navigation": true }
}
Attachment #8553999 - Flags: feedback?(myk) → feedback+
oops! I copied the headers into the manifest, I'll fix that up.  Surely we don't plan to land tests we haven't run though, right?
I'll try to run the tests on my machine
Hey All, attached is our UX proposal for desktop app chrome. Take a look and let us know what you think.
(In reply to Philip from comment #28)
> Created attachment 8555643 [details]
> Desktop App Chrome Proposal.pdf
> 
> Hey All, attached is our UX proposal for desktop app chrome. Take a look and
> let us know what you think.

Lovely -- ship it!
+1 looks good
So say we all!  Regarding the question about the Favorites button, I don't think there's an analog on desktop, so I would leave that button out.  As for styling the background with the brand color, unsure if there's an explicit way to do it, but the Android runtime chooses a splash screen background color via some extrapolation from the colors in the app icon, and perhaps we could do the same.
I like it. Nice work.

The one thing that I noticed and thought I'd raise was position. On desktop we have it introduced at the top (i guess like the desktop browser) but on mobile it appears at the bottom.

Since it's chromeless without other normal desktop browser controls should we look at having it consistent on the bottom as with the mobile experience.

From personal experience when I looked at the visual — I immediately looked to the bottom of the image and it took me a while to realise that the controls were actually at the top.
I don't feel too hot about the ability to bookmark or favorite.  It makes sense when using the FxOS browser application, and you want to add an icon to your homescreen, but once an app is installed, that icon actually disappears, see attached.  In order to see the UX from the proposal, the user would have to have already installed the application, so it would not make sense to offer UI to allow them to install the app again.  This is why the favorite icon is not available to apps on FxOS that use the chrome: navigation.
Also, are the icons existing in the gecko codebase?  If not, where can I get them from?
@myk @n: Awesome, let's leave out favourite. I say we just stick with the dark grey background for the controls as pulling a colour from the app icon is too much of a crap shoot. One of those things that is great in theory but often falls flat when implemented. If you can't find the icons in the codebase, I'll create some.

@cyberdees: I have to strongly insist that the controls stay at the top, as controls on the bottom of a window is not a common pattern on desktop. Having the controls in different places based on platform is something users are used to (ex: Safari on iOS has some controls on bottom and top, Safari on OS X has controls all on top).
> Other than that, this needs some styling, for which you'll need to add a
> stylesheet, since webapp.xul doesn't have one yet (besides the global one). 
> Take a look at webapprt/content/downloads/download.xul for an example of
> that, although download.xul is more complicated, since it actually has two
> extra stylesheets, whereas webapp.xul only needs one, and it can live
> alongside webapp.xul in the webapprt/content/ directory (as can any images
> for the buttons).

Myk, when I add a file, webapprt/content/webapp.css then in webapprt/content/webapp.xul add the line:
<?xml-stylesheet href="chrome://webapprt/content/webapp.css" type="text/css"?>

I get a bunch of resource failed to load errors:
Message: Error: Request failed with status code = 2152857618 after NetUtil.asyncFetch for url = chrome://webapprt/content/webapp.css

What's the proper way to add stylesheet to xul?  webapprt/content/downloads/download.xul doesn't seem to be doing anything different.
Flags: needinfo?(myk)
You probably need to add the file to the webapprt/jar.mn manifest.
Right on, thanks Marco.  Now just need to find the icons.  How's this look so far?
Flags: needinfo?(myk)
Attached patch v4 (obsolete) — Splinter Review
Attachment #8553999 - Attachment is obsolete: true
Attached image Current UI without icons (obsolete) —
Attachment #8553884 - Attachment is obsolete: true
Hey Nick,

Those looks good, let's roll with the platform specific icons that already exist.
Flags: needinfo?(pwalmsley)
Attached image Current UI OSX
Attachment #8556173 - Attachment is obsolete: true
Attachment #8558033 - Flags: feedback?(pwalmsley)
Attached file v5 (OSX only so far) (obsolete) —
Attachment #8556172 - Attachment is obsolete: true
Looks good to me!
Attached patch v6 (obsolete) — Splinter Review
One thing to note that there is a subtle difference between platform styles:

in webapprt/themes/osx/webapp.css:

#backButton, #forwardButton {
  background-position: -40px -1px;
}

but in windows and linux:
webapprt/themes/windows/webapp.css
webapprt/themes/linux/webapp.css

#backButton, #forwardButton {
  background-position: -40px 0px;
}

This is because on OSX we have higher resolution icons to use but appear to be off by 2px when rotated 180deg.  This brings them closer together by 1px each.  This is probably subjective and should be checked.  I do have an astigmatism. 8-)
Attachment #8558046 - Attachment is obsolete: true
Attachment #8558067 - Flags: review?(myk)
Attachment #8558067 - Flags: review?(mar.castelluccio)
Attachment #8558033 - Flags: feedback?(pwalmsley)
Unfortunately the test isn't passing (it is timing out).
That's the same behavior I was seeing on OSX.
Attachment #8558067 - Attachment is patch: true
Attached patch v6 with test fixes (obsolete) — Splinter Review
Ok, I got tests running on Linux and diagnosed several test script issues.

First, the script was waiting for a window to load before starting to make test assertions and navigating, but the primary webapp window is already loaded by then, so it waited forever.

Second, the script was checking a *disabled* attribute on the button elements, but the *locationchange* listener in webapp.js is adding/removing an *active* class list member, so *disabled* didn't reflect the state of the buttons.

(Using XUL <button> elements rather than generic <box> elements and toggling the *disabled* state would be a good idea, however.)

Third, onOpenWindow in webapp.js checks the manifest for a navigation.chrome element before displaying the chrome, but that happens before the test calls loadWebapp to load the app, so navigation.chrome isn't in the manifest yet.  That probably needs to happen in the *startup* function in Startup.jsm, which gets called by loadWebapp via becomeWebapp.

Here's a version of the patch that fixes the first two issues.  I've also pushed this to my gecko-dev fork on GitHub as a branch:

https://github.com/mykmelez/gecko-dev/tree/webapp-chrome-1100442

There were a few other issues I noticed, some of which I fixed, others outstanding.  But before we go further, we really need to make it possible for you to test on all three platforms, so you can confirm fixes.
Attachment #8558067 - Flags: review?(myk)
Attachment #8558067 - Flags: review?(mar.castelluccio)
Blocks: 1116294
Attached patch v7 (obsolete) — Splinter Review
un-bitrot myk's v6
Attachment #8558067 - Attachment is obsolete: true
Attachment #8562583 - Attachment is obsolete: true
Attachment #8603071 - Flags: review?(myk)
This should be visually inspected on OSX, Windows, and Linux.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1100442#c18 for apps that have chrome or not.
On Linux, the buttons have borders and are missing their images.
Mac has the same problem.  (Still waiting on the Windows build.)
The Windows build fails with:

> 368:34.58 RuntimeError: File "downloads/downloadButtons-aero.png" not found in c:\Users\myk\Projects\gecko-dev\webapprt\themes\windows, c:\Users\myk\Projects\gecko-dev\obj-i686-pc-mingw32\webapprt\themes\windows
Whiteboard: DesktopWebRT2 → DesktopWebRT2 [dependency: marketplace-partners]
Comment on attachment 8603071 [details] [diff] [review]
v7

This appears to have bitrotted, not only as described in comment 54, comment 55, and comment 56, but also the tests are failing on my Mac with:

205 INFO TEST-PASS | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_window-chrome.js | Forward button disabled 
206 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_window-chrome.js | Back button disabled - 
Stack trace:
chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_window-chrome.js:test/<:68
chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/head.js:onLoadApp:31
null:null:0

And then later:

213 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_window-chrome.js | Test timed out - expected PASS
-*- Webapps.jsm : cancelDownload http://test/webapprtChrome/webapprt/test/chrome/window-chrome.webapp
-*- Webapps.jsm : Could not find a download for http://test/webapprtChrome/webapprt/test/chrome/window-chrome.webapp


Nick: Are you available to look into the regressions?  Since the last patch, Marco and I have landed some fixes that should make it easier.  In particular, we fixed all the existing desktop runtime chrome tests, so they're all passing now.  And the test harness now always picks your custom build on Mac, so you don't have to worry about it finding and using another installation of Nightly.

The mach command is a bit different due to some changes in the way mochitests are invoked generally.  You now invoke it with:

mach mochitest webapprt/test/chrome/
Attachment #8603071 - Flags: review?(myk)
Also, in case it helps, I maintain a branch with these changes in my Git clone of gecko-dev:

https://github.com/mykmelez/gecko-dev/tree/bug-1100442-chrome-manifest-property
Huh, I'm seeing bigger issues with DesktopRT.  Testing with Outlook, it looks like the launch path is not followed?  When I attach the remote debugger, `location` is `about:blank`.  The window opens but it is blank.  This is before I even apply my patch.  Some failed assertions:

[13052] WARNING: NS_ENSURE_TRUE(mTextInputHandler) failed: file /Users/Nicholas/mozilla/mozilla-central-git/widget/cocoa/nsChildView.mm, line 5277
[13052] WARNING: NS_ENSURE_TRUE(mTextInputHandler) failed: file /Users/Nicholas/mozilla/mozilla-central-git/widget/cocoa/nsChildView.mm, line 5277
!! Creating a dummy channel for {41546b92-d1d2-8d46-a11a-18dcfb054b44} (no appInfo)
[13052] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004001: file /Users/Nicholas/mozilla/mozilla-central-git/netwerk/base/nsNetUtil.inl, line 163
[13052] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004001: file /Users/Nicholas/mozilla/mozilla-central-git/netwerk/base/nsNetUtil.inl, line 214
[13052] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004001: file /Users/Nicholas/mozilla/mozilla-central-git/dom/base/nsFrameLoader.cpp, line 399
Anyways, the selector matching got messed up, should be a simple fix.
fun fact, XUL buttons need the CSS rule `-moz-appearance: none;` now.  Big thanks to :bgrinstead!
Attached patch v8Splinter Review
Here's a diff of v7 and v8:

1c1
< commit 5cc2ed1bb16e557f07ce4d249b7ff4fc14b8ee08
---
> commit 6ab6d9a529a6003c99f14405620554c1c4b6bbfb
3c3
< Date:   Thu May 7 16:43:34 2015 -0700
---
> Date:   Thu Jul 9 16:31:03 2015 -0700
135c135
< index 0000000..cd53664
---
> index 0000000..208c237
138c138
< @@ -0,0 +1,76 @@
---
> @@ -0,0 +1,78 @@
206c206,208
< +    ok(!backButton.classList.contains("active"), "Back button disabled");
---
> +    // unable to test that the back button is initially disabled since the
> +    // way mochitests run is by navigating forward to the next test.
> +    //ok(!backButton.classList.contains("active"), "Back button disabled");
255c257
< index 0000000..7624273
---
> index 0000000..a7b1f80
258c260
< @@ -0,0 +1,5 @@
---
> @@ -0,0 +1,8 @@
262c264,267
< +  "launch_path": "/webapprtChrome/webapprt/test/chrome/window-chrome.html"
---
> +  "launch_path": "/webapprtChrome/webapprt/test/chrome/window-chrome.html",
> +  "chrome": {
> +    "navigation": true
> +  }
282c287
< index 0000000..d2dd9b3
---
> index 0000000..5712c1e
285c290
< @@ -0,0 +1,29 @@
---
> @@ -0,0 +1,30 @@
292c297
< +#appChrome box {
---
> +#appChrome > vbox > button {
297a303
> +  -moz-appearance: none;
327c333
< index 0000000..e182cb15b
---
> index 0000000..ee3ecfd
330c336
< @@ -0,0 +1,29 @@
---
> @@ -0,0 +1,30 @@
337c343
< +#appChrome box {
---
> +#appChrome > vbox > button {
342a349
> +  -moz-appearance: none;
382c389
< index 0000000..d2dd9b3
---
> index 0000000..5712c1e
385c392
< @@ -0,0 +1,29 @@
---
> @@ -0,0 +1,30 @@
392c399
< +#appChrome box {
---
> +#appChrome > vbox > button {
397a405
> +  -moz-appearance: none;


We have some issues running the mochitests that we'll have to discuss when I get back from vacation. In the meantime, Myk, you should be able to review that the apps w/ chrome look appropriate on Windows/OSX/Linux, or wait until I'm back 7/20.
Attachment #8603071 - Attachment is obsolete: true
(In reply to Nick Desaulniers [:\n] from comment #59)
> Huh, I'm seeing bigger issues with DesktopRT.  Testing with Outlook, it
> looks like the launch path is not followed?  When I attach the remote
> debugger, `location` is `about:blank`.  The window opens but it is blank. 
> This is before I even apply my patch.

Hmm, I don't see this behavior.  On my Mac, installing and then launching Outlook causes it to start and run as expected.

> Some failed assertions:
> 
> [13052] WARNING: NS_ENSURE_TRUE(mTextInputHandler) failed: file
> /Users/Nicholas/mozilla/mozilla-central-git/widget/cocoa/nsChildView.mm,
> line 5277
> [13052] WARNING: NS_ENSURE_TRUE(mTextInputHandler) failed: file
> /Users/Nicholas/mozilla/mozilla-central-git/widget/cocoa/nsChildView.mm,
> line 5277
> !! Creating a dummy channel for {41546b92-d1d2-8d46-a11a-18dcfb054b44} (no
> appInfo)
> [13052] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004001:
> file /Users/Nicholas/mozilla/mozilla-central-git/netwerk/base/nsNetUtil.inl,
> line 163
> [13052] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004001:
> file /Users/Nicholas/mozilla/mozilla-central-git/netwerk/base/nsNetUtil.inl,
> line 214
> [13052] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004001:
> file /Users/Nicholas/mozilla/mozilla-central-git/dom/base/nsFrameLoader.cpp,
> line 399

Hmm, I don't see these, but I'm also not sure that they're related.


(In reply to Nick Desaulniers [:\n] from comment #62)
> Created attachment 8631879 [details] [diff] [review]
> v8
> 
> Here's a diff of v7 and v8:

Note that the attachment's "diff" view can show you this difference!

https://bugzilla.mozilla.org/attachment.cgi?oldid=8603071&action=interdiff&newid=8631879&headers=1


> We have some issues running the mochitests that we'll have to discuss when I
> get back from vacation.

I get a hang while running the browser_window-chrome test. Perhaps that's the same problem you're seeing?


> In the meantime, Myk, you should be able to review
> that the apps w/ chrome look appropriate on Windows/OSX/Linux, or wait until
> I'm back 7/20.

Thanks!  So far I've only looked at Mac, where buttons generally look better, except that each one displays all sprites in the image rather than its particular sprite, as shown in the attached screenshot.
Per bug 1238079, we're going to disable the desktop web runtime and remove it from the codebase, so we won't fix these bugs in it.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.